Share SyncPointManager implementation in in-process cmd buffer

gpu/command_buffer/service/sync_point_manager.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 "gpu/command_buffer/service/sync_point_manager.h"
 
 #include <climits>
 
 #include "base/logging.h"
 #include "base/rand_util.h"
 
 namespace gpu {
 
 static const int kMaxSyncBase = INT_MAX;
 
-SyncPointManager::SyncPointManager()
-    : next_sync_point_(base::RandInt(1, kMaxSyncBase)) {
+ThreadedSyncPointManager::ThreadedSyncPointManager()
+    : next_sync_point_(base::RandInt(1, kMaxSyncBase)), cond_var_(&lock_) {
   // To reduce the risk that a sync point created in a previous GPU process
   // will be in flight in the next GPU process, randomize the starting sync
   // point number. http://crbug.com/373452
 }
 
-SyncPointManager::~SyncPointManager() {
+ThreadedSyncPointManager::~ThreadedSyncPointManager() {
 }
 
-uint32 SyncPointManager::GenerateSyncPoint() {
+uint32 ThreadedSyncPointManager::GenerateSyncPoint() {
   base::AutoLock lock(lock_);
   uint32 sync_point = next_sync_point_++;
   // When an integer overflow occurs, don't return 0.
   if (!sync_point)
     sync_point = next_sync_point_++;
 
   // Note: wrapping would take days for a buggy/compromized renderer that would
   // insert sync points in a loop, but if that were to happen, better explicitly
   // crash the GPU process than risk worse.
   // For normal operation (at most a few per frame), it would take ~a year to
   // wrap.
   CHECK(sync_point_map_.find(sync_point) == sync_point_map_.end());
   sync_point_map_.insert(std::make_pair(sync_point, ClosureList()));
   return sync_point;
 }
 
-void SyncPointManager::RetireSyncPoint(uint32 sync_point) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+void ThreadedSyncPointManager::RetireSyncPoint(uint32 sync_point) {
   ClosureList list;
+  base::AutoLock lock(lock_);
+  SyncPointMap::iterator it = sync_point_map_.find(sync_point);
+  if (it == sync_point_map_.end()) {
+    LOG(ERROR) << "Attempted to retire sync point that"
+                  " didn't exist or was already retired.";
+    return;
+  }
+  list.swap(it->second);
+  sync_point_map_.erase(it);
+
+  {
+    base::AutoUnlock unlock(lock_);
+    for (ClosureList::iterator i = list.begin(); i != list.end(); ++i)
+      i->Run();
+  }
+
+  cond_var_.Broadcast();

[no sievers, 2015/01/27 21:26:39]

Can we somehow avoid dealing with the cond. var/lock overhead for the normal
multi-process use case?

[no sievers, 2015/01/27 21:26:39]

Also, it'd be slightly better to call Broadcast() before running the callbacks,
in case one of them does something synchronous wrt the GPU thread.

+}
+
+void ThreadedSyncPointManager::AddSyncPointCallback(uint32 sync_point,
+                                            const base::Closure& callback) {
   {
     base::AutoLock lock(lock_);
     SyncPointMap::iterator it = sync_point_map_.find(sync_point);
-    if (it == sync_point_map_.end()) {
-      LOG(ERROR) << "Attempted to retire sync point that"
-                    " didn't exist or was already retired.";
-      return;
-    }
-    list.swap(it->second);
-    sync_point_map_.erase(it);
-  }
-  for (ClosureList::iterator i = list.begin(); i != list.end(); ++i)
-    i->Run();
-}
-
-void SyncPointManager::AddSyncPointCallback(uint32 sync_point,
-                                            const base::Closure& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  {
-    base::AutoLock lock(lock_);
-    SyncPointMap::iterator it = sync_point_map_.find(sync_point);
     if (it != sync_point_map_.end()) {
       it->second.push_back(callback);
       return;
     }
   }
   callback.Run();
 }
 
+bool ThreadedSyncPointManager::IsSyncPointRetired(uint32 sync_point) {
+  {
+    base::AutoLock lock(lock_);
+    return IsSyncPointRetiredLocked(sync_point);
+  }
+}
+
+void ThreadedSyncPointManager::WaitSyncPoint(uint32 sync_point) {
+  base::AutoLock lock(lock_);
+  while (!IsSyncPointRetiredLocked(sync_point)) {
+    cond_var_.Wait();
+  }
+}
+
+bool ThreadedSyncPointManager::IsSyncPointRetiredLocked(uint32 sync_point) {
+  lock_.AssertAcquired();
+  SyncPointMap::iterator it = sync_point_map_.find(sync_point);
+  return it == sync_point_map_.end();
+}
+
+SyncPointManager::SyncPointManager() {
+}
+
+SyncPointManager::~SyncPointManager() {
+}
+
+uint32 SyncPointManager::GenerateSyncPoint() {
+  return threaded_sync_point_manager_.GenerateSyncPoint();
+}
+
+void SyncPointManager::RetireSyncPoint(uint32 sync_point) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  threaded_sync_point_manager_.RetireSyncPoint(sync_point);

[no sievers, 2015/01/27 21:26:39]

Since this only adds three DCHECKs(), can you either make these runtime checks
(DCHECK(allow_fully_threaded_ || thread_checker_.CalledOnValidThread()) or
create a factory function (Create() which could then check the cmdline for which
version to instantiate) and then have this wrapper impl hidden in the cc file?
Then we don't need two interfaces in the header.

+}
+
+void SyncPointManager::AddSyncPointCallback(uint32 sync_point,
+                                            const base::Closure& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  threaded_sync_point_manager_.AddSyncPointCallback(sync_point, callback);
+}
+
 bool SyncPointManager::IsSyncPointRetired(uint32 sync_point) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  {
-    base::AutoLock lock(lock_);
-    SyncPointMap::iterator it = sync_point_map_.find(sync_point);
-    return it == sync_point_map_.end();
-  }
+  return threaded_sync_point_manager_.IsSyncPointRetired(sync_point);
 }
 
 }  // namespace gpu