Reland "[heap] Uncommit marking deque in concurrent task."

This reverts commit 35e4a03f5b.

BUG=

Review-Url: https://codereview.chromium.org/2454723002
Cr-Commit-Position: refs/heads/master@{#40614}

src/cancelable-task.cc

@@ -48,8 +48,8 @@ void CancelableTaskManager::RemoveFinishedTask(uint32_t id) {
   cancelable_tasks_barrier_.NotifyOne();
 }
 
-
-bool CancelableTaskManager::TryAbort(uint32_t id) {
+CancelableTaskManager::TryAbortResult CancelableTaskManager::TryAbort(
+    uint32_t id) {
   base::LockGuard<base::Mutex> guard(&mutex_);
   auto entry = cancelable_tasks_.find(id);
   if (entry != cancelable_tasks_.end()) {
@@ -58,10 +58,12 @@ bool CancelableTaskManager::TryAbort(uint32_t id) {
       // Cannot call RemoveFinishedTask here because of recursive locking.
       cancelable_tasks_.erase(entry);
       cancelable_tasks_barrier_.NotifyOne();
-      return true;
+      return kTaskAborted;
+    } else {
+      return kTaskRunning;
     }
   }
-  return false;
+  return kTaskRemoved;
 }
 
 

src/cancelable-task.h

@@ -32,14 +32,14 @@ class V8_EXPORT_PRIVATE CancelableTaskManager {
   uint32_t Register(Cancelable* task);
 
   // Try to abort running a task identified by {id}. The possible outcomes are:
-  // (1) The task is already finished running and thus has been removed from
-  //     the manager.
+  // (1) The task is already finished running or was canceled before and
+  //     thus has been removed from the manager.
   // (2) The task is currently running and cannot be canceled anymore.
   // (3) The task is not yet running (or finished) so it is canceled and
   //     removed.
   //
-  // Returns {false} for (1) and (2), and {true} for (3).
-  bool TryAbort(uint32_t id);
+  enum TryAbortResult { kTaskRemoved, kTaskRunning, kTaskAborted };
+  TryAbortResult TryAbort(uint32_t id);
 
   // Cancels all remaining registered tasks and waits for tasks that are
   // already running. This disallows subsequent Register calls.

src/heap/mark-compact.cc

@@ -58,6 +58,7 @@ MarkCompactCollector::MarkCompactCollector(Heap* heap)
       compacting_(false),
       black_allocation_(false),
       have_code_to_deoptimize_(false),
+      marking_deque_(heap),
       code_flusher_(nullptr),
       sweeper_(heap) {
 }
@@ -2117,9 +2118,13 @@ void MarkingDeque::SetUp() {
   }
 }
 
-void MarkingDeque::TearDown() { delete backing_store_; }
+void MarkingDeque::TearDown() {
+  CancelOrWaitForUncommitTask();
+  delete backing_store_;
+}
 
 void MarkingDeque::StartUsing() {
+  base::LockGuard<base::Mutex> guard(&mutex_);
   if (in_use_) {
     // This can happen in mark-compact GC if the incremental marker already
     // started using the marking deque.
@@ -2139,11 +2144,16 @@ void MarkingDeque::StartUsing() {
 }
 
 void MarkingDeque::StopUsing() {
+  base::LockGuard<base::Mutex> guard(&mutex_);
   DCHECK(IsEmpty());
   DCHECK(!overflowed_);
   top_ = bottom_ = mask_ = 0;
-  Uncommit();
   in_use_ = false;
+  if (FLAG_concurrent_sweeping) {
+    StartUncommitTask();
+  } else {
+    Uncommit();
+  }
 }
 
 void MarkingDeque::Clear() {
@@ -2153,7 +2163,7 @@ void MarkingDeque::Clear() {
 }
 
 void MarkingDeque::Uncommit() {
-  DCHECK(in_use_);
+  DCHECK(!in_use_);
   bool success = backing_store_->Uncommit(backing_store_->address(),
                                           backing_store_committed_size_);
   backing_store_committed_size_ = 0;
@@ -2175,6 +2185,28 @@ void MarkingDeque::EnsureCommitted() {
   }
 }
 
+void MarkingDeque::StartUncommitTask() {
+  if (!uncommit_task_pending_) {
+    UncommitTask* task = new UncommitTask(heap_->isolate(), this);
+    uncommit_task_id_ = task->id();
+    uncommit_task_pending_ = true;
+    V8::GetCurrentPlatform()->CallOnBackgroundThread(
+        task, v8::Platform::kShortRunningTask);
+  }
+}
+
+void MarkingDeque::CancelOrWaitForUncommitTask() {
+  base::LockGuard<base::Mutex> guard(&mutex_);
+  if (!uncommit_task_pending_ ||
+      heap_->isolate()->cancelable_task_manager()->TryAbort(
+          uncommit_task_id_) != CancelableTaskManager::kTaskRunning) {
+    return;
+  }
+  while (uncommit_task_pending_) {
+    uncommit_task_barrier_.Wait(&mutex_);
+  }
+}
+
 class MarkCompactCollector::ObjectStatsVisitor
     : public MarkCompactCollector::HeapObjectVisitor {
  public:

src/heap/mark-compact.h

@@ -8,6 +8,8 @@
 #include <deque>
 
 #include "src/base/bits.h"
+#include "src/base/platform/condition-variable.h"
+#include "src/cancelable-task.h"
 #include "src/heap/marking.h"
 #include "src/heap/spaces.h"
 #include "src/heap/store-buffer.h"
@@ -52,7 +54,7 @@ class ObjectMarking : public AllStatic {
 // Marking deque for tracing live objects.
 class MarkingDeque {
  public:
-  MarkingDeque()
+  explicit MarkingDeque(Heap* heap)
       : backing_store_(nullptr),
         backing_store_committed_size_(0),
         array_(nullptr),
@@ -60,11 +62,16 @@ class MarkingDeque {
         bottom_(0),
         mask_(0),
         overflowed_(false),
-        in_use_(false) {}
+        in_use_(false),
+        uncommit_task_pending_(false),
+        uncommit_task_id_(0),
+        heap_(heap) {}
 
   void SetUp();
   void TearDown();
 
+  // Ensures that the marking deque is committed and will stay committed until
+  // StopUsing() is called.
   void StartUsing();
   void StopUsing();
   void Clear();
@@ -122,12 +129,45 @@ class MarkingDeque {
   void set_top(int top) { top_ = top; }
 
  private:
+  // This task uncommits the marking_deque backing store if
+  // markin_deque->in_use_ is false.
+  class UncommitTask : public CancelableTask {
+   public:
+    explicit UncommitTask(Isolate* isolate, MarkingDeque* marking_deque)
+        : CancelableTask(isolate), marking_deque_(marking_deque) {}
+
+   private:
+    // CancelableTask override.
+    void RunInternal() override {
+      base::LockGuard<base::Mutex> guard(&marking_deque_->mutex_);
+      if (!marking_deque_->in_use_) {
+        marking_deque_->Uncommit();
+      }
+      marking_deque_->uncommit_task_pending_ = false;
+      marking_deque_->uncommit_task_barrier_.NotifyOne();
+    }
+
+    MarkingDeque* marking_deque_;
+    DISALLOW_COPY_AND_ASSIGN(UncommitTask);
+  };
+
   static const size_t kMaxSize = 4 * MB;
   static const size_t kMinSize = 256 * KB;
 
+  // Must be called with mutex lock.
   void EnsureCommitted();
+
+  // Must be called with mutex lock.
   void Uncommit();
 
+  // Must be called with mutex lock.
+  void StartUncommitTask();
+
+  void CancelOrWaitForUncommitTask();
+
+  base::Mutex mutex_;
+  base::ConditionVariable uncommit_task_barrier_;
+
   base::VirtualMemory* backing_store_;
   size_t backing_store_committed_size_;
   HeapObject** array_;
@@ -138,7 +178,12 @@ class MarkingDeque {
   int bottom_;
   int mask_;
   bool overflowed_;
+  // in_use_ == true after taking mutex lock implies that the marking deque is
+  // committed and will stay committed at least until in_use_ == false.
   bool in_use_;
+  bool uncommit_task_pending_;
+  uint32_t uncommit_task_id_;
+  Heap* heap_;
 
   DISALLOW_COPY_AND_ASSIGN(MarkingDeque);
 };

src/heap/page-parallel-job.h

@@ -103,7 +103,8 @@ class PageParallelJob {
     delete main_task;
     // Wait for background tasks.
     for (int i = 0; i < num_tasks_; i++) {
-      if (!cancelable_task_manager_->TryAbort(task_ids[i])) {
+      if (cancelable_task_manager_->TryAbort(task_ids[i]) !=
+          CancelableTaskManager::kTaskAborted) {
         pending_tasks_->Wait();
       }
     }

src/wasm/wasm-module.cc

@@ -319,7 +319,8 @@ void WaitForCompilationTasks(Isolate* isolate, uint32_t* task_ids,
   for (size_t i = 0; i < num_tasks; ++i) {
     // If the task has not started yet, then we abort it. Otherwise we wait for
     // it to finish.
-    if (!isolate->cancelable_task_manager()->TryAbort(task_ids[i])) {
+    if (isolate->cancelable_task_manager()->TryAbort(task_ids[i]) !=
+        CancelableTaskManager::kTaskAborted) {
       pending_tasks->Wait();
     }
   }

test/cctest/heap/test-mark-compact.cc

@@ -51,7 +51,7 @@ using v8::Just;
 
 TEST(MarkingDeque) {
   CcTest::InitializeVM();
-  MarkingDeque s;
+  MarkingDeque s(CcTest::i_isolate()->heap());
   s.SetUp();
   s.StartUsing();
   Address original_address = reinterpret_cast<Address>(&s);