Improve GamepadSeqLock impl

content/browser/gamepad/gamepad_provider_unittest.cc

 // Copyright (c) 2011 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 "base/memory/scoped_ptr.h"
 #include "base/process_util.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/system_monitor/system_monitor.h"
+#include "base/threading/platform_thread.h"
 #include "content/browser/gamepad/gamepad_provider.h"
 #include "content/common/gamepad_messages.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
 namespace {
 
 using WebKit::WebGamepads;
 
 class MockDataFetcher : public GamepadDataFetcher {
  public:
-  MockDataFetcher(WebGamepads& test_data) : read_data_(false, false) {
+  explicit MockDataFetcher(WebGamepads const& test_data) {
     test_data_ = test_data;
   }
   virtual void GetGamepadData(WebGamepads* pads,
                               bool devices_changed_hint) OVERRIDE {
     *pads = test_data_;
-    read_data_.Signal();
   }
 
-  void WaitForDataRead() { return read_data_.Wait(); }
-
   WebGamepads test_data_;
-  base::WaitableEvent read_data_;
 };
 
 // Main test fixture
 class GamepadProviderTest : public testing::Test {
  public:
   GamepadProvider* CreateProvider(WebGamepads& test_data) {
 #if defined(OS_MACOSX)
     base::SystemMonitor::AllocateSystemIOPorts();
 #endif
     system_monitor_.reset(new base::SystemMonitor);
@@ skipping 28 matching lines @@
   test_data.items[0].timestamp = 0;
   test_data.items[0].buttonsLength = 1;
   test_data.items[0].axesLength = 2;
   test_data.items[0].buttons[0] = 1.f;
   test_data.items[0].axes[0] = -1.f;
   test_data.items[0].axes[1] = .5f;
 
   GamepadProvider* provider = CreateProvider(test_data);
   provider->Start();
 
   main_message_loop_.RunAllPending();

[scottmg, 2011/12/01 22:36:39]

The intention was to test the collision between reading and writing, which is
why it blocked here to ensure the test data had been read.

 
-  mock_data_fetcher_->WaitForDataRead();
-
   // Renderer-side, pull data out of poll buffer.
   base::SharedMemoryHandle handle =
       provider->GetRendererSharedMemoryHandle(base::GetCurrentProcessHandle());
   scoped_ptr<base::SharedMemory> shared_memory(
       new base::SharedMemory(handle, true));
   EXPECT_TRUE(shared_memory->Map(sizeof(GamepadHardwareBuffer)));
   void* mem = shared_memory->memory();
 
   GamepadHardwareBuffer* hwbuf = static_cast<GamepadHardwareBuffer*>(mem);
-  // See gamepad_hardware_buffer.h for details on the read discipline.
   WebGamepads output;
-
-  base::subtle::Atomic32 version;
-  do {
-    version = hwbuf->sequence.ReadBegin();
-    memcpy(&output, &hwbuf->buffer, sizeof(output));
-  } while (hwbuf->sequence.ReadRetry(version));
-
+  output.length = 0;

[scottmg, 2011/12/01 22:36:39]

this loop seems unnecessary with the ReadTo api.

[dvyukov, 2011/12/03 10:12:14]

The situation is more involved that it appears on first sight.
Currently, if the writer is in GetGamepadData(), it means that it started
writing into SeqLock but not yet finished. If the readers start reading at this
point, it blocks on SeqLock's internal spinlock waiting for the write to
complete. When the write completes, the reader reads the new version.
With this change, if the writer is in GetGamepadData(), it is not yet started
writing into SeqLock, so at this point the reader will just read the old version
(all zeroes). Even if we somehow detect when the writer is in process of writing
into SeqLock, and start reading at that point, the reader will still read the
old version (the write is not yet completed).

So I am not sure what to do with this test. We can (1) remove the loop and
verify that the reader get either consistent initial version or consistent new
version (however in tests we will test only 1 option most of the time), or (2)
block the reader till the writer finishes write (return from
GamepadSeqLock::Write()) and verify that the reader observes the new version, or
(3) leave it as-is - currently it verifies that the new consistent data
eventually becomes available to readers.

+  for (;;) {
+    hwbuf->gamepads.ReadTo(&output);
+    if (output.length == 1)
+      break;
+    base::PlatformThread::YieldCurrentThread();
+  }
   EXPECT_EQ(1u, output.length);
   EXPECT_EQ(1u, output.items[0].buttonsLength);
   EXPECT_EQ(1.f, output.items[0].buttons[0]);
   EXPECT_EQ(2u, output.items[0].axesLength);
   EXPECT_EQ(-1.f, output.items[0].axes[0]);
   EXPECT_EQ(0.5f, output.items[0].axes[1]);
 
   provider->Stop();
 }
 
 }  // namespace
 
 } // namespace content