[heap] Safepointing with an atomic state

To improve performance of parking, keep the thread state in an atomic
variable instead of protecting it with a mutex.

However the mutex was used e.g. to force Unpark() to block while the
safepoint operation was still running. Therefore the safepoint algorithm
has to change as well.

Park() and Unpark() use CAS operation to transition the state.
Safepoint() uses a relaxed load for checking whether a safepoint was
requested. Since Safepoint(), Park() and Unpark() all have a slow path,
there is no need for busy-waiting on the main thread.

We need two more ThreadStates:
* SafepointRequested: This state is set by GlobalSafepoint to force
    Running threads into the slow path on Safepoint() and Park(). This
    state also replaces the separate atomic<bool> safepoint_requested_
    field we used before.
* ParkedSafepoint: This state is set by GlobalSafepoint as well to force
    parked threads into the slow path on Unpark().

When stopping all threads, GlobalSafepoint transitions states from
Running --> SafepointRequested and Parked --> ParkedSafepoint to force
the slow path for all three methods. After performing the transition
for each thread we know the exact number of Running threads and wait
until each of them either reached a safepoint or parked itself.

Design doc: https://docs.google.com/document/d/1p9klWyqT_AScAnK_PdHZTcNhZGzoBiYWPkUciIh2C58/edit?usp=sharing

Bug: chromium:1177144, v8:10315
Change-Id: I8697da915c7d18e2fb941f1bedf6181226408feb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2704075Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73089}

src/heap/local-heap-inl.h

@@ -5,6 +5,8 @@
 #ifndef V8_HEAP_LOCAL_HEAP_INL_H_
 #define V8_HEAP_LOCAL_HEAP_INL_H_
 
+#include <atomic>
+
 #include "src/common/assert-scope.h"
 #include "src/handles/persistent-handles.h"
 #include "src/heap/concurrent-allocator-inl.h"
@@ -24,6 +26,9 @@ AllocationResult LocalHeap::AllocateRaw(int size_in_bytes, AllocationType type,
                  alignment == AllocationAlignment::kWordAligned);
   Heap::HeapState state = heap()->gc_state();
   DCHECK(state == Heap::TEAR_DOWN || state == Heap::NOT_IN_GC);
+  ThreadState current = state_.load(std::memory_order_relaxed);
+  DCHECK(current == ThreadState::Running ||
+         current == ThreadState::SafepointRequested);
 #endif
 
   // Each allocation is supposed to be a safepoint.

src/heap/local-heap.cc

@@ -4,6 +4,7 @@
 
 #include "src/heap/local-heap.h"
 
+#include <atomic>
 #include <memory>
 
 #include "src/base/logging.h"
@@ -11,6 +12,7 @@
 #include "src/common/globals.h"
 #include "src/execution/isolate.h"
 #include "src/handles/local-handles.h"
+#include "src/heap/gc-tracer.h"
 #include "src/heap/heap-inl.h"
 #include "src/heap/heap-write-barrier.h"
 #include "src/heap/local-heap-inl.h"
@@ -43,7 +45,6 @@ LocalHeap::LocalHeap(Heap* heap, ThreadKind kind,
     : heap_(heap),
       is_main_thread_(kind == ThreadKind::kMain),
       state_(ThreadState::Parked),
-      safepoint_requested_(false),
       allocation_failed_(false),
       prev_(nullptr),
       next_(nullptr),
@@ -122,7 +123,9 @@ bool LocalHeap::IsHandleDereferenceAllowed() {
 #ifdef DEBUG
   VerifyCurrent();
 #endif
-  return state_ == ThreadState::Running;
+  ThreadState state = state_relaxed();
+  return state == ThreadState::Running ||
+         state == ThreadState::SafepointRequested;
 }
 #endif
 
@@ -130,40 +133,53 @@ bool LocalHeap::IsParked() {
 #ifdef DEBUG
   VerifyCurrent();
 #endif
-  return state_ == ThreadState::Parked;
+  ThreadState state = state_relaxed();
+  return state == ThreadState::Parked || state == ThreadState::ParkedSafepoint;
 }
 
 void LocalHeap::Park() {
-  base::MutexGuard guard(&state_mutex_);
-  CHECK_EQ(ThreadState::Running, state_);
-  state_ = ThreadState::Parked;
-  state_change_.NotifyAll();
+  ThreadState expected = ThreadState::Running;
+  if (!state_.compare_exchange_strong(expected, ThreadState::Parked)) {
+    CHECK_EQ(expected, ThreadState::SafepointRequested);
+    expected = ThreadState::SafepointRequested;
+    CHECK(
+        state_.compare_exchange_strong(expected, ThreadState::ParkedSafepoint));
+    heap_->safepoint()->NotifyPark();
+  }
 }
 
 void LocalHeap::Unpark() {
-  base::MutexGuard guard(&state_mutex_);
-  CHECK(state_ == ThreadState::Parked);
-  state_ = ThreadState::Running;
+  while (true) {
+    ThreadState expected = ThreadState::Parked;
+    if (!state_.compare_exchange_strong(expected, ThreadState::Running)) {
+      CHECK_EQ(expected, ThreadState::ParkedSafepoint);
+      DCHECK(!is_main_thread());
+      DCHECK_EQ(LocalHeap::Current(), this);
+      TRACE_GC1(heap_->tracer(), GCTracer::Scope::BACKGROUND_UNPARK,
+                ThreadKind::kBackground);
+      heap_->safepoint()->WaitInUnpark();
+    } else {
+      break;
+    }
+  }
 }
 
 void LocalHeap::EnsureParkedBeforeDestruction() {
-  if (IsParked()) return;
-  base::MutexGuard guard(&state_mutex_);
-  state_ = ThreadState::Parked;
-  state_change_.NotifyAll();
-}
-
-void LocalHeap::RequestSafepoint() {
-  safepoint_requested_.store(true, std::memory_order_relaxed);
-}
-
-void LocalHeap::ClearSafepointRequested() {
-  safepoint_requested_.store(false, std::memory_order_relaxed);
+  DCHECK_IMPLIES(!is_main_thread(), IsParked());
 }
 
-void LocalHeap::EnterSafepoint() {
+void LocalHeap::SafepointSlowPath() {
+  DCHECK(!is_main_thread());
   DCHECK_EQ(LocalHeap::Current(), this);
-  if (state_ == ThreadState::Running) heap_->safepoint()->EnterFromThread(this);
+  TRACE_GC1(heap_->tracer(), GCTracer::Scope::BACKGROUND_SAFEPOINT,
+            ThreadKind::kBackground);
+  LocalHeap::ThreadState expected = LocalHeap::ThreadState::SafepointRequested;
+  CHECK(state_.compare_exchange_strong(expected,
+                                       LocalHeap::ThreadState::Safepoint));
+  heap_->safepoint()->WaitInSafepoint();
+  // This might be a bit surprising, GlobalSafepoint transitions the state from
+  // Safepoint (--> Running) --> Parked when returning from the safepoint.
+  Unpark();
 }
 
 void LocalHeap::FreeLinearAllocationArea() {

src/heap/local-heap.h

@@ -40,18 +40,14 @@ class V8_EXPORT_PRIVATE LocalHeap {
       std::unique_ptr<PersistentHandles> persistent_handles = nullptr);
   ~LocalHeap();
 
-  // Invoked by main thread to signal this thread that it needs to halt in a
-  // safepoint.
-  void RequestSafepoint();
-
   // Frequently invoked by local thread to check whether safepoint was requested
   // from the main thread.
   void Safepoint() {
     DCHECK(AllowSafepoints::IsAllowed());
+    ThreadState current = state_relaxed();
 
-    if (IsSafepointRequested()) {
-      ClearSafepointRequested();
-      EnterSafepoint();
+    if (V8_UNLIKELY(current == ThreadState::SafepointRequested)) {
+      SafepointSlowPath();
     }
   }
 
@@ -144,15 +140,27 @@ class V8_EXPORT_PRIVATE LocalHeap {
 
  private:
   enum class ThreadState {
-    // Threads in this state need to be stopped in a safepoint.
+    // Threads in this state are allowed to access the heap.
     Running,
     // Thread was parked, which means that the thread is not allowed to access
-    // or manipulate the heap in any way.
+    // or manipulate the heap in any way. This is considered to be a safepoint.
     Parked,
-    // Thread was stopped in a safepoint.
-    Safepoint
+
+    // All other states are needed for stopping-the-world.
+    // SafepointRequested is used for Running threads to force Safepoint() and
+    // Park() into the slow path.
+    SafepointRequested,
+    // A thread transitions into this state from SafepointRequested when it
+    // enters a safepoint.
+    Safepoint,
+    // This state is used for Parked threads and forces Unpark() into the slow
+    // path. It prevents Unpark() to succeed before the safepoint operation is
+    // finished.
+    ParkedSafepoint,
   };
 
+  ThreadState state_relaxed() { return state_.load(std::memory_order_relaxed); }
+
   // Slow path of allocation that performs GC and then retries allocation in
   // loop.
   Address PerformCollectionAndAllocateAgain(int object_size,
@@ -163,26 +171,16 @@ class V8_EXPORT_PRIVATE LocalHeap {
   void Park();
   void Unpark();
   void EnsureParkedBeforeDestruction();
+  void SafepointSlowPath();
 
   void EnsurePersistentHandles();
 
-  V8_INLINE bool IsSafepointRequested() {
-    return safepoint_requested_.load(std::memory_order_relaxed);
-  }
-  void ClearSafepointRequested();
-
-  void EnterSafepoint();
-
   void InvokeGCEpilogueCallbacksInSafepoint();
 
   Heap* heap_;
   bool is_main_thread_;
 
-  base::Mutex state_mutex_;
-  base::ConditionVariable state_change_;
-  ThreadState state_;
-
-  std::atomic<bool> safepoint_requested_;
+  std::atomic<ThreadState> state_;
 
   bool allocation_failed_;
 

src/heap/safepoint.cc

@@ -4,11 +4,15 @@
 
 #include "src/heap/safepoint.h"
 
+#include <atomic>
+
 #include "src/base/logging.h"
+#include "src/handles/handles.h"
 #include "src/handles/local-handles.h"
 #include "src/handles/persistent-handles.h"
 #include "src/heap/gc-tracer.h"
 #include "src/heap/heap-inl.h"
+#include "src/heap/heap.h"
 #include "src/heap/local-heap.h"
 
 namespace v8 {
@@ -29,26 +33,37 @@ void GlobalSafepoint::EnterSafepointScope() {
   barrier_.Arm();
   DCHECK_NULL(LocalHeap::Current());
 
-  for (LocalHeap* current = local_heaps_head_; current;
-       current = current->next_) {
-    if (current->is_main_thread()) {
+  int running = 0;
+
+  for (LocalHeap* local_heap = local_heaps_head_; local_heap;
+       local_heap = local_heap->next_) {
+    if (local_heap->is_main_thread()) {
       continue;
     }
-    current->RequestSafepoint();
+    DCHECK(!local_heap->is_main_thread());
+
+    LocalHeap::ThreadState expected = local_heap->state_relaxed();
+
+    while (true) {
+      CHECK(expected == LocalHeap::ThreadState::Parked ||
+            expected == LocalHeap::ThreadState::Running);
+      LocalHeap::ThreadState new_state =
+          expected == LocalHeap::ThreadState::Parked
+              ? LocalHeap::ThreadState::ParkedSafepoint
+              : LocalHeap::ThreadState::SafepointRequested;
+
+      if (local_heap->state_.compare_exchange_strong(expected, new_state)) {
+        if (expected == LocalHeap::ThreadState::Running) {
+          running++;
+        } else {
+          CHECK_EQ(expected, LocalHeap::ThreadState::Parked);
         }
-
-  for (LocalHeap* current = local_heaps_head_; current;
-       current = current->next_) {
-    if (current->is_main_thread()) {
-      continue;
+        break;
       }
-    DCHECK(!current->is_main_thread());
-    current->state_mutex_.Lock();
-
-    while (current->state_ == LocalHeap::ThreadState::Running) {
-      current->state_change_.Wait(&current->state_mutex_);
     }
   }
+
+  barrier_.WaitUntilRunningThreadsInSafepoint(running);
 }
 
 void GlobalSafepoint::LeaveSafepointScope() {
@@ -57,12 +72,22 @@ void GlobalSafepoint::LeaveSafepointScope() {
 
   DCHECK_NULL(LocalHeap::Current());
 
-  for (LocalHeap* current = local_heaps_head_; current;
-       current = current->next_) {
-    if (current->is_main_thread()) {
+  for (LocalHeap* local_heap = local_heaps_head_; local_heap;
+       local_heap = local_heap->next_) {
+    if (local_heap->is_main_thread()) {
       continue;
     }
-    current->state_mutex_.Unlock();
+
+    // We transition both ParkedSafepoint and Safepoint states to Parked. While
+    // this is probably intuitive for ParkedSafepoint, this might be surprising
+    // for Safepoint though. SafepointSlowPath() will later unpark that thread
+    // again. Going through Parked means that a background thread doesn't need
+    // to be waked up before the main thread can start the next safepoint.
+
+    LocalHeap::ThreadState old_state =
+        local_heap->state_.exchange(LocalHeap::ThreadState::Parked);
+    CHECK(old_state == LocalHeap::ThreadState::ParkedSafepoint ||
+          old_state == LocalHeap::ThreadState::Safepoint);
   }
 
   barrier_.Disarm();
@@ -70,39 +95,59 @@ void GlobalSafepoint::LeaveSafepointScope() {
   local_heaps_mutex_.Unlock();
 }
 
-void GlobalSafepoint::EnterFromThread(LocalHeap* local_heap) {
-  {
-    base::MutexGuard guard(&local_heap->state_mutex_);
-    DCHECK_EQ(local_heap->state_, LocalHeap::ThreadState::Running);
-    local_heap->state_ = LocalHeap::ThreadState::Safepoint;
-    local_heap->state_change_.NotifyAll();
-  }
+void GlobalSafepoint::WaitInSafepoint() { barrier_.WaitInSafepoint(); }
 
-  barrier_.Wait();
+void GlobalSafepoint::WaitInUnpark() { barrier_.WaitInUnpark(); }
 
-  {
-    base::MutexGuard guard(&local_heap->state_mutex_);
-    local_heap->state_ = LocalHeap::ThreadState::Running;
-  }
-}
+void GlobalSafepoint::NotifyPark() { barrier_.NotifyPark(); }
 
 void GlobalSafepoint::Barrier::Arm() {
   base::MutexGuard guard(&mutex_);
-  CHECK(!armed_);
+  DCHECK(!IsArmed());
   armed_ = true;
+  stopped_ = 0;
 }
 
 void GlobalSafepoint::Barrier::Disarm() {
   base::MutexGuard guard(&mutex_);
-  CHECK(armed_);
+  DCHECK(IsArmed());
   armed_ = false;
-  cond_.NotifyAll();
+  stopped_ = 0;
+  cv_resume_.NotifyAll();
+}
+
+void GlobalSafepoint::Barrier::WaitUntilRunningThreadsInSafepoint(int running) {
+  base::MutexGuard guard(&mutex_);
+  DCHECK(IsArmed());
+  while (stopped_ < running) {
+    cv_stopped_.Wait(&mutex_);
+  }
+  DCHECK_EQ(stopped_, running);
 }
 
-void GlobalSafepoint::Barrier::Wait() {
+void GlobalSafepoint::Barrier::NotifyPark() {
   base::MutexGuard guard(&mutex_);
-  while (armed_) {
-    cond_.Wait(&mutex_);
+  CHECK(IsArmed());
+  stopped_++;
+  cv_stopped_.NotifyOne();
+}
+
+void GlobalSafepoint::Barrier::WaitInSafepoint() {
+  base::MutexGuard guard(&mutex_);
+  CHECK(IsArmed());
+  stopped_++;
+  cv_stopped_.NotifyOne();
+
+  while (IsArmed()) {
+    cv_resume_.Wait(&mutex_);
+  }
+}
+
+void GlobalSafepoint::Barrier::WaitInUnpark() {
+  base::MutexGuard guard(&mutex_);
+
+  while (IsArmed()) {
+    cv_resume_.Wait(&mutex_);
   }
 }
 

src/heap/safepoint.h

@@ -24,8 +24,14 @@ class GlobalSafepoint {
  public:
   explicit GlobalSafepoint(Heap* heap);
 
-  // Enter the safepoint from a thread
-  void EnterFromThread(LocalHeap* local_heap);
+  // Wait until unpark operation is safe again
+  void WaitInUnpark();
+
+  // Enter the safepoint from a running thread
+  void WaitInSafepoint();
+
+  // Running thread reached a safepoint by parking itself.
+  void NotifyPark();
 
   V8_EXPORT_PRIVATE bool ContainsLocalHeap(LocalHeap* local_heap);
   V8_EXPORT_PRIVATE bool ContainsAnyLocalHeap();
@@ -48,15 +54,24 @@ class GlobalSafepoint {
  private:
   class Barrier {
     base::Mutex mutex_;
-    base::ConditionVariable cond_;
+    base::ConditionVariable cv_resume_;
+    base::ConditionVariable cv_stopped_;
     bool armed_;
 
+    int stopped_ = 0;
+
+    bool IsArmed() { return armed_; }
+
    public:
-    Barrier() : armed_(false) {}
+    Barrier() : armed_(false), stopped_(0) {}
 
     void Arm();
     void Disarm();
-    void Wait();
+    void WaitUntilRunningThreadsInSafepoint(int running);
+
+    void WaitInSafepoint();
+    void WaitInUnpark();
+    void NotifyPark();
   };
 
   void EnterSafepointScope();

src/init/heap-symbols.h

@@ -517,6 +517,8 @@
   F(BACKGROUND_FULL_ARRAY_BUFFER_SWEEP)           \
   F(BACKGROUND_COLLECTION)                        \
   F(BACKGROUND_UNMAPPER)                          \
+  F(BACKGROUND_UNPARK)                            \
+  F(BACKGROUND_SAFEPOINT)                         \
   F(MC_BACKGROUND_EVACUATE_COPY)                  \
   F(MC_BACKGROUND_EVACUATE_UPDATE_POINTERS)       \
   F(MC_BACKGROUND_MARKING)                        \

tools/gcmole/gcmole.py

@@ -306,7 +306,7 @@ ALLOWLIST = [
 ]
 
 GC_PATTERN = ",.*Collect.*Garbage"
-SAFEPOINT_PATTERN = ",EnterSafepoint"
+SAFEPOINT_PATTERN = ",SafepointSlowPath"
 ALLOWLIST_PATTERN = "|".join("(?:%s)" % p for p in ALLOWLIST)