Added global order numbers to in process command buffers.

gpu/command_buffer/service/sync_point_manager.h

 // 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.
 
 #ifndef GPU_COMMAND_BUFFER_SERVICE_SYNC_POINT_MANAGER_H_
 #define GPU_COMMAND_BUFFER_SERVICE_SYNC_POINT_MANAGER_H_
 
 #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"
 #include "gpu/gpu_export.h"
 
 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 {
  public:
   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();
 
   // Retires a sync point. This will call all the registered callbacks for this
   // sync point. This can only be called on the main thread.
   void RetireSyncPoint(uint32 sync_point);
 
   // Adds a callback to the sync point. The callback will be called when the
   // sync point is retired, or immediately (from within that function) if the
   // sync point was already retired (or not created yet). This can only be
   // called on the main thread.
   void AddSyncPointCallback(uint32 sync_point, const base::Closure& callback);
 
   bool IsSyncPointRetired(uint32 sync_point);
 
   // Block and wait until a sync point is signaled. This is only useful when
   // the sync point is signaled on another thread.
   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_;
   SyncPointMap sync_point_map_;
   uint32 next_sync_point_;
   base::ConditionVariable retire_cond_var_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncPointManager);
 };
 
 }  // namespace gpu
 
 #endif  // GPU_COMMAND_BUFFER_SERVICE_SYNC_POINT_MANAGER_H_