[heap] Factor out the barrier from Scavenger and add tests

Bug: chromium:738865, chromium:750084
Change-Id: Ife30da4be118cd6f3212e84752978ebb39500f15
Reviewed-on: https://chromium-review.googlesource.com/641414
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47710}

BUILD.gn

@@ -1600,6 +1600,7 @@ v8_source_set("v8_base") {
     "src/heap/array-buffer-tracker-inl.h",
     "src/heap/array-buffer-tracker.cc",
     "src/heap/array-buffer-tracker.h",
+    "src/heap/barrier.h",
     "src/heap/code-stats.cc",
     "src/heap/code-stats.h",
     "src/heap/concurrent-marking.cc",

src/heap/barrier.h

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_HEAP_BARRIER_H_
+#define V8_HEAP_BARRIER_H_
+
+#include "src/base/platform/condition-variable.h"
+#include "src/base/platform/mutex.h"
+
+namespace v8 {
+namespace internal {
+
+// Barrier that can be used once to synchronize a dynamic number of tasks
+// working concurrently.
+//
+// Usage:
+//   void RunConcurrently(OneShotBarrier* shared_barrier) {
+//     shared_barrier->Start();
+//     do {
+//       {
+//         /* process work and create new work */
+//         barrier->NotifyAll();
+//         /* process work and create new work */
+//       }
+//     } while(!shared_barrier->Wait());
+//   }
+//
+// Note: If Start() is not called in time, e.g., because the first concurrent
+// task is already done processing all work, then Done() will return true
+// immediately.
+class OneshotBarrier {
+ public:
+  OneshotBarrier() : tasks_(0), waiting_(0), done_(false) {}
+
+  void Start() {
+    base::LockGuard<base::Mutex> guard(&mutex_);
+    tasks_++;
+  }
+
+  void NotifyAll() {
+    base::LockGuard<base::Mutex> guard(&mutex_);
+    if (waiting_ > 0) condition_.NotifyAll();
+  }
+
+  bool Wait() {
+    base::LockGuard<base::Mutex> guard(&mutex_);
+    if (done_) return true;
+
+    DCHECK_LE(waiting_, tasks_);
+    waiting_++;
+    if (waiting_ == tasks_) {
+      done_ = true;
+      condition_.NotifyAll();
+    } else {
+      // Spurious wakeup is ok here.
+      condition_.Wait(&mutex_);
+    }
+    waiting_--;
+    return done_;
+  }
+
+  // Only valid to be called in a sequential setting.
+  bool DoneForTesting() const { return done_; }
+
+ private:
+  base::ConditionVariable condition_;
+  base::Mutex mutex_;
+  int tasks_;
+  int waiting_;
+  bool done_;
+};
+
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_HEAP_BARRIER_H_

src/heap/heap.cc

@@ -24,6 +24,7 @@
 #include "src/feedback-vector.h"
 #include "src/global-handles.h"
 #include "src/heap/array-buffer-tracker-inl.h"
+#include "src/heap/barrier.h"
 #include "src/heap/code-stats.h"
 #include "src/heap/concurrent-marking.h"
 #include "src/heap/embedder-tracing.h"
@@ -1805,7 +1806,7 @@ class ScavengingItem : public ItemParallelJob::Item {
 
 class ScavengingTask final : public ItemParallelJob::Task {
  public:
-  ScavengingTask(Heap* heap, Scavenger* scavenger, Scavenger::Barrier* barrier)
+  ScavengingTask(Heap* heap, Scavenger* scavenger, OneshotBarrier* barrier)
       : ItemParallelJob::Task(heap->isolate()),
         heap_(heap),
         scavenger_(scavenger),
@@ -1821,10 +1822,9 @@ class ScavengingTask final : public ItemParallelJob::Task {
         item->Process(scavenger_);
         item->MarkFinished();
       }
-      while (!barrier_->Done()) {
+      do {
         scavenger_->Process(barrier_);
-        barrier_->Wait();
-      }
+      } while (!barrier_->Wait());
       scavenger_->Process();
     }
     if (FLAG_trace_parallel_scavenge) {
@@ -1838,7 +1838,7 @@ class ScavengingTask final : public ItemParallelJob::Task {
  private:
   Heap* const heap_;
   Scavenger* const scavenger_;
-  Scavenger::Barrier* const barrier_;
+  OneshotBarrier* const barrier_;
 };
 
 class PageScavengingItem final : public ScavengingItem {
@@ -1921,7 +1921,7 @@ void Heap::Scavenge() {
   Scavenger* scavengers[kMaxScavengerTasks];
   const bool is_logging = IsLogging(isolate());
   const int num_scavenge_tasks = NumberOfScavengeTasks();
-  Scavenger::Barrier barrier;
+  OneshotBarrier barrier;
   CopiedList copied_list(num_scavenge_tasks);
   PromotionList promotion_list(num_scavenge_tasks);
   for (int i = 0; i < num_scavenge_tasks; i++) {

src/heap/scavenger.cc

@@ -4,6 +4,7 @@
 
 #include "src/heap/scavenger.h"
 
+#include "src/heap/barrier.h"
 #include "src/heap/heap-inl.h"
 #include "src/heap/mark-compact-inl.h"
 #include "src/heap/objects-visiting-inl.h"
@@ -91,7 +92,7 @@ void Scavenger::IterateAndScavengePromotedObject(HeapObject* target, int size) {
   }
 }
 
-void Scavenger::Process(Barrier* barrier) {
+void Scavenger::Process(OneshotBarrier* barrier) {
   // Threshold when to switch processing the promotion list to avoid
   // allocating too much backing store in the worklist.
   const int kProcessPromotionListThreshold = kPromotionListSegmentSize / 2;

src/heap/scavenger.h

@@ -14,6 +14,8 @@
 namespace v8 {
 namespace internal {
 
+class OneshotBarrier;
+
 static const int kCopiedListSegmentSize = 256;
 static const int kPromotionListSegmentSize = 256;
 
@@ -24,45 +26,6 @@ using PromotionList = Worklist<ObjectAndSize, kPromotionListSegmentSize>;
 
 class Scavenger {
  public:
-  class Barrier {
-   public:
-    Barrier() : tasks_(0), waiting_(0), done_(false) {}
-
-    void Start() {
-      base::LockGuard<base::Mutex> guard(&mutex_);
-      tasks_++;
-    }
-
-    void NotifyAll() {
-      base::LockGuard<base::Mutex> guard(&mutex_);
-      if (waiting_ > 0) condition_.NotifyAll();
-    }
-
-    void Wait() {
-      base::LockGuard<base::Mutex> guard(&mutex_);
-      waiting_++;
-      if (waiting_ == tasks_) {
-        done_ = true;
-        condition_.NotifyAll();
-      } else {
-        // Spurious wakeup is ok here.
-        condition_.Wait(&mutex_);
-      }
-      waiting_--;
-    }
-
-    void Reset() { done_ = false; }
-
-    bool Done() { return done_; }
-
-   private:
-    base::ConditionVariable condition_;
-    base::Mutex mutex_;
-    int tasks_;
-    int waiting_;
-    bool done_;
-  };
-
   Scavenger(Heap* heap, bool is_logging, CopiedList* copied_list,
             PromotionList* promotion_list, int task_id);
 
@@ -77,7 +40,7 @@ class Scavenger {
 
   // Processes remaining work (=objects) after single objects have been
   // manually scavenged using ScavengeObject or CheckAndScavengeObject.
-  void Process(Barrier* barrier = nullptr);
+  void Process(OneshotBarrier* barrier = nullptr);
 
   // Finalize the Scavenger. Needs to be called from the main thread.
   void Finalize();

src/v8.gyp

@@ -996,6 +996,7 @@
         'heap/array-buffer-tracker-inl.h',
         'heap/array-buffer-tracker.cc',
         'heap/array-buffer-tracker.h',
+        'heap/barrier.h',
         'heap/code-stats.cc',
         'heap/code-stats.h',
         'heap/concurrent-marking.cc',

test/unittests/BUILD.gn

@@ -102,6 +102,7 @@ v8_executable("unittests") {
     "counters-unittest.cc",
     "eh-frame-iterator-unittest.cc",
     "eh-frame-writer-unittest.cc",
+    "heap/barrier-unittest.cc",
     "heap/bitmap-unittest.cc",
     "heap/embedder-tracing-unittest.cc",
     "heap/gc-idle-time-handler-unittest.cc",

test/unittests/heap/barrier-unittest.cc

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/heap/barrier.h"
+#include "src/base/platform/platform.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace v8 {
+namespace internal {
+namespace heap {
+
+TEST(OneshotBarrier, InitializeNotDone) {
+  OneshotBarrier barrier;
+  EXPECT_FALSE(barrier.DoneForTesting());
+}
+
+TEST(OneshotBarrier, DoneAfterWait_Sequential) {
+  OneshotBarrier barrier;
+  barrier.Start();
+  barrier.Wait();
+  EXPECT_TRUE(barrier.DoneForTesting());
+}
+
+namespace {
+
+class ThreadWaitingOnBarrier final : public base::Thread {
+ public:
+  ThreadWaitingOnBarrier()
+      : base::Thread(Options("ThreadWaitingOnBarrier")), barrier_(nullptr) {}
+
+  void Initialize(OneshotBarrier* barrier) { barrier_ = barrier; }
+
+  void Run() final { barrier_->Wait(); }
+
+ private:
+  OneshotBarrier* barrier_;
+};
+
+}  // namespace
+
+TEST(OneshotBarrier, DoneAfterWait_Concurrent) {
+  const int kThreadCount = 2;
+  OneshotBarrier barrier;
+  ThreadWaitingOnBarrier threads[kThreadCount];
+  for (int i = 0; i < kThreadCount; i++) {
+    threads[i].Initialize(&barrier);
+    // All threads need to call Wait() to be done.
+    barrier.Start();
+  }
+  for (int i = 0; i < kThreadCount; i++) {
+    threads[i].Start();
+  }
+  for (int i = 0; i < kThreadCount; i++) {
+    threads[i].Join();
+  }
+  EXPECT_TRUE(barrier.DoneForTesting());
+}
+
+TEST(OneshotBarrier, EarlyFinish_Concurrent) {
+  const int kThreadCount = 2;
+  OneshotBarrier barrier;
+  ThreadWaitingOnBarrier threads[kThreadCount];
+  // Test that one thread that actually finishes processing work before other
+  // threads call Start() will move the barrier in Done state.
+  barrier.Start();
+  barrier.Wait();
+  EXPECT_TRUE(barrier.DoneForTesting());
+  for (int i = 0; i < kThreadCount; i++) {
+    threads[i].Initialize(&barrier);
+    // All threads need to call Wait() to be done.
+    barrier.Start();
+  }
+  for (int i = 0; i < kThreadCount; i++) {
+    threads[i].Start();
+  }
+  for (int i = 0; i < kThreadCount; i++) {
+    threads[i].Join();
+  }
+  EXPECT_TRUE(barrier.DoneForTesting());
+}
+
+namespace {
+
+class CountingThread final : public base::Thread {
+ public:
+  CountingThread(OneshotBarrier* barrier, base::Mutex* mutex, size_t* work)
+      : base::Thread(Options("CountingThread")),
+        barrier_(barrier),
+        mutex_(mutex),
+        work_(work),
+        processed_work_(0) {}
+
+  void Run() final {
+    do {
+      ProcessWork();
+    } while (!barrier_->Wait());
+    // Main thread is not processing work, so we need one last step.
+    ProcessWork();
+  }
+
+  size_t processed_work() const { return processed_work_; }
+
+ private:
+  void ProcessWork() {
+    base::LockGuard<base::Mutex> guard(mutex_);
+    processed_work_ += *work_;
+    *work_ = 0;
+  }
+
+  OneshotBarrier* const barrier_;
+  base::Mutex* const mutex_;
+  size_t* const work_;
+  size_t processed_work_;
+};
+
+}  // namespace
+
+TEST(OneshotBarrier, Processing_Concurrent) {
+  const size_t kWorkCounter = 173173;
+  OneshotBarrier barrier;
+  base::Mutex mutex;
+  size_t work = 0;
+  CountingThread counting_thread(&barrier, &mutex, &work);
+  barrier.Start();
+  barrier.Start();
+  EXPECT_FALSE(barrier.DoneForTesting());
+  counting_thread.Start();
+
+  for (size_t i = 0; i < kWorkCounter; i++) {
+    {
+      base::LockGuard<base::Mutex> guard(&mutex);
+      work++;
+    }
+    barrier.NotifyAll();
+  }
+  barrier.Wait();
+  counting_thread.Join();
+  EXPECT_TRUE(barrier.DoneForTesting());
+  EXPECT_EQ(kWorkCounter, counting_thread.processed_work());
+}
+
+}  // namespace heap
+}  // namespace internal
+}  // namespace v8

test/unittests/unittests.gyp

@@ -99,6 +99,7 @@
       'counters-unittest.cc',
       'eh-frame-iterator-unittest.cc',
       'eh-frame-writer-unittest.cc',
+      'heap/barrier-unittest.cc',
       'heap/bitmap-unittest.cc',
       'heap/embedder-tracing-unittest.cc',
       'heap/gc-idle-time-handler-unittest.cc',