[PartitionAlloc] Use base::Lock in PartitionAlloc, remove SpinLock.

subtle::SpinLock is a spinlock, which has clear shortcomings. Replace it
with base::Lock for the partition lock in PartitionAlloc. As this was
the last user of subtle::SpinLock, remove it as the same time.

Note to perf sheriffs: If there is a regression in range, this is likely
the cause, as the fast path is likely still slower in base::Lock vs
base::subtle::SpinLock.

Bug: 1061437
Change-Id: Idbc4968cfd2fa50bdb1a2468657f4a3da6e29aa4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2390066Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804721}

base/BUILD.gn

@@ -1782,8 +1782,6 @@ component("base") {
         "allocator/partition_allocator/partition_tag_bitmap.h",
         "allocator/partition_allocator/random.cc",
         "allocator/partition_allocator/random.h",
-        "allocator/partition_allocator/spin_lock.cc",
-        "allocator/partition_allocator/spin_lock.h",
       ]
       if (is_win) {
         sources +=
@@ -2437,10 +2435,7 @@ test("base_perftests") {
   ]
   if (!is_ios) {
     # iOS doesn't use the partition allocator, therefore it can't run this test.
-    sources += [
-      "allocator/partition_allocator/partition_alloc_perftest.cc",
-      "allocator/partition_allocator/spin_lock_perftest.cc",
-    ]
+    sources += [ "allocator/partition_allocator/partition_alloc_perftest.cc" ]
   }
   deps = [
     ":base",
@@ -3208,7 +3203,6 @@ test("base_unittests") {
       "allocator/partition_allocator/memory_reclaimer_unittest.cc",
       "allocator/partition_allocator/page_allocator_unittest.cc",
       "allocator/partition_allocator/partition_alloc_unittest.cc",
-      "allocator/partition_allocator/spin_lock_unittest.cc",
     ]
   }
 

base/allocator/partition_allocator/address_space_randomization.cc

@@ -7,7 +7,6 @@
 #include "base/allocator/partition_allocator/page_allocator.h"
 #include "base/allocator/partition_allocator/partition_alloc_check.h"
 #include "base/allocator/partition_allocator/random.h"
-#include "base/allocator/partition_allocator/spin_lock.h"
 #include "base/check_op.h"
 #include "build/build_config.h"
 

base/allocator/partition_allocator/partition_alloc.cc

@@ -17,7 +17,6 @@
 #include "base/allocator/partition_allocator/partition_direct_map_extent.h"
 #include "base/allocator/partition_allocator/partition_oom.h"
 #include "base/allocator/partition_allocator/partition_page.h"
-#include "base/allocator/partition_allocator/spin_lock.h"
 #include "base/check_op.h"
 #include "base/no_destructor.h"
 #include "base/synchronization/lock.h"

base/allocator/partition_allocator/partition_alloc.h

@@ -71,6 +71,7 @@
 #include "base/bits.h"
 #include "base/check_op.h"
 #include "base/compiler_specific.h"
+#include "base/gtest_prod_util.h"
 #include "base/notreached.h"
 #include "base/partition_alloc_buildflags.h"
 #include "base/stl_util.h"

base/allocator/partition_allocator/partition_lock.h

@@ -7,7 +7,6 @@
 
 #include <atomic>
 
-#include "base/allocator/partition_allocator/spin_lock.h"
 #include "base/no_destructor.h"
 #include "base/partition_alloc_buildflags.h"
 #include "base/synchronization/lock.h"
@@ -39,13 +38,12 @@ class SCOPED_LOCKABLE ScopedGuard {
   MaybeSpinLock<thread_safe>& lock_;
 };
 
-#if DCHECK_IS_ON()
 template <>
 class LOCKABLE MaybeSpinLock<true> {
  public:
   MaybeSpinLock() : lock_() {}
   void Lock() EXCLUSIVE_LOCK_FUNCTION() {
-#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
+#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && DCHECK_IS_ON()
     // When PartitionAlloc is malloc(), it can easily become reentrant. For
     // instance, a DCHECK() triggers in external code (such as
     // base::Lock). DCHECK() error message formatting allocates, which triggers
@@ -82,7 +80,7 @@ class LOCKABLE MaybeSpinLock<true> {
   }
 
   void Unlock() UNLOCK_FUNCTION() {
-#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
+#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && DCHECK_IS_ON()
     owning_thread_ref_.store(PlatformThreadRef(), std::memory_order_relaxed);
 #endif
     lock_->Release();
@@ -101,26 +99,11 @@ class LOCKABLE MaybeSpinLock<true> {
   // library, and the destructor is a no-op.
   base::NoDestructor<base::Lock> lock_;
 
-#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
+#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && DCHECK_IS_ON()
   std::atomic<PlatformThreadRef> owning_thread_ref_ GUARDED_BY(lock_);
 #endif
 };
 
-#else   // DCHECK_IS_ON()
-template <>
-class LOCKABLE MaybeSpinLock<true> {
- public:
-  void Lock() EXCLUSIVE_LOCK_FUNCTION() { lock_.lock(); }
-  void Unlock() UNLOCK_FUNCTION() { lock_.unlock(); }
-  void AssertAcquired() const ASSERT_EXCLUSIVE_LOCK() {
-    // Not supported by subtle::SpinLock.
-  }
-
- private:
-  subtle::SpinLock lock_;
-};
-#endif  // DCHECK_IS_ON()
-
 template <>
 class LOCKABLE MaybeSpinLock<false> {
  public:

base/allocator/partition_allocator/random.cc

@@ -4,7 +4,6 @@
 
 #include "base/allocator/partition_allocator/random.h"
 
-#include "base/allocator/partition_allocator/spin_lock.h"
 #include "base/no_destructor.h"
 #include "base/rand_util.h"
 #include "base/synchronization/lock.h"

base/allocator/partition_allocator/spin_lock.cc

-// Copyright 2015 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/allocator/partition_allocator/spin_lock.h"
-
-#include "base/threading/platform_thread.h"
-#include "build/build_config.h"
-
-#if defined(OS_WIN)
-#include <windows.h>
-#elif defined(OS_POSIX) || defined(OS_FUCHSIA)
-#include <sched.h>
-#endif
-
-// The YIELD_PROCESSOR macro wraps an architecture specific-instruction that
-// informs the processor we're in a busy wait, so it can handle the branch more
-// intelligently and e.g. reduce power to our core or give more resources to the
-// other hyper-thread on this core. See the following for context:
-// https://software.intel.com/en-us/articles/benefitting-power-and-performance-sleep-loops
-//
-// The YIELD_THREAD macro tells the OS to relinquish our quantum. This is
-// basically a worst-case fallback, and if you're hitting it with any frequency
-// you really should be using a proper lock (such as |base::Lock|)rather than
-// these spinlocks.
-#if defined(OS_WIN)
-
-#define YIELD_PROCESSOR YieldProcessor()
-#define YIELD_THREAD SwitchToThread()
-
-#elif defined(OS_POSIX) || defined(OS_FUCHSIA)
-
-#if defined(ARCH_CPU_X86_64) || defined(ARCH_CPU_X86)
-#define YIELD_PROCESSOR __asm__ __volatile__("pause")
-#elif (defined(ARCH_CPU_ARMEL) && __ARM_ARCH >= 6) || defined(ARCH_CPU_ARM64)
-#define YIELD_PROCESSOR __asm__ __volatile__("yield")
-#elif defined(ARCH_CPU_MIPSEL)
-// The MIPS32 docs state that the PAUSE instruction is a no-op on older
-// architectures (first added in MIPS32r2). To avoid assembler errors when
-// targeting pre-r2, we must encode the instruction manually.
-#define YIELD_PROCESSOR __asm__ __volatile__(".word 0x00000140")
-#elif defined(ARCH_CPU_MIPS64EL) && __mips_isa_rev >= 2
-// Don't bother doing using .word here since r2 is the lowest supported mips64
-// that Chromium supports.
-#define YIELD_PROCESSOR __asm__ __volatile__("pause")
-#elif defined(ARCH_CPU_PPC64_FAMILY)
-#define YIELD_PROCESSOR __asm__ __volatile__("or 31,31,31")
-#elif defined(ARCH_CPU_S390_FAMILY)
-// just do nothing
-#define YIELD_PROCESSOR ((void)0)
-#endif  // ARCH
-
-#ifndef YIELD_PROCESSOR
-#warning "Processor yield not supported on this architecture."
-#define YIELD_PROCESSOR ((void)0)
-#endif
-
-#define YIELD_THREAD sched_yield()
-
-#else  // Other OS
-
-#warning "Thread yield not supported on this OS."
-#define YIELD_THREAD ((void)0)
-
-#endif  // OS_WIN
-
-namespace base {
-namespace subtle {
-
-void SpinLock::LockSlow() {
-  // The value of |kYieldProcessorTries| is cargo culted from TCMalloc, Windows
-  // critical section defaults, and various other recommendations.
-  // TODO(jschuh): Further tuning may be warranted.
-  static const int kYieldProcessorTries = 1000;
-  // The value of |kYieldThreadTries| is completely made up.
-  static const int kYieldThreadTries = 10;
-  int yield_thread_count = 0;
-  do {
-    do {
-      for (int count = 0; count < kYieldProcessorTries; ++count) {
-        // Let the processor know we're spinning.
-        YIELD_PROCESSOR;
-        if (!lock_.load(std::memory_order_relaxed) &&
-            LIKELY(!lock_.exchange(true, std::memory_order_acquire)))
-          return;
-      }
-
-      if (yield_thread_count < kYieldThreadTries) {
-        ++yield_thread_count;
-        // Give the OS a chance to schedule something on this core.
-        YIELD_THREAD;
-      } else {
-        // At this point, it's likely that the lock is held by a lower priority
-        // thread that is unavailable to finish its work because of higher
-        // priority threads spinning here. Sleeping should ensure that they make
-        // progress.
-        PlatformThread::Sleep(TimeDelta::FromMilliseconds(1));
-      }
-    } while (lock_.load(std::memory_order_relaxed));
-  } while (UNLIKELY(lock_.exchange(true, std::memory_order_acquire)));
-}
-
-}  // namespace subtle
-}  // namespace base

base/allocator/partition_allocator/spin_lock.h

-// Copyright (c) 2013 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 BASE_ALLOCATOR_PARTITION_ALLOCATOR_SPIN_LOCK_H_
-#define BASE_ALLOCATOR_PARTITION_ALLOCATOR_SPIN_LOCK_H_
-
-#include <atomic>
-#include <memory>
-#include <mutex>
-
-#include "base/base_export.h"
-#include "base/compiler_specific.h"
-#include "base/gtest_prod_util.h"
-
-// DEPRECATED. THIS CLASS SHOULD NOT BE USED. It will be removed as part of
-// http://crbug.com/1061437.
-//
-// Spinlock is a simple spinlock class based on the standard CPU primitive of
-// atomic increment and decrement of an int at a given memory address. These are
-// intended only for very short duration locks and assume a system with multiple
-// cores. For any potentially longer wait you should use a real lock, such as
-// |base::Lock|.
-namespace base {
-
-namespace internal {
-template <bool thread_safe>
-class MaybeSpinLock;
-}
-
-namespace subtle {
-
-class BASE_EXPORT SpinLock {
- public:
-  ~SpinLock() = default;
-  using Guard = std::lock_guard<SpinLock>;
-
-  ALWAYS_INLINE void lock() {
-    static_assert(sizeof(lock_) == sizeof(int),
-                  "int and lock_ are different sizes");
-    if (LIKELY(!lock_.exchange(true, std::memory_order_acquire)))
-      return;
-    LockSlow();
-  }
-
-  ALWAYS_INLINE void unlock() { lock_.store(false, std::memory_order_release); }
-
- private:
-  template <bool thread_safe>
-  friend class base::internal::MaybeSpinLock;
-
-  FRIEND_TEST_ALL_PREFIXES(SpinLockTest, Torture);
-  FRIEND_TEST_ALL_PREFIXES(SpinLockPerfTest, Simple);
-  FRIEND_TEST_ALL_PREFIXES(SpinLockPerfTest, WithCompetingThread);
-
-  constexpr SpinLock() = default;
-
-  // This is called if the initial attempt to acquire the lock fails. It's
-  // slower, but has a much better scheduling and power consumption behavior.
-  void LockSlow();
-
-  std::atomic_int lock_{0};
-};
-
-}  // namespace subtle
-}  // namespace base
-
-#endif  // BASE_ALLOCATOR_PARTITION_ALLOCATOR_SPIN_LOCK_H_

base/allocator/partition_allocator/spin_lock_perftest.cc

-// Copyright 2019 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/allocator/partition_allocator/spin_lock.h"
-#include "base/threading/platform_thread.h"
-#include "base/time/time.h"
-#include "base/timer/lap_timer.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "testing/perf/perf_result_reporter.h"
-
-namespace base {
-namespace subtle {
-namespace {
-
-constexpr int kWarmupRuns = 1;
-constexpr TimeDelta kTimeLimit = TimeDelta::FromSeconds(1);
-constexpr int kTimeCheckInterval = 100000;
-
-constexpr char kMetricPrefixSpinLock[] = "SpinLock.";
-constexpr char kMetricLockUnlockThroughput[] = "lock_unlock_throughput";
-constexpr char kStoryBaseline[] = "baseline_story";
-constexpr char kStoryWithCompetingThread[] = "with_competing_thread";
-
-perf_test::PerfResultReporter SetUpReporter(const std::string& story_name) {
-  perf_test::PerfResultReporter reporter(kMetricPrefixSpinLock, story_name);
-  reporter.RegisterImportantMetric(kMetricLockUnlockThroughput, "runs/s");
-  return reporter;
-}
-
-class Spin : public PlatformThread::Delegate {
- public:
-  Spin(SpinLock* lock, size_t* data)
-      : lock_(lock), data_(data), should_stop_(false) {}
-  ~Spin() override = default;
-
-  void ThreadMain() override {
-    while (!should_stop_.load(std::memory_order_relaxed)) {
-      lock_->lock();
-      (*data_)++;
-      lock_->unlock();
-    }
-  }
-
-  void Stop() { should_stop_ = true; }
-
- private:
-  SpinLock* lock_;
-  size_t* data_;
-  std::atomic<bool> should_stop_;
-};
-
-}  // namespace
-
-TEST(SpinLockPerfTest, Simple) {
-  LapTimer timer(kWarmupRuns, kTimeLimit, kTimeCheckInterval);
-  size_t data = 0;
-
-  SpinLock lock;
-
-  do {
-    lock.lock();
-    data += 1;
-    lock.unlock();
-    timer.NextLap();
-  } while (!timer.HasTimeLimitExpired());
-
-  auto reporter = SetUpReporter(kStoryBaseline);
-  reporter.AddResult(kMetricLockUnlockThroughput, timer.LapsPerSecond());
-}
-
-TEST(SpinLockPerfTest, WithCompetingThread) {
-  LapTimer timer(kWarmupRuns, kTimeLimit, kTimeCheckInterval);
-  size_t data = 0;
-
-  SpinLock lock;
-
-  // Starts a competing thread executing the same loop as this thread.
-  Spin thread_main(&lock, &data);
-  PlatformThreadHandle thread_handle;
-  ASSERT_TRUE(PlatformThread::Create(0, &thread_main, &thread_handle));
-
-  do {
-    lock.lock();
-    data += 1;
-    lock.unlock();
-    timer.NextLap();
-  } while (!timer.HasTimeLimitExpired());
-
-  thread_main.Stop();
-  PlatformThread::Join(thread_handle);
-
-  auto reporter = SetUpReporter(kStoryWithCompetingThread);
-  reporter.AddResult(kMetricLockUnlockThroughput, timer.LapsPerSecond());
-}
-}  // namespace subtle
-}  // namespace base

base/allocator/partition_allocator/spin_lock_unittest.cc

-// Copyright 2017 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/allocator/partition_allocator/spin_lock.h"
-
-#include <memory>
-
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/threading/thread.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace base {
-namespace subtle {
-
-static constexpr size_t kBufferSize = 16;
-
-static void FillBuffer(volatile char* buffer, char fill_pattern) {
-  for (size_t i = 0; i < kBufferSize; ++i)
-    buffer[i] = fill_pattern;
-}
-
-static void ChangeAndCheckBuffer(volatile char* buffer) {
-  FillBuffer(buffer, '\0');
-  int total = 0;
-  for (size_t i = 0; i < kBufferSize; ++i)
-    total += buffer[i];
-
-  EXPECT_EQ(0, total);
-
-  // This will mess with the other thread's calculation if we accidentally get
-  // concurrency.
-  FillBuffer(buffer, '!');
-}
-
-static void ThreadMain(SpinLock* lock, volatile char* buffer) {
-  for (int i = 0; i < 500000; ++i) {
-    SpinLock::Guard guard(*lock);
-    ChangeAndCheckBuffer(buffer);
-  }
-}
-
-TEST(SpinLockTest, Torture) {
-  SpinLock lock;
-  char shared_buffer[kBufferSize];
-
-  Thread thread1("thread1");
-  Thread thread2("thread2");
-
-  thread1.StartAndWaitForTesting();
-  thread2.StartAndWaitForTesting();
-
-  thread1.task_runner()->PostTask(
-      FROM_HERE, BindOnce(&ThreadMain, Unretained(&lock),
-                          Unretained(static_cast<char*>(shared_buffer))));
-  thread2.task_runner()->PostTask(
-      FROM_HERE, BindOnce(&ThreadMain, Unretained(&lock),
-                          Unretained(static_cast<char*>(shared_buffer))));
-}
-}  // namespace subtle
-}  // namespace base