Old-gen marking on separate thread

runtime/vm/gc_marker.cc

 // Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/gc_marker.h"
 
 #include <map>
 #include <utility>
 #include <vector>
 
 #include "vm/allocation.h"
 #include "vm/dart_api_state.h"
 #include "vm/isolate.h"
 #include "vm/log.h"
 #include "vm/pages.h"
 #include "vm/raw_object.h"
 #include "vm/stack_frame.h"
 #include "vm/store_buffer.h"
 #include "vm/thread_pool.h"
 #include "vm/visitor.h"
 #include "vm/object_id_ring.h"
 
 namespace dart {
 
+DEFINE_FLAG(int, marker_tasks, 1,
+            "The number of tasks to spawn during old gen GC marking (0 means "
+            "perform all marking on main thread).");
+
 typedef StoreBufferBlock PointerBlock;  // TODO(koda): Rename to PointerBlock.
 typedef StoreBuffer MarkingStack;  // TODO(koda): Create shared base class.
 
 class DelaySet {
  private:
   typedef std::multimap<RawObject*, RawWeakProperty*> Map;
   typedef std::pair<RawObject*, RawWeakProperty*> MapEntry;
 
  public:
   DelaySet() : mutex_(new Mutex()) {}
@@ skipping 379 matching lines @@
   }
 }
 
 
 void GCMarker::IterateRoots(Isolate* isolate,
                             ObjectPointerVisitor* visitor,
                             bool visit_prologue_weak_persistent_handles) {
   isolate->VisitObjectPointers(visitor,
                                visit_prologue_weak_persistent_handles,
                                StackFrameIterator::kDontValidateFrames);
-  heap_->new_space()->VisitObjectPointers(visitor);
+  isolate->heap()->new_space()->VisitObjectPointers(visitor);
 }
 
 
 void GCMarker::IterateWeakRoots(Isolate* isolate,
                                 HandleVisitor* visitor,
                                 bool visit_prologue_weak_persistent_handles) {
   ApiState* state = isolate->api_state();
   ASSERT(state != NULL);
   isolate->VisitWeakPersistentHandles(visitor,
                                       visit_prologue_weak_persistent_handles);
@@ skipping 99 matching lines @@
 
 
 void GCMarker::ProcessObjectIdTable(Isolate* isolate) {
   ObjectIdRingClearPointerVisitor visitor(isolate);
   ObjectIdRing* ring = isolate->object_id_ring();
   ASSERT(ring != NULL);
   ring->VisitPointers(&visitor);
 }
 
 
+class MarkTask : public ThreadPool::Task {
+ public:
+  MarkTask(GCMarker* marker,
+           Isolate* isolate,
+           Heap* heap,
+           PageSpace* page_space,
+           MarkingStack* marking_stack,
+           DelaySet* delay_set,
+           bool collect_code,
+           bool invoke_api_callbacks)

[Ivan Posva, 2015/08/28 05:23:22]

ditto. see below

[koda, 2015/08/28 13:27:54]

Done.

+      : marker_(marker),
+        isolate_(isolate),
+        heap_(heap),
+        page_space_(page_space),
+        marking_stack_(marking_stack),
+        delay_set_(delay_set),
+        collect_code_(collect_code),
+        invoke_api_callbacks_(invoke_api_callbacks) {
+  }
+
+  virtual void Run() {
+    Thread::EnterIsolateAsHelper(isolate_, true);
+    {
+      StackZone stack_zone(Thread::Current());
+      Zone* zone = stack_zone.GetZone();
+      SkippedCodeFunctions* skipped_code_functions =
+        collect_code_ ? new(zone) SkippedCodeFunctions() : NULL;
+      MarkingVisitor visitor(isolate_, heap_, page_space_, marking_stack_,
+                             delay_set_, skipped_code_functions);
+      // Phase 1: Populate and drain marking stack in task.
+      // TODO(koda): Split root iteration work among multiple tasks.
+      GCMarker::IterateRoots(isolate_, &visitor, !invoke_api_callbacks_);
+      visitor.DrainMarkingStack();
+      marker_->TaskSync();
+      // Phase 2: Weak processing and follow-up marking on main thread.
+      marker_->TaskSync();
+      // Phase 3: Finalize results from all markers (detach code, etc.).
+      marker_->FinalizeResultsFrom(&visitor);
+    }
+    Thread::ExitIsolateAsHelper(true);
+    // This task is done. Notify the original thread.
+    marker_->TaskNotifyDone();
+  }
+
+ private:
+  GCMarker* marker_;
+  Isolate* isolate_;
+  Heap* heap_;
+  PageSpace* page_space_;
+  MarkingStack* marking_stack_;
+  DelaySet* delay_set_;
+  bool collect_code_;
+  bool invoke_api_callbacks_;
+
+  DISALLOW_COPY_AND_ASSIGN(MarkTask);
+};
+
+
+void GCMarker::MainSync(intptr_t num_tasks) {
+  MonitorLocker ml(&monitor_);
+  while (done_count_ < num_tasks) {
+    ml.Wait();
+  }
+  done_count_ = 0;  // Tasks may now resume.
+  // TODO(koda): Add barrier utility with two condition variables to allow for
+  // Notify rather than NotifyAll. Also use it for safepoints.
+  ml.NotifyAll();
+}
+
+
+void GCMarker::TaskNotifyDone() {
+  MonitorLocker ml(&monitor_);
+  ++done_count_;
+  // TODO(koda): Add barrier utility with two condition variables to allow for
+  // Notify rather than NotifyAll. Also use it for safepoints.
+  ml.NotifyAll();
+}
+
+
+void GCMarker::TaskSync() {
+  TaskNotifyDone();
+  MonitorLocker ml(&monitor_);

[Ivan Posva, 2015/08/28 05:23:23]

At the expense of duplicated code, but it has two benefits: Visualizes how the
single lock barrier really works, and avoids grabbing the lock multiple times.

void GCMarker::TaskSync() {
  MonitorLocker ml(&monitor_);
  ++done_count_;
  ml.NotifyAll();  // Notify controller that this thread reached end of phase.
  ASSERT(done_count_ > 0);
  while (done_count_ > 0) {  // Wait for the controller to release into next
phase.
    ml.Wait();
  }
}

[koda, 2015/08/28 13:27:54]

Done.

+  while (done_count_ > 0) {
+    ml.Wait();
+  }
+}
+
+
+void GCMarker::FinalizeResultsFrom(MarkingVisitor* visitor) {
+  {
+    MonitorLocker ml(&monitor_);
+    marked_bytes_ += visitor->marked_bytes();
+  }
+  visitor->Finalize();
+}
+
+
 void GCMarker::MarkObjects(Isolate* isolate,
                            PageSpace* page_space,
                            bool invoke_api_callbacks,
                            bool collect_code) {
   Prologue(isolate, invoke_api_callbacks);
   // The API prologue/epilogue may create/destroy zones, so we must not
   // depend on zone allocations surviving beyond the epilogue callback.
   {
     StackZone stack_zone(Thread::Current());
     Zone* zone = stack_zone.GetZone();
     MarkingStack marking_stack;
     DelaySet delay_set;
-    SkippedCodeFunctions* skipped_code_functions =
-        collect_code ? new(zone) SkippedCodeFunctions() : NULL;
-    MarkingVisitor mark(isolate, heap_, page_space, &marking_stack,
-                        &delay_set, skipped_code_functions);
-    IterateRoots(isolate, &mark, !invoke_api_callbacks);
-    mark.DrainMarkingStack();
-    IterateWeakReferences(isolate, &mark);
-    MarkingWeakVisitor mark_weak;
-    IterateWeakRoots(isolate, &mark_weak, invoke_api_callbacks);
-    // TODO(koda): Add hand-over callback.
-    marked_bytes_ = mark.marked_bytes();
-    mark.Finalize();
+    marked_bytes_ = 0;
+    const int num_tasks = FLAG_marker_tasks;
+    if (num_tasks == 0) {
+      // Mark everything on main thread.
+      SkippedCodeFunctions* skipped_code_functions =
+          collect_code ? new(zone) SkippedCodeFunctions() : NULL;
+      MarkingVisitor mark(isolate, heap_, page_space, &marking_stack,
+                          &delay_set, skipped_code_functions);
+      IterateRoots(isolate, &mark, !invoke_api_callbacks);
+      mark.DrainMarkingStack();
+      IterateWeakReferences(isolate, &mark);
+      MarkingWeakVisitor mark_weak;
+      IterateWeakRoots(isolate, &mark_weak, invoke_api_callbacks);
+      // All marking done; detach code, etc.
+      FinalizeResultsFrom(&mark);
+    } else {
+      if (num_tasks > 1) {
+        // TODO(koda): Support multiple:
+        // 1. non-concurrent tasks, after splitting root iteration work, then
+        // 2. concurrent tasks, after synchronizing headers.
+        FATAL("Multiple marking tasks not yet supported");
+      }
+      // Phase 1: Populate and drain marking stack in task.

[Ivan Posva, 2015/08/28 05:23:22]

How about:

bool visit_prologue_weak_persistent_handles = !invoke_api_callbacks;

And then pass visit_prologue_weak_persistent_handles as a parameter to the
MarkTask constructor to document the fact that you are not going to
invoke_api_callbacks on the spawned thread. This was a big source of confusion
and concern during this review.

[koda, 2015/08/28 13:27:54]

Done. Also updated the existing code in the num_tasks==0 case above to use
visit_prologue....

+      MarkTask* mark_task =
+          new MarkTask(this, isolate, heap_, page_space, &marking_stack,
+                       &delay_set, collect_code, invoke_api_callbacks);
+      ThreadPool* pool = Dart::thread_pool();
+      pool->Run(mark_task);
+      MainSync(num_tasks);
+      // Phase 2: Weak processing and follow-up marking on main thread.
+      SkippedCodeFunctions* skipped_code_functions =
+          collect_code ? new(zone) SkippedCodeFunctions() : NULL;
+      MarkingVisitor mark(isolate, heap_, page_space, &marking_stack,
+                          &delay_set, skipped_code_functions);
+      IterateWeakReferences(isolate, &mark);
+      MarkingWeakVisitor mark_weak;
+      IterateWeakRoots(isolate, &mark_weak, invoke_api_callbacks);

[Ivan Posva, 2015/08/28 05:23:22]

And here use !visit_prologue_weak_persistent_handles instead of
invoke_api_callbacks.

[koda, 2015/08/28 13:27:54]

Done.

+      MainSync(num_tasks);
+      // Phase 3: Finalize results from all markers (detach code, etc.).
+      FinalizeResultsFrom(&mark);
+      MainSync(num_tasks);
+      // Finalization complete and all tasks exited.
+    }
     delay_set.ClearReferences();
     ProcessWeakTables(page_space);
     ProcessObjectIdTable(isolate);
   }
   Epilogue(isolate, invoke_api_callbacks);
 }
 
 }  // namespace dart