Old-gen marking on separate thread

- Add marking task running on separate thread.
- A task has its own MarkingVisitor and SkippedCodeFunctions.
- Weak processing still happens on main thread, with tasks paused.
- Finalization (detaching code) happens in task.
- Cmdline-flag "marker_tasks=0" reverts to old behavior.

BUG=

Committed: https://github.com/dart-lang/sdk/commit/db8e0e6423cfe842465e8397e6489fcd26deb4f2

[koda, 2015-08-27 22:57:08]

koda@google.com changed reviewers:
+ iposva@google.com

[koda, 2015-08-28 01:26:30]

Extended comments about the phases, to help the reader matching up the parallel
timelines in task and main thread.

Also, moved finalization of main thread marking visitor into the correct phase,
thus making it concurrent with the finalization in the task.

PTAL

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

https://codereview.chromium.org/1309033007/diff/40001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/1309033007/diff/40001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:428:
isolate->heap()->new_space()->VisitObjectPointers(visitor);
The GCMarker still has a heap_ reference, why has this changed?

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:557: bool invoke_api_callbacks)
ditto. see below

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:629: MonitorLocker ml(&monitor_);
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();
  }
}

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:679: // Phase 1: Populate and drain marking stack in
task.
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.

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:693: IterateWeakRoots(isolate, &mark_weak,
invoke_api_callbacks);
And here use !visit_prologue_weak_persistent_handles instead of
invoke_api_callbacks.

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

Addressed comments. Also reverted the concurrent finalization, since ISL_Print,
which is used by DetachCode when tracing is enabled, is not yet thread-safe
(added TODO).

https://codereview.chromium.org/1309033007/diff/40001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/1309033007/diff/40001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:428:
isolate->heap()->new_space()->VisitObjectPointers(visitor);
On 2015/08/28 05:23:22, Ivan Posva wrote:
> The GCMarker still has a heap_ reference, why has this changed?

Because I made this method static, since it doesn't really use any state of
GCMarker (and if it will, we need to ensure that access is thread-safe).

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:557: bool invoke_api_callbacks)
On 2015/08/28 05:23:22, Ivan Posva wrote:
> ditto. see below

Done.

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:629: MonitorLocker ml(&monitor_);
On 2015/08/28 05:23:23, Ivan Posva wrote:
> 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();
>   }
> }

Done.

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:679: // Phase 1: Populate and drain marking stack in
task.
On 2015/08/28 05:23:22, Ivan Posva wrote:
> 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.

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

https://codereview.chromium.org/1309033007/diff/60001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:693: IterateWeakRoots(isolate, &mark_weak,
invoke_api_callbacks);
On 2015/08/28 05:23:22, Ivan Posva wrote:
> And here use !visit_prologue_weak_persistent_handles instead of
> invoke_api_callbacks.

Done.

[Ivan Posva, 2015-08-28 16:06:22]

LGTMwC

-Ivan

https://codereview.chromium.org/1309033007/diff/80001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/1309033007/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:428:
isolate->heap()->new_space()->VisitObjectPointers(visitor);
heap_

[koda, 2015-08-28 16:09:09]

https://codereview.chromium.org/1309033007/diff/80001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/1309033007/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:428:
isolate->heap()->new_space()->VisitObjectPointers(visitor);
On 2015/08/28 16:06:22, Ivan Posva wrote:
> heap_

As I already replied above (on earlier patch set), the method is now static. Do
you suggest I make it non-static again?

[Ivan Posva, 2015-08-28 16:38:08]

-Ivan

https://codereview.chromium.org/1309033007/diff/80001/runtime/vm/gc_marker.cc
File runtime/vm/gc_marker.cc (right):

https://codereview.chromium.org/1309033007/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:428:
isolate->heap()->new_space()->VisitObjectPointers(visitor);
On 2015/08/28 16:09:09, koda wrote:
> On 2015/08/28 16:06:22, Ivan Posva wrote:
> > heap_
> 
> As I already replied above (on earlier patch set), the method is now static.
Do
> you suggest I make it non-static again?

Missed that comment, sorry. Then the question is, why is this static?

[koda, 2015-08-28 16:43:04]

On 2015/08/28 16:38:08, Ivan Posva wrote:
> -Ivan
> 
> https://codereview.chromium.org/1309033007/diff/80001/runtime/vm/gc_marker.cc
> File runtime/vm/gc_marker.cc (right):
> 
>
https://codereview.chromium.org/1309033007/diff/80001/runtime/vm/gc_marker.cc...
> runtime/vm/gc_marker.cc:428:
> isolate->heap()->new_space()->VisitObjectPointers(visitor);
> On 2015/08/28 16:09:09, koda wrote:
> > On 2015/08/28 16:06:22, Ivan Posva wrote:
> > > heap_
> > 
> > As I already replied above (on earlier patch set), the method is now static.
> Do
> > you suggest I make it non-static again?
> 
> Missed that comment, sorry. Then the question is, why is this static?

From above:
"since it doesn't really use any state of
GCMarker (and if it will, we need to ensure that access is thread-safe)."

But if you find it confusing, maybe others will too, so I've now reverted it.

[koda, 2015-08-28 17:00:09]

Committed patchset #6 (id:100001) manually as
db8e0e6423cfe842465e8397e6489fcd26deb4f2 (presubmit successful).