Reland "[heap] Replace usages of CollectionRequested with SafepointRequested"

This is a reland of 5ef4e14f

The previous CL caused flaky test failures with some concurrent
allocation tests. The reason for this was that the main thread's state
and collection_requested_ can't be updated in an atomic operation
anymore.

Any thread will now invoke RequestGC() first. Then it will wait in
AwaitCollectionBackground() when the main thread was running. Both
methods can and will be invoked more than once.

The new flag block_for_collection_ is used to decide whether a thread
needs wait for the GC. collection_requested_ can't be used for that
purpose because that flag is also true when the main thread is parked.

Original change's description:
> [heap] Replace usages of CollectionRequested with SafepointRequested
>
> CollectionRequested was used exclusively on the main thread when a
> background thread requested a GC. The main thread never used
> SafepointRequested at any time. Now with the shared GC we might need to
> stop multiple isolates in a safepoint in the future. In such a situation
> we would need to use SafepointRequested also on the main thread.
>
> This CL prepares V8 for this situation by using SafepointRequested
> instead of CollectionRequested and friends on the main thread. The slow
> path of Safepoint(), Park() and Unpark() will check in the future
> whether the main thread needs to halt for a shared GC or needs to
> perform a local GC. At the moment, simply performing the local GC is
> still enough.
>
> Bug: v8:11708
> Change-Id: I819b6f7db8251074a4adf8b554e0a1393c76f7da
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2891834
> Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#74673}

Bug: v8:11708
Change-Id: Ibe245cd1822310123b3af2026872fd9927ee410e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2912576
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74739}

src/heap/collection-barrier.cc

@@ -4,6 +4,7 @@
 
 #include "src/heap/collection-barrier.h"
 
+#include "src/base/platform/mutex.h"
 #include "src/base/platform/time.h"
 #include "src/common/globals.h"
 #include "src/execution/isolate.h"
@@ -17,25 +18,18 @@
 namespace v8 {
 namespace internal {
 
-bool CollectionBarrier::CollectionRequested() {
-  return main_thread_state_relaxed() == LocalHeap::kCollectionRequested;
+bool CollectionBarrier::WasGCRequested() {
+  return collection_requested_.load();
 }
 
-LocalHeap::ThreadState CollectionBarrier::main_thread_state_relaxed() {
-  LocalHeap* main_thread_local_heap = heap_->main_thread_local_heap();
-  return main_thread_local_heap->state_relaxed();
-}
-
-void CollectionBarrier::NotifyShutdownRequested() {
+void CollectionBarrier::RequestGC() {
   base::MutexGuard guard(&mutex_);
-  if (timer_.IsStarted()) timer_.Stop();
-  shutdown_requested_ = true;
-  cv_wakeup_.NotifyAll();
-}
+  bool was_already_requested = collection_requested_.exchange(true);
 
-void CollectionBarrier::ResumeThreadsAwaitingCollection() {
-  base::MutexGuard guard(&mutex_);
-  cv_wakeup_.NotifyAll();
+  if (!was_already_requested) {
+    CHECK(!timer_.IsStarted());
+    timer_.Start();
+  }
 }
 
 class BackgroundCollectionInterruptTask : public CancelableTask {
@@ -56,11 +50,39 @@ class BackgroundCollectionInterruptTask : public CancelableTask {
   Heap* heap_;
 };
 
+void CollectionBarrier::NotifyShutdownRequested() {
+  base::MutexGuard guard(&mutex_);
+  if (timer_.IsStarted()) timer_.Stop();
+  shutdown_requested_ = true;
+  cv_wakeup_.NotifyAll();
+}
+
+void CollectionBarrier::ResumeThreadsAwaitingCollection() {
+  base::MutexGuard guard(&mutex_);
+  collection_requested_.store(false);
+  block_for_collection_ = false;
+  cv_wakeup_.NotifyAll();
+}
+
 bool CollectionBarrier::AwaitCollectionBackground(LocalHeap* local_heap) {
+  bool first_thread;
+
+  {
+    // Update flag before parking this thread, this guarantees that the flag is
+    // set before the next GC.
+    base::MutexGuard guard(&mutex_);
+    first_thread = !block_for_collection_;
+    block_for_collection_ = true;
+    CHECK(timer_.IsStarted());
+  }
+
+  // The first thread needs to activate the stack guard and post the task.
+  if (first_thread) ActivateStackGuardAndPostTask();
+
   ParkedScope scope(local_heap);
   base::MutexGuard guard(&mutex_);
 
-  while (CollectionRequested()) {
+  while (block_for_collection_) {
     if (shutdown_requested_) return false;
     cv_wakeup_.Wait(&mutex_);
   }
@@ -68,16 +90,22 @@ bool CollectionBarrier::AwaitCollectionBackground(LocalHeap* local_heap) {
   return true;
 }
 
-void CollectionBarrier::StopTimeToCollectionTimer() {
-  LocalHeap::ThreadState main_thread_state = main_thread_state_relaxed();
-  CHECK(main_thread_state == LocalHeap::kRunning ||
-        main_thread_state == LocalHeap::kCollectionRequested);
+void CollectionBarrier::ActivateStackGuardAndPostTask() {
+  Isolate* isolate = heap_->isolate();
+  ExecutionAccess access(isolate);
+  isolate->stack_guard()->RequestGC();
 
-  if (main_thread_state == LocalHeap::kCollectionRequested) {
+  V8::GetCurrentPlatform()
+      ->GetForegroundTaskRunner(reinterpret_cast<v8::Isolate*>(isolate))
+      ->PostTask(std::make_unique<BackgroundCollectionInterruptTask>(heap_));
+}
+
+void CollectionBarrier::StopTimeToCollectionTimer() {
+  if (collection_requested_.load()) {
     base::MutexGuard guard(&mutex_);
-    // The first background thread that requests the GC, starts the timer first
-    // and only then parks itself. Since we are in a safepoint here, the timer
-    // is therefore always initialized here already.
+    // The first thread that requests the GC, starts the timer first and *then*
+    // parks itself. Since we are in a safepoint here, the timer is always
+    // initialized here already.
     CHECK(timer_.IsStarted());
     base::TimeDelta delta = timer_.Elapsed();
     TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("v8.gc"),
@@ -92,20 +120,5 @@ void CollectionBarrier::StopTimeToCollectionTimer() {
   }
 }
 
-void CollectionBarrier::ActivateStackGuardAndPostTask() {
-  Isolate* isolate = heap_->isolate();
-  ExecutionAccess access(isolate);
-  isolate->stack_guard()->RequestGC();
-
-  auto taskrunner = V8::GetCurrentPlatform()->GetForegroundTaskRunner(
-      reinterpret_cast<v8::Isolate*>(isolate));
-  taskrunner->PostTask(
-      std::make_unique<BackgroundCollectionInterruptTask>(heap_));
-
-  base::MutexGuard guard(&mutex_);
-  CHECK(!timer_.IsStarted());
-  timer_.Start();
-}
-
 }  // namespace internal
 }  // namespace v8

src/heap/collection-barrier.h

@@ -21,20 +21,14 @@ class Heap;
 
 // This class stops and resumes all background threads waiting for GC.
 class CollectionBarrier {
-  Heap* heap_;
-  base::Mutex mutex_;
-  base::ConditionVariable cv_wakeup_;
-  base::ElapsedTimer timer_;
-  bool shutdown_requested_;
-
-  LocalHeap::ThreadState main_thread_state_relaxed();
-
  public:
-  explicit CollectionBarrier(Heap* heap)
-      : heap_(heap), shutdown_requested_(false) {}
+  explicit CollectionBarrier(Heap* heap) : heap_(heap) {}
 
   // Returns true when collection was requested.
-  bool CollectionRequested();
+  bool WasGCRequested();
+
+  // Requests a GC from the main thread.
+  void RequestGC();
 
   // Resumes all threads waiting for GC when tear down starts.
   void NotifyShutdownRequested();
@@ -48,9 +42,17 @@ class CollectionBarrier {
   // This is the method use by background threads to request and wait for GC.
   bool AwaitCollectionBackground(LocalHeap* local_heap);
 
-  // Request GC by activating stack guards and posting a task to perform the
-  // GC.
+ private:
+  // Activate stack guards and posting a task to perform the GC.
   void ActivateStackGuardAndPostTask();
+
+  Heap* heap_;
+  base::Mutex mutex_;
+  base::ConditionVariable cv_wakeup_;
+  base::ElapsedTimer timer_;
+  std::atomic<bool> collection_requested_{false};
+  bool block_for_collection_ = false;
+  bool shutdown_requested_ = false;
 };
 
 }  // namespace internal

src/heap/heap.cc

@@ -1309,7 +1309,7 @@ void Heap::GarbageCollectionEpilogueInSafepoint(GarbageCollector collector) {
       main_thread_local_heap()->state_.exchange(LocalHeap::kRunning);
 
   CHECK(old_state == LocalHeap::kRunning ||
-        old_state == LocalHeap::kCollectionRequested);
+        old_state == LocalHeap::kSafepointRequested);
 
   // Resume all threads waiting for the GC.
   collection_barrier_->ResumeThreadsAwaitingCollection();
@@ -2084,7 +2084,7 @@ void Heap::EnsureFromSpaceIsCommitted() {
 }
 
 bool Heap::CollectionRequested() {
-  return collection_barrier_->CollectionRequested();
+  return collection_barrier_->WasGCRequested();
 }
 
 void Heap::CollectGarbageForBackground(LocalHeap* local_heap) {

src/heap/local-heap.cc

@@ -126,8 +126,7 @@ bool LocalHeap::IsHandleDereferenceAllowed() {
   VerifyCurrent();
 #endif
   ThreadState state = state_relaxed();
-  return state == kRunning || state == kSafepointRequested ||
-         state == kCollectionRequested;
+  return state == kRunning || state == kSafepointRequested;
 }
 #endif
 
@@ -136,14 +135,13 @@ bool LocalHeap::IsParked() {
   VerifyCurrent();
 #endif
   ThreadState state = state_relaxed();
-  return state == kParked || state == kParkedSafepointRequested ||
-         state == kParkedCollectionRequested;
+  return state == kParked || state == kParkedSafepointRequested;
 }
 
 void LocalHeap::ParkSlowPath(ThreadState current_state) {
   if (is_main_thread()) {
     while (true) {
-      CHECK_EQ(current_state, kCollectionRequested);
+      CHECK_EQ(current_state, kSafepointRequested);
       heap_->CollectGarbageForBackground(this);
 
       current_state = kRunning;
@@ -161,8 +159,8 @@ void LocalHeap::ParkSlowPath(ThreadState current_state) {
 
 void LocalHeap::UnparkSlowPath() {
   if (is_main_thread()) {
-    ThreadState expected = kParkedCollectionRequested;
-    CHECK(state_.compare_exchange_strong(expected, kCollectionRequested));
+    ThreadState expected = kParkedSafepointRequested;
+    CHECK(state_.compare_exchange_strong(expected, kSafepointRequested));
     heap_->CollectGarbageForBackground(this);
   } else {
     while (true) {
@@ -185,7 +183,7 @@ void LocalHeap::EnsureParkedBeforeDestruction() {
 
 void LocalHeap::SafepointSlowPath() {
   if (is_main_thread()) {
-    CHECK_EQ(kCollectionRequested, state_relaxed());
+    CHECK_EQ(kSafepointRequested, state_relaxed());
     heap_->CollectGarbageForBackground(this);
   } else {
     TRACE_GC1(heap_->tracer(), GCTracer::Scope::BACKGROUND_SAFEPOINT,
@@ -221,6 +219,8 @@ bool LocalHeap::TryPerformCollection() {
     heap_->CollectGarbageForBackground(this);
     return true;
   } else {
+    heap_->collection_barrier_->RequestGC();
+
     LocalHeap* main_thread = heap_->main_thread_local_heap();
     ThreadState current = main_thread->state_relaxed();
 
@@ -228,27 +228,25 @@ bool LocalHeap::TryPerformCollection() {
       switch (current) {
         case kRunning:
           if (main_thread->state_.compare_exchange_strong(
-                  current, kCollectionRequested)) {
-            heap_->collection_barrier_->ActivateStackGuardAndPostTask();
+                  current, kSafepointRequested)) {
             return heap_->collection_barrier_->AwaitCollectionBackground(this);
           }
           break;
 
-        case kCollectionRequested:
+        case kSafepointRequested:
           return heap_->collection_barrier_->AwaitCollectionBackground(this);
 
         case kParked:
           if (main_thread->state_.compare_exchange_strong(
-                  current, kParkedCollectionRequested)) {
-            heap_->collection_barrier_->ActivateStackGuardAndPostTask();
+                  current, kParkedSafepointRequested)) {
             return false;
           }
           break;
 
-        case kParkedCollectionRequested:
+        case kParkedSafepointRequested:
           return false;
 
-        default:
+        case kSafepoint:
           UNREACHABLE();
       }
     }

src/heap/local-heap.h

@@ -46,7 +46,6 @@ class V8_EXPORT_PRIVATE LocalHeap {
   void Safepoint() {
     DCHECK(AllowSafepoints::IsAllowed());
     ThreadState current = state_relaxed();
-    STATIC_ASSERT(kSafepointRequested == kCollectionRequested);
 
     // The following condition checks for both kSafepointRequested (background
     // thread) and kCollectionRequested (main thread).
@@ -150,31 +149,16 @@ class V8_EXPORT_PRIVATE LocalHeap {
     // or manipulate the heap in any way. This is considered to be a safepoint.
     kParked,
 
-    // SafepointRequested is used for Running background threads to force
-    // Safepoint() and
+    // SafepointRequested is used for Running threads to force Safepoint() and
     // Park() into the slow path.
     kSafepointRequested,
-    // A background thread transitions into this state from SafepointRequested
-    // when it
+    // A thread transitions into this state from SafepointRequested when it
     // enters a safepoint.
     kSafepoint,
     // This state is used for Parked background threads and forces Unpark() into
-    // the slow
-    // path. It prevents Unpark() to succeed before the safepoint operation is
-    // finished.
+    // the slow path. It prevents Unpark() to succeed before the safepoint
+    // operation is finished.
     kParkedSafepointRequested,
-
-    // This state is used on the main thread when at least one background thread
-    // requested a GC while the main thread was Running.
-    // We can use the same value for CollectionRequested and SafepointRequested
-    // since the first is only used on the main thread, while the other one only
-    // occurs on background threads. This property is used to have a faster
-    // check in Safepoint().
-    kCollectionRequested = kSafepointRequested,
-
-    // This state is used on the main thread when at least one background thread
-    // requested a GC while the main thread was Parked.
-    kParkedCollectionRequested,
   };
 
   ThreadState state_relaxed() { return state_.load(std::memory_order_relaxed); }