Refactor delay set to allow more than one marking visitor

Move the delay set into its own class.

Prepare it for actual parallelism:
* add synchronization
* abstract out the header check/update (will become one CAS)

Use two separate marking visitors (sharing the delay set), with the main marking visitor in a separate thread (while mutator is paused), just to show we can.

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

[koda, 2015-06-11 01:06:31]

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

[koda, 2015-06-11 17:25:54]

Updated to run the main marker on a separate thread (just to show we can).

PTAL

[Ivan Posva, 2015-06-16 17:21:00]

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

https://codereview.chromium.org/1176183002/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:585: ml.NotifyAll();
Why NotifyAll()? Do you really intend to wakeup all threads that are waiting on
the number of tasks to change?

Also please add a comment explaining who is waiting for these notifications.

https://codereview.chromium.org/1176183002/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:597: ml.NotifyAll();
ditto

https://codereview.chromium.org/1176183002/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:624: for (int i = 0; i < kNumMarkers; ++i) {
It would be much more readable if the DrainTask would stack allocate its own
MarkingVisitor instead of trying to account for them here.

https://codereview.chromium.org/1176183002/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:654: delay_set.ClearAll();
ClearAll is ambiguous. How about ClearReferences()?

[koda, 2015-06-16 19:41:44]

PTAL

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

https://codereview.chromium.org/1176183002/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:585: ml.NotifyAll();
On 2015/06/16 17:21:00, Ivan Posva wrote:
> Why NotifyAll()? Do you really intend to wakeup all threads that are waiting
on
> the number of tasks to change?
> 
> Also please add a comment explaining who is waiting for these notifications.

Removed.

https://codereview.chromium.org/1176183002/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:597: ml.NotifyAll();
On 2015/06/16 17:21:00, Ivan Posva wrote:
> ditto

Changed to Notify.

https://codereview.chromium.org/1176183002/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:624: for (int i = 0; i < kNumMarkers; ++i) {
On 2015/06/16 17:21:00, Ivan Posva wrote:
> It would be much more readable if the DrainTask would stack allocate its own
> MarkingVisitor instead of trying to account for them here.

Yes, but ThreadPool deletes each Task after it has Run, so we'd need a callback
or other method to coordinate the detaching of code (kept in each visitor) after
*all* marking has completed. It can be done by the GCMarker (pull/push from/to
each visitor via callback), but I was saving this for a future CL. Note that
marked_words_ will need to be summed up in the GCMarker from each visitor (e.g.,
by pull or push via callback).

https://codereview.chromium.org/1176183002/diff/80001/runtime/vm/gc_marker.cc...
runtime/vm/gc_marker.cc:654: delay_set.ClearAll();
On 2015/06/16 17:21:00, Ivan Posva wrote:
> ClearAll is ambiguous. How about ClearReferences()?

Done.

[koda, 2015-06-19 19:58:29]

Committed patchset #8 (id:130001) manually as
12dddf1ced6fa07e477ea97c7695daff900a32ad (presubmit successful).

[koda, 2015-06-19 20:00:02]

Committed after removing the DrainTask, to be added back once the helper threads
have their own zone.