Revert "Preempt ConcurrentMarking tasks instead of merely pausing in PauseScope."

This reverts commit e9750cb8.

Reason for revert: Several GC failures, e.g. https://build.chromium.org/p/client.v8/builders/V8%20Linux/builds/23236, https://build.chromium.org/p/client.v8/builders/V8%20Mac/builds/18390 

Original change's description:
> Preempt ConcurrentMarking tasks instead of merely pausing in PauseScope.
> 
> Follow-up to https://chromium-review.googlesource.com/c/v8/v8/+/924867
> 
> This is the core goal of the initial CL @
> https://chromium-review.googlesource.com/c/v8/v8/+/922103
> which was since split into multiple to diagnose a bots-only failure.
> 
> Bug: chromium:812178
> Change-Id: I4c4e0b517737e020862917bd89fa6ce38244e597
> Reviewed-on: https://chromium-review.googlesource.com/924031
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#51356}

TBR=gab@chromium.org,ulan@chromium.org,mlippautz@chromium.org

Change-Id: Ic095e32708e58acbe5955bf29e65af34c59d321e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:812178
Reviewed-on: https://chromium-review.googlesource.com/925301Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51363}

src/heap/concurrent-marking.cc

@@ -436,8 +436,13 @@ void ConcurrentMarking::Run(int task_id, TaskState* task_state) {
                       GCTracer::BackgroundScope::MC_BACKGROUND_MARKING);
   size_t kBytesUntilInterruptCheck = 64 * KB;
   int kObjectsUntilInterrupCheck = 1000;
-  ConcurrentMarkingVisitor visitor(shared_, bailout_, &task_state->live_bytes,
-                                   weak_objects_, task_id);
+  LiveBytesMap* live_bytes = nullptr;
+  {
+    base::LockGuard<base::Mutex> guard(&task_state->lock);
+    live_bytes = &task_state->live_bytes;
+  }
+  ConcurrentMarkingVisitor visitor(shared_, bailout_, live_bytes, weak_objects_,
+                                   task_id);
   double time_ms;
   size_t marked_bytes = 0;
   if (FLAG_trace_concurrent_marking) {
@@ -446,41 +451,48 @@ void ConcurrentMarking::Run(int task_id, TaskState* task_state) {
   }
   {
     TimedScope scope(&time_ms);
-
-    bool done = false;
-    while (!done) {
-      size_t current_marked_bytes = 0;
-      int objects_processed = 0;
-      while (current_marked_bytes < kBytesUntilInterruptCheck &&
-             objects_processed < kObjectsUntilInterrupCheck) {
-        HeapObject* object;
-        if (!shared_->Pop(task_id, &object)) {
-          done = true;
-          break;
+    {
+      base::LockGuard<base::Mutex> guard(&task_state->lock);
+      bool done = false;
+      while (!done) {
+        size_t current_marked_bytes = 0;
+        int objects_processed = 0;
+        while (current_marked_bytes < kBytesUntilInterruptCheck &&
+               objects_processed < kObjectsUntilInterrupCheck) {
+          HeapObject* object;
+          if (!shared_->Pop(task_id, &object)) {
+            done = true;
+            break;
+          }
+          objects_processed++;
+          Address new_space_top = heap_->new_space()->original_top();
+          Address new_space_limit = heap_->new_space()->original_limit();
+          Address addr = object->address();
+          if (new_space_top <= addr && addr < new_space_limit) {
+            on_hold_->Push(task_id, object);
+          } else {
+            Map* map = object->synchronized_map();
+            current_marked_bytes += visitor.Visit(map, object);
+          }
         }
-        objects_processed++;
-        Address new_space_top = heap_->new_space()->original_top();
-        Address new_space_limit = heap_->new_space()->original_limit();
-        Address addr = object->address();
-        if (new_space_top <= addr && addr < new_space_limit) {
-          on_hold_->Push(task_id, object);
-        } else {
-          Map* map = object->synchronized_map();
-          current_marked_bytes += visitor.Visit(map, object);
+        marked_bytes += current_marked_bytes;
+        base::AsAtomicWord::Relaxed_Store<size_t>(&task_state->marked_bytes,
+                                                  marked_bytes);
+        if (task_state->interrupt_request.Value()) {
+          TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.gc"),
+                       "ConcurrentMarking::Run Paused");
+          task_state->interrupt_condition.Wait(&task_state->lock);
+        } else if (task_state->preemption_request.Value()) {
+          TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.gc"),
+                       "ConcurrentMarking::Run Preempted");
+          break;
         }
       }
-      marked_bytes += current_marked_bytes;
-      base::AsAtomicWord::Relaxed_Store<size_t>(&task_state->marked_bytes,
-                                                marked_bytes);
-      if (task_state->preemption_request.Value()) {
-        TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.gc"),
-                     "ConcurrentMarking::Run Preempted");
-        break;
-      }
+      // The lock is also required to synchronize with worklist update after
+      // young generation GC.
+      bailout_->FlushToGlobal(task_id);
+      on_hold_->FlushToGlobal(task_id);
     }
-    bailout_->FlushToGlobal(task_id);
-    on_hold_->FlushToGlobal(task_id);
-
     weak_objects_->weak_cells.FlushToGlobal(task_id);
     weak_objects_->transition_arrays.FlushToGlobal(task_id);
     base::AsAtomicWord::Relaxed_Store<size_t>(&task_state->marked_bytes, 0);
@@ -522,6 +534,7 @@ void ConcurrentMarking::ScheduleTasks() {
             "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_;
       Task* task = new Task(heap_->isolate(), this, &task_state_[i], i);
@@ -610,12 +623,23 @@ size_t ConcurrentMarking::TotalMarkedBytes() {
 ConcurrentMarking::PauseScope::PauseScope(ConcurrentMarking* concurrent_marking)
     : concurrent_marking_(concurrent_marking) {
   if (!FLAG_concurrent_marking) return;
-  concurrent_marking_->Stop(ConcurrentMarking::StopRequest::PREEMPT_TASKS);
+  // Request task_state for all tasks.
+  for (int i = 1; i <= kMaxTasks; i++) {
+    concurrent_marking_->task_state_[i].interrupt_request.SetValue(true);
+  }
+  // Now take a lock to ensure that the tasks are waiting.
+  for (int i = 1; i <= kMaxTasks; i++) {
+    concurrent_marking_->task_state_[i].lock.Lock();
+  }
 }
 
 ConcurrentMarking::PauseScope::~PauseScope() {
   if (!FLAG_concurrent_marking) return;
-  concurrent_marking_->RescheduleTasksIfNeeded();
+  for (int i = kMaxTasks; i >= 1; i--) {
+    concurrent_marking_->task_state_[i].interrupt_request.SetValue(false);
+    concurrent_marking_->task_state_[i].interrupt_condition.NotifyAll();
+    concurrent_marking_->task_state_[i].lock.Unlock();
+  }
 }
 
 }  // namespace internal

src/heap/concurrent-marking.h

@@ -30,8 +30,7 @@ using LiveBytesMap =
 class ConcurrentMarking {
  public:
   // When the scope is entered, the concurrent marking tasks
-  // are preempted and are not looking at the heap objects, concurrent marking
-  // is resumed when the scope is exited.
+  // are paused and are not looking at the heap objects.
   class PauseScope {
    public:
     explicit PauseScope(ConcurrentMarking* concurrent_marking);
@@ -82,6 +81,16 @@ class ConcurrentMarking {
     // 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;
+    // The main thread sets this flag to true, when it wants the concurrent
+    // maker to give up the lock.
+    base::AtomicValue<bool> interrupt_request;
+    // The concurrent marker waits on this condition until the request
+    // flag is cleared by the main thread.
+    base::ConditionVariable interrupt_condition;
+
     LiveBytesMap live_bytes;
     size_t marked_bytes = 0;
     char cache_line_padding[64];