Improve GamepadSeqLock impl

content/common/gamepad_seqlock.h

Index: content/common/gamepad_seqlock.h
===================================================================
--- content/common/gamepad_seqlock.h	(revision 122820)
+++ content/common/gamepad_seqlock.h	(working copy)
@@ -6,38 +6,75 @@
 #define CONTENT_COMMON_GAMEPAD_SEQLOCK_H_
 
 #include "base/atomicops.h"
-#include "base/threading/platform_thread.h"
-#include "content/common/content_export.h"
+#include "base/basictypes.h"
 
 namespace content {
 
+namespace internal {
+
+class GamepadSeqLockBase {
+ protected:
+  static const size_t kEntries = 2;
+  typedef base::subtle::Atomic32 Atomic32;
+  struct BaseEntry {
+    Atomic32 sequence_;
+    // Both writer and readers work with the array concurrently,
+    // so it must be accessed with atomic operations.
+    Atomic32 data_[];
+  };
+
+  GamepadSeqLockBase(BaseEntry* entries, size_t size);
+  void ReadTo(Atomic32* obj);
+  void Write(const Atomic32* obj);
+
+ private:
+  const size_t size_;
+  Atomic32 current_;
+  BaseEntry* entries_[kEntries];
+};
+
+}  // namespace internal
+
 // This SeqLock handles only *one* writer and multiple readers. It may be
-// suitable for low-contention with relatively infrequent writes, and many
-// readers. See:
+// suitable for high read frequency scenarios and is especially useful in shared
+// memory environment. See for the basic idea:
 //   http://en.wikipedia.org/wiki/Seqlock
-//   http://www.concurrencykit.org/doc/ck_sequence.html
-// This implementation is based on ck_sequence.h from http://concurrencykit.org.
-//
-// Currently, this is used in only one location. It may make sense to
-// generalize with a higher-level construct that owns both the lock and the
-// data buffer, if it is to be used more widely.
-//
-// You must be very careful not to operate on potentially inconsistent read
-// buffers. If the read must be retry'd, the data in the read buffer could
-// contain any random garbage. e.g., contained pointers might be
-// garbage, or indices could be out of range. Probably the only suitable thing
-// to do during the read loop is to make a copy of the data, and operate on it
-// only after the read was found to be consistent.
-class CONTENT_EXPORT GamepadSeqLock {
+// However, this implementation is an improved lock-free variant.
+// The SeqLock can hold only POD fully-embed data (no pointers
+// to satellite data), copies are done with memcpy.
+template<typename T>
+class GamepadSeqLock : public internal::GamepadSeqLockBase {
  public:
-  GamepadSeqLock();
-  base::subtle::Atomic32 ReadBegin();
-  bool ReadRetry(base::subtle::Atomic32 version);
-  void WriteBegin();
-  void WriteEnd();
+  GamepadSeqLock()
+    : internal::GamepadSeqLockBase(entries_, kSize) {

[darin (slow to review), 2012/06/19 21:14:35]

nit: indent by 4 spaces

+  }
 
- private:
-  base::subtle::Atomic32 sequence_;
+  // May be called concurrently with ReadTo and Write.
+  // - If ReadTo occurs in the middle of Write (on another thread),
+  //   ReadTo gets the old data (not the new data being written
+  //   by the other thread).
+  // - ReadTo never spins unless the writer thread is calling Write
+  //   multiple times before ReadTo completes.
+  void ReadTo(T* obj) {
+    // Breaks strict-aliasing rules.
+    Base::ReadTo(reinterpret_cast<Atomic32*>(obj));
+  }
+
+  // Must be called by a single thread at a time.  May be called concurrently
+  // with ReadTo.
+  void Write(const T& obj) {
+    // Breaks strict-aliasing rules.
+    Base::Write(reinterpret_cast<const Atomic32*>(&obj));
+  }
+
+private:
+  typedef internal::GamepadSeqLockBase Base;
+  COMPILE_ASSERT(sizeof(T) % sizeof(Atomic32) == 0, bad_object_size);
+  static size_t const kSize = sizeof(T) / sizeof(Atomic32);
+  struct Entry : Base::BaseEntry{

[darin (slow to review), 2012/06/19 21:14:35]

nit: insert space before "{"

+    Atomic32 data_[kSize];
+  };
+  Entry entries_[kEntries];
   DISALLOW_COPY_AND_ASSIGN(GamepadSeqLock);
 };