Improve GamepadSeqLock impl

content/common/gamepad_seqlock.h

Index: content/common/gamepad_seqlock.h
===================================================================
--- content/common/gamepad_seqlock.h	(revision 117420)
+++ content/common/gamepad_seqlock.h	(working copy)
@@ -6,38 +6,66 @@
 #define CONTENT_COMMON_GAMEPAD_SEQLOCK_H_
 
 #include "base/atomicops.h"
-#include "base/threading/platform_thread.h"
-#include "content/common/content_export.h"
 
 namespace content {
 
+namespace internal {
+
+class GamepadSeqLockBase {
+ protected:
+  static const size_t kEntries = 2;
+  typedef base::subtle::Atomic32 Atomic32;
+  struct Entry {
+    Atomic32 sequence_;
+    // Both writer and readers work with the array concurrently,
+    // so it must be accessed with atomic operations.
+    Atomic32 data_[];
+  };
+
+  GamepadSeqLockBase(Entry* entries, size_t size);
+  void ReadTo(Atomic32* obj);
+  void Write(const Atomic32* obj);
+
+ private:
+  const size_t size_;
+  Atomic32 current_;
+  Entry* 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()
+    : Base(entries_, kSize) {

[jbates, 2012/02/17 23:29:52]

hrmm, so I see you haven't tried to compile this yet :p

[dvyukov, 2012/02/21 15:18:04]

Compile?.. What for? :)
Done... hope I guessed correctly what error compilation produces on this code...

+  }
 
- private:
-  base::subtle::Atomic32 sequence_;
+  void ReadTo(T* obj) {

[jbates, 2012/02/17 23:29:52]

Would be helpful to see the properties and guarantees of ReadTo in the comments.
For example:
- 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.

[dvyukov, 2012/02/21 15:18:04]

Done.

+    // Breaks strict-aliasing rules.
+    Base::ReadTo(reinterpret_cast<Atomic32*>(obj));
+  }
+
+  void Write(const T& obj) {
+    // Breaks strict-aliasing rules.
+    Base::Write(reinterpret_cast<const Atomic32*>(&obj));
+  }
+
+private:
+  typedef internal::GamepadSeqLockBase Base;
+  typedef char static_assert_size[sizeof(T) % sizeof(Atomic32) ? -1 : 1];

[jbates, 2012/02/17 23:29:52]

could use the macro for this in base/basictypes.h:
COMPILE_ASSERT(sizeof(T) % sizeof(Atomic32) == 0)

[dvyukov, 2012/02/21 15:18:04]

Done.

+  static size_t const kSize = sizeof(T) / sizeof(Atomic32);
+  struct Entry : Base::Entry{

[jbates, 2012/02/17 23:29:52]

I was confused by "Entry" since it is the same name as the derived Entry type. A
different name here would help.

[dvyukov, 2012/02/21 15:18:04]

Done.

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