[heap] Improve concurrent marking pausing protocol.

This patch allows the concurrent marker to process more objects before
checking for the interrupt request from the main thread.

Bug: chromium:694255
TBR: mlippautz@chromium.org
Change-Id: I876d3156ca9843196f2fdddbd8bd28d1a3f472b1
Reviewed-on: https://chromium-review.googlesource.com/602131
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47182}

BUILD.gn

@@ -77,7 +77,7 @@ declare_args() {
   v8_enable_trace_ignition = false
 
   # Sets -dV8_CONCURRENT_MARKING
-  v8_enable_concurrent_marking = false
+  v8_enable_concurrent_marking = true
 
   # Sets -dV8_CSA_WRITE_BARRIER
   v8_enable_csa_write_barrier = false

src/heap/concurrent-marking.cc

@@ -290,10 +290,10 @@ class ConcurrentMarkingVisitor final
 class ConcurrentMarking::Task : public CancelableTask {
  public:
   Task(Isolate* isolate, ConcurrentMarking* concurrent_marking,
-       base::Mutex* lock, int task_id)
+       TaskInterrupt* interrupt, int task_id)
       : CancelableTask(isolate),
         concurrent_marking_(concurrent_marking),
-        lock_(lock),
+        interrupt_(interrupt),
         task_id_(task_id) {}
 
   virtual ~Task() {}
@@ -301,11 +301,11 @@ class ConcurrentMarking::Task : public CancelableTask {
  private:
   // v8::internal::CancelableTask overrides.
   void RunInternal() override {
-    concurrent_marking_->Run(task_id_, lock_);
+    concurrent_marking_->Run(task_id_, interrupt_);
   }
 
   ConcurrentMarking* concurrent_marking_;
-  base::Mutex* lock_;
+  TaskInterrupt* interrupt_;
   int task_id_;
   DISALLOW_COPY_AND_ASSIGN(Task);
 };
@@ -327,20 +327,31 @@ ConcurrentMarking::ConcurrentMarking(Heap* heap, MarkingWorklist* shared,
   }
 }
 
-void ConcurrentMarking::Run(int task_id, base::Mutex* lock) {
+void ConcurrentMarking::Run(int task_id, TaskInterrupt* interrupt) {
+  size_t kBytesUntilInterruptCheck = 64 * KB;
+  int kObjectsUntilInterrupCheck = 1000;
   ConcurrentMarkingVisitor visitor(shared_, bailout_, weak_cells_, task_id);
   double time_ms;
-  size_t bytes_marked = 0;
+  size_t total_bytes_marked = 0;
   if (FLAG_trace_concurrent_marking) {
     heap_->isolate()->PrintWithTimestamp(
         "Starting concurrent marking task %d\n", task_id);
   }
   {
     TimedScope scope(&time_ms);
-    while (true) {
-      base::LockGuard<base::Mutex> guard(lock);
+    bool done = false;
+    while (!done) {
+      base::LockGuard<base::Mutex> guard(&interrupt->lock);
+      size_t bytes_marked = 0;
+      int objects_processed = 0;
+      while (bytes_marked < kBytesUntilInterruptCheck &&
+             objects_processed < kObjectsUntilInterrupCheck) {
         HeapObject* object;
-      if (!shared_->Pop(task_id, &object)) break;
+        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();
@@ -351,10 +362,15 @@ void ConcurrentMarking::Run(int task_id, base::Mutex* lock) {
           bytes_marked += visitor.Visit(map, object);
         }
       }
+      total_bytes_marked += bytes_marked;
+      if (interrupt->request.Value()) {
+        interrupt->condition.Wait(&interrupt->lock);
+      }
+    }
     {
       // Take the lock to synchronize with worklist update after
       // young generation GC.
-      base::LockGuard<base::Mutex> guard(lock);
+      base::LockGuard<base::Mutex> guard(&interrupt->lock);
       bailout_->FlushToGlobal(task_id);
     }
     weak_cells_->FlushToGlobal(task_id);
@@ -368,7 +384,7 @@ void ConcurrentMarking::Run(int task_id, base::Mutex* lock) {
   if (FLAG_trace_concurrent_marking) {
     heap_->isolate()->PrintWithTimestamp(
         "Task %d concurrently marked %dKB in %.2fms\n", task_id,
-        static_cast<int>(bytes_marked / KB), time_ms);
+        static_cast<int>(total_bytes_marked / KB), time_ms);
   }
 }
 
@@ -383,10 +399,11 @@ void ConcurrentMarking::ScheduleTasks() {
           heap_->isolate()->PrintWithTimestamp(
               "Scheduling concurrent marking task %d\n", i);
         }
+        task_interrupt_[i].request.SetValue(false);
         is_pending_[i] = true;
         ++pending_task_count_;
         V8::GetCurrentPlatform()->CallOnBackgroundThread(
-            new Task(heap_->isolate(), this, &task_lock_[i].lock, i),
+            new Task(heap_->isolate(), this, &task_interrupt_[i], i),
             v8::Platform::kShortRunningTask);
       }
     }
@@ -415,15 +432,22 @@ void ConcurrentMarking::EnsureCompleted() {
 ConcurrentMarking::PauseScope::PauseScope(ConcurrentMarking* concurrent_marking)
     : concurrent_marking_(concurrent_marking) {
   if (!FLAG_concurrent_marking) return;
+  // Request interrupt for all tasks.
   for (int i = 1; i <= kTasks; i++) {
-    concurrent_marking_->task_lock_[i].lock.Lock();
+    concurrent_marking_->task_interrupt_[i].request.SetValue(true);
+  }
+  // Now take a lock to ensure that the tasks are waiting.
+  for (int i = 1; i <= kTasks; i++) {
+    concurrent_marking_->task_interrupt_[i].lock.Lock();
   }
 }
 
 ConcurrentMarking::PauseScope::~PauseScope() {
   if (!FLAG_concurrent_marking) return;
   for (int i = kTasks; i >= 1; i--) {
-    concurrent_marking_->task_lock_[i].lock.Unlock();
+    concurrent_marking_->task_interrupt_[i].request.SetValue(false);
+    concurrent_marking_->task_interrupt_[i].condition.NotifyAll();
+    concurrent_marking_->task_interrupt_[i].lock.Unlock();
   }
 }
 

src/heap/concurrent-marking.h

@@ -43,17 +43,25 @@ class ConcurrentMarking {
   void RescheduleTasksIfNeeded();
 
  private:
-  struct TaskLock {
+  struct TaskInterrupt {
+    // 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> request;
+    // The concurrent marker waits on this condition until the request
+    // flag is cleared by the main thread.
+    base::ConditionVariable condition;
     char cache_line_padding[64];
   };
   class Task;
-  void Run(int task_id, base::Mutex* lock);
+  void Run(int task_id, TaskInterrupt* interrupt);
   Heap* heap_;
   MarkingWorklist* shared_;
   MarkingWorklist* bailout_;
   WeakCellWorklist* weak_cells_;
-  TaskLock task_lock_[kTasks + 1];
+  TaskInterrupt task_interrupt_[kTasks + 1];
   base::Mutex pending_lock_;
   base::ConditionVariable pending_condition_;
   int pending_task_count_;