Revert "Introduce ConcurrentMarking::StopRequest API."

This reverts commit f4b41099.

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:
> 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}

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

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

src/heap/concurrent-marking.cc

@@ -480,8 +480,6 @@ 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
@@ -529,7 +527,6 @@ 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_;
@@ -553,22 +550,25 @@ void ConcurrentMarking::RescheduleTasksIfNeeded() {
   }
 }
 
-void ConcurrentMarking::Stop(StopRequest stop_request) {
+void ConcurrentMarking::WaitForTasks() {
   if (!FLAG_concurrent_marking) return;
   base::LockGuard<base::Mutex> guard(&pending_lock_);
+  while (pending_task_count_ > 0) {
+    pending_condition_.Wait(&pending_lock_);
+  }
+}
 
-  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);
-        }
+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_;
       }
     }
   }

src/heap/concurrent-marking.h

@@ -40,17 +40,6 @@ 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 */>;
 
@@ -58,12 +47,9 @@ 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 Stop(StopRequest stop_request);
-
+  void WaitForTasks();
+  void EnsureCompleted();
   void RescheduleTasksIfNeeded();
   // Flushes the local live bytes into the given marking state.
   void FlushLiveBytes(MajorNonAtomicMarkingState* marking_state);
@@ -77,10 +63,6 @@ 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,6 +16,7 @@
 #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"
@@ -866,9 +867,7 @@ void MarkCompactCollector::Prepare() {
   if (was_marked_incrementally_ && heap_->ShouldAbortIncrementalMarking()) {
     heap()->incremental_marking()->Stop();
     heap()->incremental_marking()->AbortBlackAllocation();
-    // TODO(gab): PREEMPT_TASKS here in a follow-up CL.
-    FinishConcurrentMarking(
-        ConcurrentMarking::StopRequest::COMPLETE_ONGOING_TASKS);
+    FinishConcurrentMarking();
     heap()->incremental_marking()->Deactivate();
     ClearMarkbits();
     AbortWeakCollections();
@@ -904,10 +903,9 @@ void MarkCompactCollector::Prepare() {
 #endif
 }
 
-void MarkCompactCollector::FinishConcurrentMarking(
-    ConcurrentMarking::StopRequest stop_request) {
+void MarkCompactCollector::FinishConcurrentMarking() {
   if (FLAG_concurrent_marking) {
-    heap()->concurrent_marking()->Stop(stop_request);
+    heap()->concurrent_marking()->EnsureCompleted();
     heap()->concurrent_marking()->FlushLiveBytes(non_atomic_marking_state());
   }
 }
@@ -2360,8 +2358,7 @@ void MarkCompactCollector::MarkLiveObjects() {
     }
     ProcessMarkingWorklist();
 
-    FinishConcurrentMarking(
-        ConcurrentMarking::StopRequest::COMPLETE_ONGOING_TASKS);
+    FinishConcurrentMarking();
     ProcessMarkingWorklist();
   }
 

src/heap/mark-compact.h

@@ -8,7 +8,6 @@
 #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"
@@ -650,9 +649,7 @@ class MarkCompactCollector final : public MarkCompactCollectorBase {
   // choosing spaces to compact.
   void Prepare();
 
-  // 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);
+  void FinishConcurrentMarking();
 
   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->Stop(
-      ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
+  concurrent_marking->WaitForTasks();
+  concurrent_marking->EnsureCompleted();
   delete concurrent_marking;
 }
 
@@ -66,39 +66,12 @@ TEST(ConcurrentMarkingReschedule) {
       new ConcurrentMarking(heap, &shared, &bailout, &on_hold, &weak_objects);
   PublishSegment(&shared, heap->undefined_value());
   concurrent_marking->ScheduleTasks();
-  concurrent_marking->Stop(
-      ConcurrentMarking::StopRequest::COMPLETE_ONGOING_TASKS);
+  concurrent_marking->WaitForTasks();
+  concurrent_marking->EnsureCompleted();
   PublishSegment(&shared, heap->undefined_value());
   concurrent_marking->RescheduleTasksIfNeeded();
-  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);
+  concurrent_marking->WaitForTasks();
+  concurrent_marking->EnsureCompleted();
   delete concurrent_marking;
 }
 
@@ -112,8 +85,8 @@ TEST(ConcurrentMarkingMarkedBytes) {
   CcTest::CollectAllGarbage();
   if (!heap->incremental_marking()->IsStopped()) return;
   heap::SimulateIncrementalMarking(heap, false);
-  heap->concurrent_marking()->Stop(
-      ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
+  heap->concurrent_marking()->WaitForTasks();
+  heap->concurrent_marking()->EnsureCompleted();
   CHECK_GE(heap->concurrent_marking()->TotalMarkedBytes(), root->Size());
 }