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

gpu/command_buffer/client/share_group.cc

Index: gpu/command_buffer/client/share_group.cc
diff --git a/gpu/command_buffer/client/share_group.cc b/gpu/command_buffer/client/share_group.cc
index 8711ce817e64485ac76cf65115bb737be88bc8b3..b0a56a13b0611814a59e2fca89aad58080400832 100644
--- a/gpu/command_buffer/client/share_group.cc
+++ b/gpu/command_buffer/client/share_group.cc
@@ -2,6 +2,8 @@
 // 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"
@@ -13,6 +15,9 @@
 namespace gpu {
 namespace gles2 {
 
+ShareGroupContextData::IdHandlerData::IdHandlerData() : flush_generation_(0) {}
+ShareGroupContextData::IdHandlerData::~IdHandlerData() {}
+
 COMPILE_ASSERT(gpu::kInvalidResource == 0,
                INVALID_RESOURCE_NOT_0_AS_GL_EXPECTS);
 
@@ -50,39 +55,122 @@ class IdHandler : public IdHandlerInterface {
     (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 {
+  virtual bool MarkAsUsedForBind(GLES2Implementation* /* gl_impl */,
+                                 GLuint id) OVERRIDE {
     if (id == 0)
       return true;
     base::AutoLock auto_lock(lock_);
     return id_allocator_.MarkAsUsed(id);
   }
 
- protected:
+  virtual void FreeContext(GLES2Implementation* gl_impl) OVERRIDE {}
+
+ private:
   base::Lock lock_;
   IdAllocator id_allocator_;
 };
 
 // An id handler that requires Gen before Bind.
-class StrictIdHandler : public IdHandler {
+class StrictIdHandler : public IdHandlerInterface {
  public:
-  StrictIdHandler() {}
+  explicit StrictIdHandler(int id_namespace) : id_namespace_(id_namespace) {}
   virtual ~StrictIdHandler() {}
 
   // Overridden from IdHandler.
-  virtual bool MarkAsUsedForBind(GLuint id) OVERRIDE {
+  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.
+    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;
+      }
+    }
+  }
+
+  // Overridden from IdHandler.
+  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);

[vmiura, 2014/02/16 17:41:31]

Bug: CollectPendingFreeIds() is recording the current 'flush_generation_', 
however the glDelete*() command itself is written to the command buffer AFTER
this FreeIds() returns.

If an internal CommandBufferHelper::Flush() occurs in
CommandBufferHelper::GetSpace() then the 'flush_generation_' will change before
the glDelete* command is flushed.

[vmiura, 2014/02/16 20:28:01]

Correction, the gl_impl->*delete_fn() call is causing the internal flush.

Need to move the gl_impl->*delete_fn() call before calling
CollectPendingFreeIds().

[vmiura, 2014/02/18 21:15:01]

Fixed by moving order of gl_impl->*delete_fn().

+
+    // Save Ids to free in a later flush_generation.
+    ShareGroupContextData::IdHandlerData* ctxt_data =
+        gl_impl->share_group_context_data()->id_handler_data(id_namespace_);
+    for (GLsizei ii = 0; ii < n; ++ii) {
+      ctxt_data->freed_ids_.push_back(ids[ii]);
+    }
+

[vmiura, 2014/02/16 17:41:31]

Bug: Keeping freed_ids_ per context is not sufficient in this case:

ctxt1:
  glGenTextures() -> id1
  glFlush()
  glDeleteTextures(id1)  // adds id1 to |ctxt1| freed_ids_

ctxt2:
  glDeleteTextures(id1)  // adds id1 to |ctxt2| freed_ids_

-> Subsequently id1 may be freed and re-used twice.

A set of freed_ids_ will be needed both per-context and per-share_group to avoid
this.

[vmiura, 2014/02/18 20:08:08]

I see that deleting the same Ids twice is a GL error in this case, so instead of
handling it I will add a DCHECK here.

[vmiura, 2014/02/18 21:15:01]

Fixed by adding DCHECKs.

+    (gl_impl->*delete_fn)(n, ids);
+    return true;
+  }
+
+  // Overridden from IdHandler.
+  virtual bool MarkAsUsedForBind(GLES2Implementation* gl_impl,
+                                 GLuint id) OVERRIDE {
 #ifndef NDEBUG
     {
       base::AutoLock auto_lock(lock_);
+      ShareGroupContextData::IdHandlerData* ctxt_data =
+          gl_impl->share_group_context_data()->id_handler_data(id_namespace_);
+      for (uint32 ii = 0; ii < ctxt_data->freed_ids_.size(); ++ii) {
+        DCHECK(id == 0 || id != ctxt_data->freed_ids_[ii]);
+      }
       DCHECK(id == 0 || id_allocator_.InUse(id));
     }
+#else
+    (void)gl_impl;
 #endif
     return true;
   }
+
+  // Overridden from IdHandlerInterface.
+  virtual void FreeContext(GLES2Implementation* gl_impl) OVERRIDE {
+    base::AutoLock auto_lock(lock_);

[vmiura, 2014/02/16 17:41:31]

Bug: This lock was missing.

[vmiura, 2014/02/18 21:15:01]

Done.

+    CollectPendingFreeIds(gl_impl);
+  }
+
+ private:
+  void CollectPendingFreeIds(GLES2Implementation* gl_impl) {
+    uint32 flush_generation = gl_impl->helper()->flush_generation();
+    ShareGroupContextData::IdHandlerData* ctxt_data =
+        gl_impl->share_group_context_data()->id_handler_data(id_namespace_);
+
+    if (ctxt_data->flush_generation_ != flush_generation) {
+      ctxt_data->flush_generation_ = flush_generation;
+      for (uint32 ii = 0; ii < ctxt_data->freed_ids_.size(); ++ii) {
+        id_allocator_.FreeID(ctxt_data->freed_ids_[ii]);
+      }
+      ctxt_data->freed_ids_.clear();
+    }
+  }
+
+  int id_namespace_;
+
+  base::Lock lock_;
+  IdAllocator id_allocator_;
 };
 
 // An id handler for ids that are never reused.
@@ -111,11 +199,14 @@ class NonReusedIdHandler : public IdHandlerInterface {
   }
 
   // Overridden from IdHandlerInterface.
-  virtual bool MarkAsUsedForBind(GLuint /* id */) OVERRIDE {
+  virtual bool MarkAsUsedForBind(GLES2Implementation* /* gl_impl */,
+                                 GLuint /* id */) OVERRIDE {
     // This is only used for Shaders and Programs which have no bind.
     return false;
   }
 
+  virtual void FreeContext(GLES2Implementation* gl_impl) OVERRIDE {}
+
  private:
   base::Lock lock_;
   GLuint last_id_;
@@ -136,7 +227,7 @@ ShareGroup::ShareGroup(bool bind_generates_resource)
       if (i == id_namespaces::kProgramsAndShaders) {
         id_handlers_[i].reset(new NonReusedIdHandler());
       } else {
-        id_handlers_[i].reset(new StrictIdHandler());
+        id_handlers_[i].reset(new StrictIdHandler(i));
       }
     }
   }