Remove unused locking functionality from base::SharedMemory.

base/memory/shared_memory_unittest.cc

 // 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.
 
+#include "base/atomicops.h"
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/shared_memory.h"
 #include "base/process/kill.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/sys_info.h"
 #include "base/test/multiprocess_test.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
@@ skipping 68 matching lines @@
   int16 id_;
 
   static const char* const s_test_name_;
 
   DISALLOW_COPY_AND_ASSIGN(MultipleThreadMain);
 };
 
 const char* const MultipleThreadMain::s_test_name_ =
     "SharedMemoryOpenThreadTest";
 
-// TODO(port):
-// This test requires the ability to pass file descriptors between processes.
-// We haven't done that yet in Chrome for POSIX.
-#if defined(OS_WIN)
-// Each thread will open the shared memory.  Each thread will take the memory,
-// and keep changing it while trying to lock it, with some small pauses in
-// between. Verify that each thread's value in the shared memory is always
-// correct.
-class MultipleLockThread : public PlatformThread::Delegate {
- public:
-  explicit MultipleLockThread(int id) : id_(id) {}
-  ~MultipleLockThread() override {}
-
-  // PlatformThread::Delegate interface.
-  void ThreadMain() override {
-    const uint32 kDataSize = sizeof(int);
-    SharedMemoryHandle handle = NULL;
-    {
-      SharedMemory memory1;
-      EXPECT_TRUE(memory1.CreateNamedDeprecated(
-          "SharedMemoryMultipleLockThreadTest", true, kDataSize));
-      EXPECT_TRUE(memory1.ShareToProcess(GetCurrentProcess(), &handle));
-      // TODO(paulg): Implement this once we have a posix version of
-      // SharedMemory::ShareToProcess.
-      EXPECT_TRUE(true);
-    }
-
-    SharedMemory memory2(handle, false);
-    EXPECT_TRUE(memory2.Map(kDataSize));
-    volatile int* const ptr = static_cast<int*>(memory2.memory());
-
-    for (int idx = 0; idx < 20; idx++) {
-      memory2.LockDeprecated();
-      int i = (id_ << 16) + idx;
-      *ptr = i;
-      PlatformThread::Sleep(TimeDelta::FromMilliseconds(1));
-      EXPECT_EQ(*ptr, i);
-      memory2.UnlockDeprecated();
-    }
-
-    memory2.Close();
-  }
-
- private:
-  int id_;
-
-  DISALLOW_COPY_AND_ASSIGN(MultipleLockThread);
-};
-#endif
-
 }  // namespace
 
 // Android doesn't support SharedMemory::Open/Delete/
 // CreateNamedDeprecated(openExisting=true)
 #if !defined(OS_ANDROID)
 TEST(SharedMemoryTest, OpenClose) {
   const uint32 kDataSize = 1024;
   std::string test_name = "SharedMemoryOpenCloseTest";
 
   // Open two handles to a memory segment, confirm that they are mapped
@@ skipping 160 matching lines @@
 
     // Wait for the threads to finish.
     for (int index = 0; index < numthreads; index++) {
       PlatformThread::Join(thread_handles[index]);
       delete thread_delegates[index];
     }
   }
   MultipleThreadMain::CleanUp();
 }
 
-// TODO(port): this test requires the MultipleLockThread class
-// (defined above), which requires the ability to pass file
-// descriptors between processes.  We haven't done that yet in Chrome
-// for POSIX.
-#if defined(OS_WIN)
-// Create a set of threads to each open a shared memory segment and write to it
-// with the lock held. Verify that they are always reading/writing consistent
-// data.
-TEST(SharedMemoryTest, Lock) {
-  PlatformThreadHandle thread_handles[kNumThreads];
-  MultipleLockThread* thread_delegates[kNumThreads];
-
-  // Spawn the threads.
-  for (int index = 0; index < kNumThreads; ++index) {
-    PlatformThreadHandle pth;
-    thread_delegates[index] = new MultipleLockThread(index);
-    EXPECT_TRUE(PlatformThread::Create(0, thread_delegates[index], &pth));
-    thread_handles[index] = pth;
-  }
-
-  // Wait for the threads to finish.
-  for (int index = 0; index < kNumThreads; ++index) {
-    PlatformThread::Join(thread_handles[index]);
-    delete thread_delegates[index];
-  }
-}
-#endif
-
 // Allocate private (unique) shared memory with an empty string for a
 // name.  Make sure several of them don't point to the same thing as
 // we might expect if the names are equal.
 TEST(SharedMemoryTest, AnonymousPrivate) {
   int i, j;
   int count = 4;
   bool rv;
   const uint32 kDataSize = 8192;
 
   scoped_ptr<SharedMemory[]> memories(new SharedMemory[count]);
@@ skipping 296 matching lines @@
   static void CleanUp() {
     SharedMemory memory;
     memory.Delete(s_test_name_);
   }
 
   static int TaskTestMain() {
     int errors = 0;
 #if defined(OS_MACOSX)
     mac::ScopedNSAutoreleasePool pool;
 #endif
-    const uint32 kDataSize = 1024;
     SharedMemory memory;
-    bool rv = memory.CreateNamedDeprecated(s_test_name_, true, kDataSize);
+    bool rv = memory.CreateNamedDeprecated(s_test_name_, true, data_size_);
     EXPECT_TRUE(rv);
     if (rv != true)
       errors++;
-    rv = memory.Map(kDataSize);
+    rv = memory.Map(data_size_);
     EXPECT_TRUE(rv);
     if (rv != true)
       errors++;
     int *ptr = static_cast<int*>(memory.memory());
 
-    for (int idx = 0; idx < 20; idx++) {
-      memory.LockDeprecated();
-      int i = (1 << 16) + idx;
-      *ptr = i;
-      PlatformThread::Sleep(TimeDelta::FromMilliseconds(10));
-      if (*ptr != i)
-        errors++;
-      memory.UnlockDeprecated();
+    // Atomically increment the value at *ptr by 1.
+    while (true) {
+      int j = *ptr;
+      int old_value = base::subtle::NoBarrier_CompareAndSwap(ptr, j, j + 1);

[Nico, 2015/06/11 15:39:18]

can you use a lock instead?

[erikchen, 2015/06/11 17:36:30]

This code needs to be atomic across multiple processes. The old implementation
grabbed a file lock, which for this test would be both slower and less generic
(since I'm changing shared memory to not necessarily be backed by a file).

[Nico, 2015/06/11 17:46:40]

Ah, right. Maybe say "// This runs concurrently in multiple processes, writes
need to be atomic." at the top of the loop.

Since this is doing an increment, how about using NoBarrier_AtomicIncrement()?

[erikchen, 2015/06/11 17:55:18]

Done.

That's weird. I thought I did use an atomic increment. I must have discovered
atomicops.h, started reading through that, come across CAS first, and said, "o
hey that will work", forgetting all about FAA.

+      if (old_value == j)
+        break;
     }
-
     memory.Close();
     return errors;
   }
 
- private:
   static const char* const s_test_name_;
+  static const uint32 data_size_;

[Nico, 2015/06/11 15:39:18]

s_data_size for consistency

[erikchen, 2015/06/11 17:36:30]

Done.

 };
 
 const char* const SharedMemoryProcessTest::s_test_name_ = "MPMem";
+const uint32 SharedMemoryProcessTest::data_size_ = 1024;
 
-TEST_F(SharedMemoryProcessTest, Tasks) {
+TEST_F(SharedMemoryProcessTest, SharedMemoryAcrossProcesses) {
   SharedMemoryProcessTest::CleanUp();
 
+  // Create a shared memory region. Set the first word to 0.
+  SharedMemory memory;
+  bool rv = memory.CreateNamedDeprecated(s_test_name_, true, data_size_);
+  ASSERT_TRUE(rv);
+  rv = memory.Map(data_size_);
+  ASSERT_TRUE(rv);
+  int* ptr = static_cast<int*>(memory.memory());
+  *ptr = 0;
+
+  // Start |kNumTasks| processes, each of which atomically increments the first
+  // word by 1.
   Process processes[kNumTasks];
   for (int index = 0; index < kNumTasks; ++index) {
     processes[index] = SpawnChild("SharedMemoryTestMain");
     ASSERT_TRUE(processes[index].IsValid());
   }
 
+  // Check that each process exited correctly.
   int exit_code = 0;
   for (int index = 0; index < kNumTasks; ++index) {
     EXPECT_TRUE(processes[index].WaitForExit(&exit_code));
     EXPECT_EQ(0, exit_code);
   }
 
+  // Check that the shared memory region reflects |kNumTasks| increments.
+  ASSERT_EQ(kNumTasks, *ptr);
+
+  memory.Close();
   SharedMemoryProcessTest::CleanUp();
 }
 
 MULTIPROCESS_TEST_MAIN(SharedMemoryTestMain) {
   return SharedMemoryProcessTest::TaskTestMain();
 }
 
 #endif  // !OS_IOS
 
 }  // namespace base