Add test of full GC on helper thread

Add unit test to ensure that we can execute a full GC on a thread other than the main mutator. (For now, the main mutator waits manually, since the GC does not yet safepoint.)

Adding this unit test ensures that we do not add any new code to the dependencies of the GC that assumes the current thread is the main mutator.

* Equip helper threads with a store buffer automatically, just like for the main thread.
* Share code by refactoring flushing.

BUG=
R=iposva@google.com

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

[koda, 2015-08-14 01:01:45]

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

[Ivan Posva, 2015-08-14 22:15:29]

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

https://codereview.chromium.org/1289153002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:203: const bool kCheckThreshold = false;  // Prevent
scheduling another GC.
Name is reverse from its meaning. Should be kDoNotCheckThreshold.

https://codereview.chromium.org/1289153002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:232: void Thread::StoreBufferFlush(bool check_threshold) {
Unused parameter "check_threshold".

How about StoreBufferRelease and adding a StoreBufferAquire?

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

https://codereview.chromium.org/1289153002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:497: // after/at safepointing.
// while reaching a safepoint.

[koda, 2015-08-17 15:51:07]

PTAL

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

https://codereview.chromium.org/1289153002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:203: const bool kCheckThreshold = false;  // Prevent
scheduling another GC.
On 2015/08/14 22:15:29, Ivan Posva wrote:
> Name is reverse from its meaning. Should be kDoNotCheckThreshold.

Done.

https://codereview.chromium.org/1289153002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:232: void Thread::StoreBufferFlush(bool check_threshold) {
On 2015/08/14 22:15:29, Ivan Posva wrote:
> Unused parameter "check_threshold".
> 
> How about StoreBufferRelease and adding a StoreBufferAquire?

Done.

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

https://codereview.chromium.org/1289153002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:497: // after/at safepointing.
On 2015/08/14 22:15:29, Ivan Posva wrote:
> // while reaching a safepoint.

Done.

[Ivan Posva, 2015-08-18 00:28:01]

LGTM with comments.

-Ivan

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

https://codereview.chromium.org/1289153002/diff/60001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:201: void Thread::PrepareForGC() {
Is there a reason this is a static method on the thread? Ditto for others such
as EnterIsolateAsHelper, ...

https://codereview.chromium.org/1289153002/diff/60001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:204: thread->StoreBufferRelease(/* check_threshold = */
kDoNotCheckThreshold);
Please remove the comment so it reads more naturally. Store buffer release, do
not check threshold.

https://codereview.chromium.org/1289153002/diff/60001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:206: thread->isolate()->store_buffer()->PopEmptyBlock();
StoreBufferAcquire?

https://codereview.chromium.org/1289153002/diff/60001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:212: store_buffer_block_ =
isolate()->store_buffer()->PopNonFullBlock();
ditto: StoreBufferAcquire.

[koda, 2015-08-18 14:21:35]

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

https://codereview.chromium.org/1289153002/diff/60001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:201: void Thread::PrepareForGC() {
On 2015/08/18 00:28:00, Ivan Posva wrote:
> Is there a reason this is a static method on the thread? Ditto for others such
> as EnterIsolateAsHelper, ...

It should become non-static and called from SafepointThreads, as I implied in
the TODO in the test.

Added comment.

(I originally made most public methods static to ensure they are not called from
one thread on another running (not safepointed) thread.)

https://codereview.chromium.org/1289153002/diff/60001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:204: thread->StoreBufferRelease(/* check_threshold = */
kDoNotCheckThreshold);
On 2015/08/18 00:28:00, Ivan Posva wrote:
> Please remove the comment so it reads more naturally. Store buffer release, do
> not check threshold.

Done.

https://codereview.chromium.org/1289153002/diff/60001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:206: thread->isolate()->store_buffer()->PopEmptyBlock();
On 2015/08/18 00:28:00, Ivan Posva wrote:
> StoreBufferAcquire?

That would not work, since we must make sure to get a completely empty block, so
that the isolate has all the entries when the GC starts. When we run all GCs on
new threads, with their own fresh store buffer blocks, it would be more natural
to leave the block as NULL here.

Added comment in the code.

https://codereview.chromium.org/1289153002/diff/60001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:212: store_buffer_block_ =
isolate()->store_buffer()->PopNonFullBlock();
On 2015/08/18 00:28:00, Ivan Posva wrote:
> ditto: StoreBufferAcquire.

Done.

[koda, 2015-08-18 14:23:21]

Committed patchset #5 (id:80001) manually as
655df891fc2d37f8ba67dd2374cf846a294473e9 (presubmit successful).