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 <set>
+#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 {
 
+ShareGroupContextData::IdHandlerData::IdHandlerData() : flush_generation_(0) {}
+ShareGroupContextData::IdHandlerData::~IdHandlerData() {}
+
 COMPILE_ASSERT(gpu::kInvalidResource == 0,
                INVALID_RESOURCE_NOT_0_AS_GL_EXPECTS);
 
 // The standard id handler.
 class IdHandler : public IdHandlerInterface {
  public:
   IdHandler() { }
   virtual ~IdHandler() { }
 
   // Overridden from IdHandlerInterface.
@@ skipping 10 matching lines @@
         ids[ii] = id_allocator_.AllocateIDAtOrAbove(id_offset);
         id_offset = ids[ii] + 1;
       }
     }
   }
 
   // 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]);
+    {
+      base::AutoLock auto_lock(lock_);
+
+      for (GLsizei ii = 0; ii < n; ++ii) {
+        id_allocator_.FreeID(ids[ii]);
+      }
     }
+
     (gl_impl->*delete_fn)(n, ids);

[piman, 2014/02/18 22:16:48]

Isn't there a race where another thread might Gen/Bind those IDs, and Flush
before the below Flush happens?

I think we want to keep the lock up to the Flush.

[vmiura, 2014/02/18 23:04:20]

I don't think another thread can be using the same gl_impl* at the same time. 
It's not thread safe.  Am I wrong?

[piman, 2014/02/18 23:28:26]

Not the same gl_impl, but the same share group.

Say gl1 and gl2 share the group.

thread1:
1. gl1.GenTexture(1, &id);
2. gl1.DeleteTexture(1, &id);
  a- grabs lock
  b- frees id
  c- releases lock
  d- add delete command
  e- flush

thread2:
3. gl2.GenTexture(1, &id);
  a- grabs lock
  b- finds free id and mark it as used
  c- releases lock
  d- add gen command
  e- flush


3.b could happen as soon as 2.c is done, and reuse the same id, before 2.e. A
possible ordering pattern is:

1
2.a
2.b
2.c
3.a
3.b
3.c
3.d
3.e
2.d
2.e

That could mean the GPU process sees
Gen(1, id1)
Gen(1, id1)
Delete(1, id1)

And that is wrong.

[vmiura, 2014/02/18 23:36:48]

2.b saves the id for later collection.  At this point it can't be reused by any
context.
3.b can not reuse the id until after 2.e.

[vmiura, 2014/02/18 23:42:25]

I'm sorry I am thinking of StrictIdHandler.
You're right, this is a bug for IdHandler.

[vmiura, 2014/02/19 19:39:22]

Done. Set the lock to include the delete + flush.

     // 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 {
+
+    // Delete stub must run before CollectPendingFreeIds.
+    (gl_impl->*delete_fn)(n, ids);
+
+    {
+      base::AutoLock auto_lock(lock_);
+
+      // Collect pending FreeIds from other flush_generation.
+      CollectPendingFreeIds(gl_impl);
+
+      // 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) {
+        GLuint id = ids[ii];
+        if (id != 0) {
+          DCHECK(group_freed_ids_.find(id) == group_freed_ids_.end());
+          DCHECK(id_allocator_.InUse(id));
+
+          // Save freed Id for later.
+          if (group_freed_ids_.insert(id).second)
+            ctxt_data->freed_ids_.push_back(id);
+        }
+      }
+    }
+
+    return true;
+  }
+
+  // Overridden from IdHandler.
+  virtual bool MarkAsUsedForBind(GLES2Implementation* gl_impl,
+                                 GLuint id) OVERRIDE {
 #ifndef NDEBUG
     {
       base::AutoLock auto_lock(lock_);
-      DCHECK(id == 0 || id_allocator_.InUse(id));
+      if (id != 0) {
+        DCHECK(group_freed_ids_.find(id) == group_freed_ids_.end());
+        DCHECK(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_);
+    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) {
+        const GLuint id = ctxt_data->freed_ids_[ii];
+        DCHECK(group_freed_ids_.find(id) != group_freed_ids_.end());
+        group_freed_ids_.erase(id);
+        id_allocator_.FreeID(id);
+      }
+      ctxt_data->freed_ids_.clear();
+    }
+  }
+
+  int id_namespace_;
+
+  base::Lock lock_;
+  IdAllocator id_allocator_;
+  std::set<GLuint> group_freed_ids_;

[piman, 2014/02/18 22:16:48]

Adding one more set seems overkill, it sounds like you'd want the entries in
id_allocator_.used_ids_ or free_ids_ to have a flag "deleted_but_dont_reuse".

Looking at it closer, for the StrictIdHandler, we could simplify dramatically
because we don't need to handle the Bind-creates-id case:
- replace the sets (used_ids_, free_ids_) by a single vector (indexed by id) of
a tri-state for the id (IN_USE vs FREE vs DELETED_WAITING_FOR_FLUSH).
- keep a free list for allocating faster, but it can be something very efficient
like a queue or a stack.

Allocations and frees would become O(1). I think it would make the code clearer
too.

[vmiura, 2014/02/18 23:04:20]

Will do.

[vmiura, 2014/02/18 23:28:01]

Done.

 };
 
 // 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(
       GLES2Implementation* /* gl_impl */,
       GLuint id_offset, GLsizei n, GLuint* ids) OVERRIDE {
     base::AutoLock auto_lock(lock_);
     for (GLsizei ii = 0; ii < n; ++ii) {
       ids[ii] = ++last_id_ + id_offset;
     }
   }
 
   // Overridden from IdHandlerInterface.
   virtual bool FreeIds(
       GLES2Implementation* gl_impl,
       GLsizei n, const GLuint* ids, DeleteFn delete_fn) OVERRIDE {
     // Ids are never freed.
     (gl_impl->*delete_fn)(n, ids);
     return true;
   }
 
   // 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_;
 };
 
 ShareGroup::ShareGroup(bool bind_generates_resource)
     : bind_generates_resource_(bind_generates_resource) {
   if (bind_generates_resource) {
     for (int i = 0; i < id_namespaces::kNumIdNamespaces; ++i) {
       if (i == id_namespaces::kProgramsAndShaders) {
         id_handlers_[i].reset(new NonReusedIdHandler());
       } else {
         id_handlers_[i].reset(new IdHandler());
       }
     }
   } else {
     for (int i = 0; i < id_namespaces::kNumIdNamespaces; ++i) {
       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));
       }
     }
   }
   program_info_manager_.reset(ProgramInfoManager::Create(false));
 }
 
 void ShareGroup::set_program_info_manager(ProgramInfoManager* manager) {
   program_info_manager_.reset(manager);
 }
 
 ShareGroup::~ShareGroup() {}
 
 }  // namespace gles2
 }  // namespace gpu