Reduce internal Flush() in GL resource glGen/Delete APIs.

gpu/command_buffer/client/share_group.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <vector>
+
 #include "gpu/command_buffer/client/share_group.h"
 
 #include "base/logging.h"
 #include "base/synchronization/lock.h"
 #include "gpu/command_buffer/client/gles2_implementation.h"
 #include "gpu/command_buffer/client/program_info_manager.h"
 #include "gpu/command_buffer/common/id_allocator.h"
 
 namespace gpu {
 namespace gles2 {
@@ skipping 14 matching lines @@
     base::AutoLock auto_lock(lock_);
     if (id_offset == 0) {
       for (GLsizei ii = 0; ii < n; ++ii) {
         ids[ii] = id_allocator_.AllocateID();
       }
     } else {
       for (GLsizei ii = 0; ii < n; ++ii) {
         ids[ii] = id_allocator_.AllocateIDAtOrAbove(id_offset);
         id_offset = ids[ii] + 1;
       }
     }

[piman, 2014/02/12 23:52:34]

We should restore the Flush() here that was removed from the command buffer
generator, for the Pepper case, otherwise it would violate the spec if one
context gens (without flushing) and another context binds the texture id (should
not cause GL_INVALID_OPERATION).

[vmiura, 2014/02/13 00:20:17]

OK, but not here since we have to flush after the Gen* command is written in the
command buffer.

I'll add flush back in the command buffer generator, conditional on
bind_generate_resource.

[piman, 2014/02/13 00:48:53]

Oh, yeah.

Or be more symmetrical with FreeIds and provide a functor. whichever is easy.

   }
 
   // Overridden from IdHandlerInterface.
   virtual bool FreeIds(
       GLES2Implementation* gl_impl,
       GLsizei n, const GLuint* ids, DeleteFn delete_fn) OVERRIDE {
     base::AutoLock auto_lock(lock_);
     for (GLsizei ii = 0; ii < n; ++ii) {
       id_allocator_.FreeID(ids[ii]);
     }
     (gl_impl->*delete_fn)(n, ids);
     // We need to ensure that the delete call is evaluated on the service side
     // before any other contexts issue commands using these client ids.
+    // TODO(vmiura): Can remove this by virtualizing internal ids, however
+    // this code only affects PPAPI for now.
     gl_impl->helper()->CommandBufferHelper::Flush();
     return true;
   }
 
   // Overridden from IdHandlerInterface.
   virtual bool MarkAsUsedForBind(GLuint id) OVERRIDE {
     if (id == 0)
       return true;
     base::AutoLock auto_lock(lock_);
     return id_allocator_.MarkAsUsed(id);
   }
 
  protected:
   base::Lock lock_;
   IdAllocator id_allocator_;
 };
 
 // An id handler that requires Gen before Bind.
 class StrictIdHandler : public IdHandler {

[piman, 2014/02/12 23:52:34]

nit: at this point, there's little advantage to inherit from IdHandler, rather
inherit from IdHandlerInterface?
That way, IdHandler fields can become private.

[vmiura, 2014/02/14 00:11:11]

Done.

  public:
-  StrictIdHandler() {}
+  StrictIdHandler() : pending_flush_generation_(0) {}
   virtual ~StrictIdHandler() {}
 
+  // Overridden from IdHandlerInterface.
+  virtual void MakeIds(GLES2Implementation* gl_impl,
+                       GLuint id_offset,
+                       GLsizei n,
+                       GLuint* ids) OVERRIDE {
+    base::AutoLock auto_lock(lock_);
+
+    // Collect pending FreeIds from other flush_generation.
+    CollectPendingFreeIds(gl_impl);
+
+    // Allocate Ids.
+    for (GLsizei ii = 0; ii < n; ++ii) {
+      ids[ii] = id_allocator_.AllocateID();
+    }
+  }
+
+  // Overridden from IdHandlerInterface.
+  virtual bool FreeIds(GLES2Implementation* gl_impl,
+                       GLsizei n,
+                       const GLuint* ids,
+                       DeleteFn delete_fn) OVERRIDE {
+    base::AutoLock auto_lock(lock_);
+
+    // Collect pending FreeIds from other flush_generation.
+    CollectPendingFreeIds(gl_impl);
+
+    // Save Ids to free in a later flush_generation.
+    for (GLsizei ii = 0; ii < n; ++ii) {
+      pending_free_ids_.push_back(ids[ii]);
+    }
+
+    (gl_impl->*delete_fn)(n, ids);
+    return true;
+  }
+
   // Overridden from IdHandler.
   virtual bool MarkAsUsedForBind(GLuint id) OVERRIDE {
 #ifndef NDEBUG
     {
       base::AutoLock auto_lock(lock_);
       DCHECK(id == 0 || id_allocator_.InUse(id));

[piman, 2014/02/12 23:52:34]

Can we update the DCHECK to take into account the ids in pending_free_ids_?

[vmiura, 2014/02/14 00:11:11]

Done.

     }
 #endif
     return true;
   }
+
+ private:
+  void CollectPendingFreeIds(GLES2Implementation* gl_impl) {
+    uint32 flush_generation = gl_impl->helper()->flush_generation();
+    if (pending_flush_generation_ != flush_generation) {

[piman, 2014/02/12 23:52:34]

gl_impl->helper()->flush_generation() is per-command buffer (i.e. per context),
whereas |this|, hence |pending_flush_generation_| is per share group, so it
doesn't really make sense to compare those (it'll do the wrong thing).

I suppose you could keep a per-command buffer {pending_free_ids_,
pending_flush_generation_} struct...

There's another issue which is that you only collect on subsequent Gen/Delete,
which means that, for example, if a command buffer gets destroyed, we won't ever
reclaim the ids that were last deleted.

[vmiura, 2014/02/13 00:20:17]

Great point.

I think this pending free handling can move up into GLES2Implementation.

[vmiura, 2014/02/14 00:11:11]

Moved these into per-GLES2Implementation data, but same idea.

+      pending_flush_generation_ = flush_generation;
+      for (uint32 ii = 0; ii < pending_free_ids_.size(); ++ii) {
+        id_allocator_.FreeID(pending_free_ids_[ii]);
+      }
+      pending_free_ids_.clear();
+    }
+  }
+
+  std::vector<GLuint> pending_free_ids_;
+  uint32 pending_flush_generation_;
 };
 
 // An id handler for ids that are never reused.
 class NonReusedIdHandler : public IdHandlerInterface {
  public:
   NonReusedIdHandler() : last_id_(0) {}
   virtual ~NonReusedIdHandler() {}
 
   // Overridden from IdHandlerInterface.
   virtual void MakeIds(
@@ skipping 48 matching lines @@
 }
 
 void ShareGroup::set_program_info_manager(ProgramInfoManager* manager) {
   program_info_manager_.reset(manager);
 }
 
 ShareGroup::~ShareGroup() {}
 
 }  // namespace gles2
 }  // namespace gpu