Reland "Introduce ConcurrentMarking::StopRequest API."

This is a reland of f4b4109936.

Not expected to be the culprit of the 4 CL revert.

Original change's description:
> Introduce ConcurrentMarking::StopRequest API.
>
> This was extracted from https://chromium-review.googlesource.com/c/v8/v8/+/924073/10
> after it became clear that using COMPLETE_TASKS/PREEMPT_TASKS where
> it should make sense to doesn't work in practice for now.
>
> Experimental CLs which led to the above conclusion:
>  - https://chromium-review.googlesource.com/c/v8/v8/+/924865
>    (COMPLETE or CANCEL -- still broken)
>  - https://chromium-review.googlesource.com/c/v8/v8/+/924866
>    (CANCEL only, as before, works)
>  - https://chromium-review.googlesource.com/c/v8/v8/+/924028
>    (CANCEL and PREEMPT -- broken as well)
>
> Introducing this unittested API allows to reduce the size
> of the CLs causing hard-to-diagnose bots-only failures
> and fix them individually follow-ups @
>
>  1) https://chromium-review.googlesource.com/c/v8/v8/+/924029
>  2) https://chromium-review.googlesource.com/c/v8/v8/+/924031
>  3) https://chromium-review.googlesource.com/c/v8/v8/+/924030
>
> Bug: chromium:812178
> Change-Id: Icdac456e9f7874b0c4b321ccdb8898297dad7d73
> Reviewed-on: https://chromium-review.googlesource.com/924867
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#51353}

Bug: chromium:812178
Change-Id: Iaa32f9cc6b2fa7004c7fae1f79aa4b00f5f8f34c
Reviewed-on: https://chromium-review.googlesource.com/924006
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51371}

src/heap/concurrent-marking.cc

@@ -480,6 +480,8 @@ void ConcurrentMarking::Run(int task_id, TaskState* task_state) {
                                                   marked_bytes);
         if (task_state->interrupt_request.Value()) {
           task_state->interrupt_condition.Wait(&task_state->lock);
+        } else if (task_state->preemption_request.Value()) {
+          break;
         }
       }
       // The lock is also required to synchronize with worklist update after
@@ -527,6 +529,7 @@ void ConcurrentMarking::ScheduleTasks() {
         heap_->isolate()->PrintWithTimestamp(
             "Scheduling concurrent marking task %d\n", i);
       }
+      task_state_[i].preemption_request.SetValue(false);
       task_state_[i].interrupt_request.SetValue(false);
       is_pending_[i] = true;
       ++pending_task_count_;
@@ -550,25 +553,22 @@ void ConcurrentMarking::RescheduleTasksIfNeeded() {
   }
 }
 
-void ConcurrentMarking::WaitForTasks() {
+void ConcurrentMarking::Stop(StopRequest stop_request) {
   if (!FLAG_concurrent_marking) return;
   base::LockGuard<base::Mutex> guard(&pending_lock_);
-  while (pending_task_count_ > 0) {
-    pending_condition_.Wait(&pending_lock_);
-  }
-}
 
-void ConcurrentMarking::EnsureCompleted() {
-  if (!FLAG_concurrent_marking) return;
-  base::LockGuard<base::Mutex> guard(&pending_lock_);
-  CancelableTaskManager* task_manager =
-      heap_->isolate()->cancelable_task_manager();
-  for (int i = 1; i <= task_count_; i++) {
-    if (is_pending_[i]) {
-      if (task_manager->TryAbort(cancelable_id_[i]) ==
-          CancelableTaskManager::kTaskAborted) {
-        is_pending_[i] = false;
-        --pending_task_count_;
+  if (stop_request != StopRequest::COMPLETE_TASKS_FOR_TESTING) {
+    CancelableTaskManager* task_manager =
+        heap_->isolate()->cancelable_task_manager();
+    for (int i = 1; i <= task_count_; i++) {
+      if (is_pending_[i]) {
+        if (task_manager->TryAbort(cancelable_id_[i]) ==
+            CancelableTaskManager::kTaskAborted) {
+          is_pending_[i] = false;
+          --pending_task_count_;
+        } else if (stop_request == StopRequest::PREEMPT_TASKS) {
+          task_state_[i].preemption_request.SetValue(true);
+        }
       }
     }
   }

src/heap/concurrent-marking.h

@@ -40,6 +40,17 @@ class ConcurrentMarking {
     ConcurrentMarking* concurrent_marking_;
   };
 
+  enum class StopRequest {
+    // Preempt ongoing tasks ASAP (and cancel unstarted tasks).
+    PREEMPT_TASKS,
+    // Wait for ongoing tasks to complete (and cancels unstarted tasks).
+    COMPLETE_ONGOING_TASKS,
+    // Wait for all scheduled tasks to complete (only use this in tests that
+    // control the full stack -- otherwise tasks cancelled by the platform can
+    // make this call hang).
+    COMPLETE_TASKS_FOR_TESTING,
+  };
+
   static constexpr int kMaxTasks = 4;
   using MarkingWorklist = Worklist<HeapObject*, 64 /* segment size */>;
 
@@ -47,9 +58,12 @@ class ConcurrentMarking {
                     MarkingWorklist* bailout, MarkingWorklist* on_hold,
                     WeakObjects* weak_objects);
 
+  // Schedules asynchronous tasks to perform concurrent marking. Objects in the
+  // heap should not be moved while these are active (can be stopped safely via
+  // Stop() or PauseScope).
   void ScheduleTasks();
-  void WaitForTasks();
-  void EnsureCompleted();
+  void Stop(StopRequest stop_request);
+
   void RescheduleTasksIfNeeded();
   // Flushes the local live bytes into the given marking state.
   void FlushLiveBytes(MajorNonAtomicMarkingState* marking_state);
@@ -63,6 +77,10 @@ class ConcurrentMarking {
 
  private:
   struct TaskState {
+    // The main thread sets this flag to true when it wants the concurrent
+    // marker to give up the worker thread.
+    base::AtomicValue<bool> preemption_request;
+
     // When the concurrent marking task has this lock, then objects in the
     // heap are guaranteed to not move.
     base::Mutex lock;

src/heap/mark-compact.cc

@@ -16,7 +16,6 @@
 #include "src/global-handles.h"
 #include "src/heap/array-buffer-collector.h"
 #include "src/heap/array-buffer-tracker-inl.h"
-#include "src/heap/concurrent-marking.h"
 #include "src/heap/gc-tracer.h"
 #include "src/heap/incremental-marking.h"
 #include "src/heap/invalidated-slots-inl.h"
@@ -867,7 +866,9 @@ void MarkCompactCollector::Prepare() {
   if (was_marked_incrementally_ && heap_->ShouldAbortIncrementalMarking()) {
     heap()->incremental_marking()->Stop();
     heap()->incremental_marking()->AbortBlackAllocation();
-    FinishConcurrentMarking();
+    // TODO(gab): PREEMPT_TASKS here in a follow-up CL.
+    FinishConcurrentMarking(
+        ConcurrentMarking::StopRequest::COMPLETE_ONGOING_TASKS);
     heap()->incremental_marking()->Deactivate();
     ClearMarkbits();
     AbortWeakCollections();
@@ -903,9 +904,10 @@ void MarkCompactCollector::Prepare() {
 #endif
 }
 
-void MarkCompactCollector::FinishConcurrentMarking() {
+void MarkCompactCollector::FinishConcurrentMarking(
+    ConcurrentMarking::StopRequest stop_request) {
   if (FLAG_concurrent_marking) {
-    heap()->concurrent_marking()->EnsureCompleted();
+    heap()->concurrent_marking()->Stop(stop_request);
     heap()->concurrent_marking()->FlushLiveBytes(non_atomic_marking_state());
   }
 }
@@ -2358,7 +2360,8 @@ void MarkCompactCollector::MarkLiveObjects() {
     }
     ProcessMarkingWorklist();
 
-    FinishConcurrentMarking();
+    FinishConcurrentMarking(
+        ConcurrentMarking::StopRequest::COMPLETE_ONGOING_TASKS);
     ProcessMarkingWorklist();
   }
 

src/heap/mark-compact.h

@@ -8,6 +8,7 @@
 #include <deque>
 #include <vector>
 
+#include "src/heap/concurrent-marking.h"
 #include "src/heap/marking.h"
 #include "src/heap/objects-visiting.h"
 #include "src/heap/spaces.h"
@@ -649,7 +650,9 @@ class MarkCompactCollector final : public MarkCompactCollectorBase {
   // choosing spaces to compact.
   void Prepare();
 
-  void FinishConcurrentMarking();
+  // Stop concurrent marking (either by preempting it right away or waiting for
+  // it to complete as requested by |stop_request|).
+  void FinishConcurrentMarking(ConcurrentMarking::StopRequest stop_request);
 
   bool StartCompaction();
 

test/cctest/heap/test-concurrent-marking.cc

@@ -44,8 +44,8 @@ TEST(ConcurrentMarking) {
       new ConcurrentMarking(heap, &shared, &bailout, &on_hold, &weak_objects);
   PublishSegment(&shared, heap->undefined_value());
   concurrent_marking->ScheduleTasks();
-  concurrent_marking->WaitForTasks();
-  concurrent_marking->EnsureCompleted();
+  concurrent_marking->Stop(
+      ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
   delete concurrent_marking;
 }
 
@@ -66,12 +66,39 @@ TEST(ConcurrentMarkingReschedule) {
       new ConcurrentMarking(heap, &shared, &bailout, &on_hold, &weak_objects);
   PublishSegment(&shared, heap->undefined_value());
   concurrent_marking->ScheduleTasks();
-  concurrent_marking->WaitForTasks();
-  concurrent_marking->EnsureCompleted();
+  concurrent_marking->Stop(
+      ConcurrentMarking::StopRequest::COMPLETE_ONGOING_TASKS);
   PublishSegment(&shared, heap->undefined_value());
   concurrent_marking->RescheduleTasksIfNeeded();
-  concurrent_marking->WaitForTasks();
-  concurrent_marking->EnsureCompleted();
+  concurrent_marking->Stop(
+      ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
+  delete concurrent_marking;
+}
+
+TEST(ConcurrentMarkingPreemptAndReschedule) {
+  if (!i::FLAG_concurrent_marking) return;
+  CcTest::InitializeVM();
+  Heap* heap = CcTest::heap();
+  CcTest::CollectAllGarbage();
+  if (!heap->incremental_marking()->IsStopped()) return;
+  MarkCompactCollector* collector = CcTest::heap()->mark_compact_collector();
+  if (collector->sweeping_in_progress()) {
+    collector->EnsureSweepingCompleted();
+  }
+
+  ConcurrentMarking::MarkingWorklist shared, bailout, on_hold;
+  WeakObjects weak_objects;
+  ConcurrentMarking* concurrent_marking =
+      new ConcurrentMarking(heap, &shared, &bailout, &on_hold, &weak_objects);
+  for (int i = 0; i < 5000; i++)
+    PublishSegment(&shared, heap->undefined_value());
+  concurrent_marking->ScheduleTasks();
+  concurrent_marking->Stop(ConcurrentMarking::StopRequest::PREEMPT_TASKS);
+  for (int i = 0; i < 5000; i++)
+    PublishSegment(&shared, heap->undefined_value());
+  concurrent_marking->RescheduleTasksIfNeeded();
+  concurrent_marking->Stop(
+      ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
   delete concurrent_marking;
 }
 
@@ -85,8 +112,8 @@ TEST(ConcurrentMarkingMarkedBytes) {
   CcTest::CollectAllGarbage();
   if (!heap->incremental_marking()->IsStopped()) return;
   heap::SimulateIncrementalMarking(heap, false);
-  heap->concurrent_marking()->WaitForTasks();
-  heap->concurrent_marking()->EnsureCompleted();
+  heap->concurrent_marking()->Stop(
+      ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
   CHECK_GE(heap->concurrent_marking()->TotalMarkedBytes(), root->Size());
 }