Added global order numbers to in process command buffers.

gpu/command_buffer/service/sync_point_manager.h

Index: gpu/command_buffer/service/sync_point_manager.h
diff --git a/gpu/command_buffer/service/sync_point_manager.h b/gpu/command_buffer/service/sync_point_manager.h
index 98e849b76ced244c3400b171d12cf513c4f4f739..dc462f21af70e94df543b17a7df51cb0a61d04cf 100644
--- a/gpu/command_buffer/service/sync_point_manager.h
+++ b/gpu/command_buffer/service/sync_point_manager.h
@@ -7,8 +7,11 @@
 
 #include <vector>
 
+#include "base/atomic_sequence_num.h"
 #include "base/callback.h"
 #include "base/containers/hash_tables.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
@@ -16,6 +19,59 @@
 
 namespace gpu {
 
+class SyncPointManager;
+
+enum SyncPointNamespace {
+  kSyncPointNamespace_GpuIO,
+  kSyncPointNamespace_InProcess,
+
+  NUM_SYNC_POINT_NAMESPACES
+};
+
+class GPU_EXPORT SyncPointClient
+    : public base::RefCountedThreadSafe<SyncPointClient> {
+ public:
+  void BeginProcessingOrderNumber(uint32_t order_num) {
+    DCHECK_GE(current_order_num_, order_num);
+    current_order_num_ = order_num;
+  }
+
+  void FinishProcessingOrderNumber(uint32_t order_num) {
+    DCHECK_EQ(current_order_num_, order_num);
+    DCHECK_GT(processed_order_num(), order_num);
+    base::subtle::Release_Store(&processed_order_num_, order_num);
+  }
+
+  uint32_t processed_order_num() const {
+    return base::subtle::Acquire_Load(&processed_order_num_);
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<SyncPointClient>;
+  friend class SyncPointManager;
+
+  SyncPointClient(SyncPointManager* sync_point_manager,
+                  SyncPointNamespace namespace_id, uint64_t client_id);
+
+ protected:
+  virtual ~SyncPointClient();
+
+  // Sync Point Manager is guaranteed to outlive the sync point client.

[piman, 2015/09/14 23:43:53]

This seems hard to guarantee if SyncPointClient is RefCountedThreadSafe. E.g. a
PostTask could keep it alive for an arbitrary long time.

I think it may be simpler to have an explicit create/destroy (or
register/unregister)  to manage the client map... I think you only need the
RefCountedThreadSafe property for accesses to processed_order_num_, right? For
the map entry, the owner knows the exact lifetime.

[David Yen, 2015/09/15 18:09:31]

Hmm, later functions probably do need a pointer to the sync point manager. For
example we will probably need the sync point clients to handle the waiting since
it needs to handle the condition variable as the current sync point
implementation does.

I also remembered I forgot to add in the "unprocessed order number", I made it
so generate order number must be called from the client and that will both
generate a new order number and assign it as an "unprocessed" one.

Calls that use the sync_point_manager_ pointer are all from the command buffer
that is in charge of destroying it though, so it should be safe for the command
buffer to manage when it can call these functions. I'm not sure what the best
way to validate/DCHECK that condition though, I have put in a DCHECK on
sync_point_manager_ now but technically that can be racey since it can be
destroyed on another thread.

[piman, 2015/09/15 23:47:15]

One way to enforce proper behavior is to split the class in 2 parts, separating
the state that is shared across threads
(processed_order_num_/unprocessed_order_num_) from the non-shared state
(everything else). The shared state would stay in a RefCountedThreadSafe class,
but the rest would be in a single-ownership class, that keeps a reference to the
shared state.
E.g. SyncPointClient (the single-ownership part) owns a SyncPointClientState
(the RefCountedThreadSafe shared part). Instead of
SyncPointManager::GetSyncPointClient, you'd have a GetSyncPointClientState that
would (under lock) lookup the client and return its shared state.

[David Yen, 2015/09/18 18:43:59]

Done. Since SyncPointClient is no longer ref counted now, I also reverted back
to the original model where it unregisters itself on destruction.

+  SyncPointManager* sync_point_manager_;
+
+  // Unique namespace/client id pair for this sync point client.
+  SyncPointNamespace namespace_id_;
+  uint64_t client_id_;
+
+  // Current IPC order number being processed (only used on processing thread).
+  uint32_t current_order_num_;
+
+  // Last finished IPC order number.
+  base::subtle::Atomic32 processed_order_num_;
+
+  DISALLOW_COPY_AND_ASSIGN(SyncPointClient);
+};
+
 // This class manages the sync points, which allow cross-channel
 // synchronization.
 class GPU_EXPORT SyncPointManager {
@@ -23,6 +79,17 @@ class GPU_EXPORT SyncPointManager {
   explicit SyncPointManager(bool allow_threaded_wait);
   ~SyncPointManager();
 
+  // Creates a sync point client which message processors should hold.
+  scoped_refptr<SyncPointClient> CreateSyncPointClient(
+      SyncPointNamespace namespace_id, uint64_t client_id);
+
+  // Finds an already created sync point client.
+  scoped_refptr<SyncPointClient> GetSyncPointClient(
+    SyncPointNamespace namespace_id, uint64_t client_id);
+
+  // Generate a global order number, messages need to be ordered by flush order.
+  uint32_t GenerateOrderNumber();
+
   // Generates a sync point, returning its ID. This can me called on any thread.
   // IDs start at a random number. Never return 0.
   uint32 GenerateSyncPoint();
@@ -44,14 +111,25 @@ class GPU_EXPORT SyncPointManager {
   void WaitSyncPoint(uint32 sync_point);
 
  private:
+  friend class SyncPointClient;
+
   typedef std::vector<base::Closure> ClosureList;
   typedef base::hash_map<uint32, ClosureList> SyncPointMap;
-
+  typedef base::hash_map<uint64_t, SyncPointClient*> ClientMap;
 
   bool IsSyncPointRetiredLocked(uint32 sync_point);
+  void RemoveSyncPointClient(SyncPointNamespace namespace_id,
+                             uint64_t client_id);
 
   const bool allow_threaded_wait_;
 
+  // Order number is global for all clients.
+  base::AtomicSequenceNumber global_order_num_;
+
+  // Client map holds a map of clients id to client for each namespace.
+  base::Lock client_maps_lock_[NUM_SYNC_POINT_NAMESPACES];

[piman, 2015/09/14 23:43:53]

nit: it seems slightly overkill to have separate locks, given that the locked
operations are fairly rare, so you will have little contention - if any.

[David Yen, 2015/09/15 18:09:31]

Done.

+  ClientMap client_maps_[NUM_SYNC_POINT_NAMESPACES];
+
   // Protects the 2 fields below. Note: callbacks shouldn't be called with this
   // held.
   base::Lock lock_;