Share SyncPointManager implementation in in-process cmd buffer

gpu/command_buffer/service/in_process_command_buffer.cc

 // Copyright 2013 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 "gpu/command_buffer/service/in_process_command_buffer.h"
 
 #include <queue>
 #include <set>
 #include <utility>
 
@@ skipping 13 matching lines @@
 #include "gpu/command_buffer/service/context_group.h"
 #include "gpu/command_buffer/service/gl_context_virtual.h"
 #include "gpu/command_buffer/service/gpu_scheduler.h"
 #include "gpu/command_buffer/service/gpu_switches.h"
 #include "gpu/command_buffer/service/image_factory.h"
 #include "gpu/command_buffer/service/image_manager.h"
 #include "gpu/command_buffer/service/mailbox_manager_impl.h"
 #include "gpu/command_buffer/service/mailbox_manager_sync.h"
 #include "gpu/command_buffer/service/memory_tracking.h"
 #include "gpu/command_buffer/service/query_manager.h"
+#include "gpu/command_buffer/service/sync_point_manager.h"
 #include "gpu/command_buffer/service/transfer_buffer_manager.h"
 #include "gpu/command_buffer/service/valuebuffer_manager.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gl/gl_context.h"
 #include "ui/gl/gl_image.h"
 #include "ui/gl/gl_share_group.h"
 
 #if defined(OS_ANDROID)
 #include "gpu/command_buffer/service/stream_texture_manager_in_process_android.h"
 #include "ui/gl/android/surface_texture.h"
@@ skipping 78 matching lines @@
 
 class ScopedEvent {
  public:
   ScopedEvent(base::WaitableEvent* event) : event_(event) {}
   ~ScopedEvent() { event_->Signal(); }
 
  private:
   base::WaitableEvent* event_;
 };
 
-class SyncPointManager {
+class ThreadedSyncPointManager {

[no sievers, 2015/01/16 20:03:37]

nit: put a class comment that explains that 'threaded' here means that it
supports retiring sync points on different threads.

  public:
-  SyncPointManager();
-  ~SyncPointManager();
+  ThreadedSyncPointManager();
+  ~ThreadedSyncPointManager();
 
+  // SyncPointManager methods.

[no sievers, 2015/01/16 20:03:37]

nit: you are not subclassing SyncPointManager

   uint32 GenerateSyncPoint();
   void RetireSyncPoint(uint32 sync_point);
+  void AddSyncPointCallback(uint32 sync_point, const base::Closure& callback);
+  bool IsSyncPointRetired(uint32 sync_point);
 
-  bool IsSyncPointPassed(uint32 sync_point);
   void WaitSyncPoint(uint32 sync_point);
 
-private:
-  // This lock protects access to pending_sync_points_ and next_sync_point_ and
-  // is used with the ConditionVariable to signal when a sync point is retired.
+ private:
   base::Lock lock_;
-  std::set<uint32> pending_sync_points_;
-  uint32 next_sync_point_;
   base::ConditionVariable cond_var_;
+  scoped_refptr<SyncPointManager> sync_point_manager_;
 };
 
-SyncPointManager::SyncPointManager() : next_sync_point_(1), cond_var_(&lock_) {}
-
-SyncPointManager::~SyncPointManager() {
-  DCHECK_EQ(pending_sync_points_.size(), 0U);
+ThreadedSyncPointManager::ThreadedSyncPointManager()
+    : cond_var_(&lock_), sync_point_manager_(new SyncPointManager) {
 }
 
-uint32 SyncPointManager::GenerateSyncPoint() {
-  base::AutoLock lock(lock_);
-  uint32 sync_point = next_sync_point_++;
-  DCHECK_EQ(pending_sync_points_.count(sync_point), 0U);
-  pending_sync_points_.insert(sync_point);
-  return sync_point;
+ThreadedSyncPointManager::~ThreadedSyncPointManager() {
 }
 
-void SyncPointManager::RetireSyncPoint(uint32 sync_point) {
+uint32 ThreadedSyncPointManager::GenerateSyncPoint() {
   base::AutoLock lock(lock_);

[no sievers, 2015/01/16 20:03:37]

This doesn't need the lock since SyncPointManager::GenerateSyncPoint() already
takes one internally (and is expected to be called on the IO thread instead of
the main thread in Chrome proper).

-  DCHECK(pending_sync_points_.count(sync_point));
-  pending_sync_points_.erase(sync_point);
+  return sync_point_manager_->GenerateSyncPoint();
+}
+
+void ThreadedSyncPointManager::RetireSyncPoint(uint32 sync_point) {
+  base::AutoLock lock(lock_);
+  sync_point_manager_->RetireSyncPoint(sync_point);

[no sievers, 2015/01/16 20:03:37]

However the other three calls will trigger the CalledOnValidThread() check in
SyncPointManager.
One thing we could do is move this code into sync_point_manager.cc and then have
two internal wrapper implementation classes: one with thread checks where it
expects only one service thread, and then this version.

   cond_var_.Broadcast();
 }
 
-bool SyncPointManager::IsSyncPointPassed(uint32 sync_point) {
+void ThreadedSyncPointManager::AddSyncPointCallback(
+    uint32 sync_point,
+    const base::Closure& callback) {
   base::AutoLock lock(lock_);
-  return pending_sync_points_.count(sync_point) == 0;
+  sync_point_manager_->AddSyncPointCallback(sync_point, callback);
 }
 
-void SyncPointManager::WaitSyncPoint(uint32 sync_point) {
+bool ThreadedSyncPointManager::IsSyncPointRetired(uint32 sync_point) {
   base::AutoLock lock(lock_);
-  while (pending_sync_points_.count(sync_point)) {
+  return sync_point_manager_->IsSyncPointRetired(sync_point);
+}
+
+void ThreadedSyncPointManager::WaitSyncPoint(uint32 sync_point) {
+  base::AutoLock lock(lock_);
+  while (!sync_point_manager_->IsSyncPointRetired(sync_point)) {
     cond_var_.Wait();

[no sievers, 2015/01/16 20:18:25]

We also have to think about the case where we mix the proper GPU thread with the
in-process stuff (like we intend to do). Then this thread here will wait on a
sync point that will likely be retired by the other stack, so it needs to
broadcast the condition variable.

[boliu, 2015/01/16 20:33:30]

Broadcast already cover all the waiting threads. Is that enough?

   }
 }
 
-base::LazyInstance<SyncPointManager> g_sync_point_manager =
+base::LazyInstance<ThreadedSyncPointManager> g_sync_point_manager =
     LAZY_INSTANCE_INITIALIZER;
 
 base::SharedMemoryHandle ShareToGpuThread(
     base::SharedMemoryHandle source_handle) {
 #if defined(OS_WIN)
   // Windows needs to explicitly duplicate the handle to current process.
   base::SharedMemoryHandle target_handle;
   if (!DuplicateHandle(GetCurrentProcess(),
                        source_handle,
                        GetCurrentProcess(),
@@ skipping 639 matching lines @@
   g_sync_point_manager.Get().WaitSyncPoint(sync_point);
   gles2::MailboxManager* mailbox_manager =
       decoder_->GetContextGroup()->mailbox_manager();
   mailbox_manager->PullTextureUpdates(sync_point);
   return true;
 }
 
 void InProcessCommandBuffer::SignalSyncPointOnGpuThread(
     unsigned sync_point,
     const base::Closure& callback) {
-  if (g_sync_point_manager.Get().IsSyncPointPassed(sync_point)) {
-    callback.Run();
-  } else {
-    service_->ScheduleIdleWork(
-        base::Bind(&InProcessCommandBuffer::SignalSyncPointOnGpuThread,
-                   gpu_thread_weak_ptr_,
-                   sync_point,
-                   callback));
-  }
+  g_sync_point_manager.Get().AddSyncPointCallback(sync_point, callback);
 }
 
 void InProcessCommandBuffer::SignalQuery(unsigned query_id,
                                          const base::Closure& callback) {
   CheckSequencedThread();
   QueueTask(base::Bind(&InProcessCommandBuffer::SignalQueryOnGpuThread,
                        base::Unretained(this),
                        query_id,
                        WrapCallback(callback)));
 }
@@ skipping 81 matching lines @@
 
 #if defined(OS_ANDROID)
 scoped_refptr<gfx::SurfaceTexture>
 InProcessCommandBuffer::GetSurfaceTexture(uint32 stream_id) {
   DCHECK(stream_texture_manager_);
   return stream_texture_manager_->GetSurfaceTexture(stream_id);
 }
 #endif
 
 }  // namespace gpu