[PartitionAlloc] Bring back SpinLock as Partition lock.

base::Lock still regresses multiple benchmarks (see crbug.com/1125866)
for a summary of regressions. Bring back the SpinLock for non-DCHECK()
builds.

This is a partial revert of
https://chromium-review.googlesource.com/c/chromium/src/+/2390066, with
a few changes:
- base::subtle::SpinLock -> base::internal::SpinLock
- lock()/unlock() -> Acquire()/Release(), to match the usual naming.
- Remove unused methods

Another motivation for a partial revert (with inlining of SpinLock in
partition_lock.h) is to prepare further work to make this lock sleep, if
base::Lock's fast path doesn't improve enough.

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

base/BUILD.gn

@@ -1767,6 +1767,7 @@ component("base") {
         "allocator/partition_allocator/partition_cookie.h",
         "allocator/partition_allocator/partition_direct_map_extent.h",
         "allocator/partition_allocator/partition_freelist_entry.h",
+        "allocator/partition_allocator/partition_lock.cc",
         "allocator/partition_allocator/partition_lock.h",
         "allocator/partition_allocator/partition_oom.cc",
         "allocator/partition_allocator/partition_oom.h",

base/allocator/partition_allocator/partition_lock.cc

@@ -0,0 +1,106 @@
+// Copyright 2020 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/partition_lock.h"
+
+#if !DCHECK_IS_ON()
+
+#include "base/threading/platform_thread.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 internal {
+
+void SpinLock::AcquireSlow() {
+  // The value of |kYieldProcessorTries| is cargo culted from TCMalloc, Windows
+  // critical section defaults, and various other recommendations.
+  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 internal
+}  // namespace base
+
+#endif  // !DCHECK_IS_ON()

base/allocator/partition_allocator/partition_lock.h

@@ -6,9 +6,10 @@
 #define BASE_ALLOCATOR_PARTITION_ALLOCATOR_PARTITION_LOCK_H_
 
 #include <atomic>
+#include <type_traits>
 
+#include "base/allocator/buildflags.h"
 #include "base/no_destructor.h"
-#include "base/partition_alloc_buildflags.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
 #include "build/build_config.h"
@@ -38,6 +39,35 @@ class SCOPED_LOCKABLE ScopedGuard {
   MaybeSpinLock<thread_safe>& lock_;
 };
 
+#if !DCHECK_IS_ON()
+// Spinlock. Do not use, to be removed. crbug.com/1061437.
+class BASE_EXPORT SpinLock {
+ public:
+  SpinLock() = default;
+  ~SpinLock() = default;
+
+  ALWAYS_INLINE void Acquire() {
+    if (LIKELY(!lock_.exchange(true, std::memory_order_acquire)))
+      return;
+    AcquireSlow();
+  }
+
+  ALWAYS_INLINE void Release() {
+    lock_.store(false, std::memory_order_release);
+  }
+
+  // Not supported.
+  void AssertAcquired() const {}
+
+ private:
+  // 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 AcquireSlow();
+
+  std::atomic_int lock_{0};
+};
+#endif  // !DCHECK_IS_ON()
+
 template <>
 class LOCKABLE MaybeSpinLock<true> {
  public:
@@ -90,14 +120,22 @@ class LOCKABLE MaybeSpinLock<true> {
   }
 
  private:
+#if DCHECK_IS_ON()
   // NoDestructor to avoid issues with the "static destruction order fiasco".
   //
-  // This also means that for DCHECK_IS_ON() builds we leak a lock when a
-  // partition is destructed. This will in practice only show in some tests, as
-  // partitions are not destructed in regular use. In addition, on most
-  // platforms, base::Lock doesn't allocate memory and neither does the OS
-  // library, and the destructor is a no-op.
+  // This also means that we leak a lock when a partition is destructed. This
+  // will in practice only show in some tests, as partitions are not destructed
+  // in regular use. In addition, on most platforms, base::Lock doesn't allocate
+  // memory and neither does the OS library, and the destructor is a no-op.
   base::NoDestructor<base::Lock> lock_;
+#else
+  // base::Lock is slower on the fast path than SpinLock, hence we still use it
+  // on non-DCHECK() builds. crbug.com/1125999
+  base::NoDestructor<SpinLock> lock_;
+  // base::NoDestructor is here to use the same code elsewhere, we are not
+  // leaking anything.
+  static_assert(std::is_trivially_destructible<SpinLock>::value, "");
+#endif
 
 #if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && DCHECK_IS_ON()
   std::atomic<PlatformThreadRef> owning_thread_ref_ GUARDED_BY(lock_);