Add test of full GC on helper thread

runtime/vm/thread.cc

Index: runtime/vm/thread.cc
diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc
index 3b3eb3308d30b292147168449f670ba1f62033f5..0ba005b917f7508aec006bbd4c9b991fb9e2a760 100644
--- a/runtime/vm/thread.cc
+++ b/runtime/vm/thread.cc
@@ -127,7 +127,7 @@ void Thread::EnterIsolate(Isolate* isolate) {
   isolate->MakeCurrentThreadMutator(thread);
   isolate->set_vm_tag(VMTag::kVMTagId);
   ASSERT(thread->store_buffer_block_ == NULL);
-  thread->store_buffer_block_ = isolate->store_buffer()->PopNonFullBlock();
+  thread->StoreBufferAcquire();
   ASSERT(isolate->heap() != NULL);
   thread->heap_ = isolate->heap();
   thread->Schedule(isolate);
@@ -153,9 +153,8 @@ void Thread::ExitIsolate() {
   Profiler::EndExecution(isolate);
   thread->set_thread_state(NULL);
   thread->Unschedule();
-  StoreBufferBlock* block = thread->store_buffer_block_;
-  thread->store_buffer_block_ = NULL;
-  isolate->store_buffer()->PushBlock(block);
+  // TODO(koda): Move store_buffer_block_ into State.
+  thread->StoreBufferRelease();
   if (isolate->is_runnable()) {
     isolate->set_vm_tag(VMTag::kIdleTagId);
   } else {
@@ -173,6 +172,8 @@ void Thread::EnterIsolateAsHelper(Isolate* isolate) {
   ASSERT(thread != NULL);
   ASSERT(thread->isolate() == NULL);
   thread->isolate_ = isolate;
+  ASSERT(thread->store_buffer_block_ == NULL);
+  thread->StoreBufferAcquire();
   ASSERT(isolate->heap() != NULL);
   thread->heap_ = isolate->heap();
   ASSERT(thread->thread_state() == NULL);
@@ -185,12 +186,11 @@ void Thread::EnterIsolateAsHelper(Isolate* isolate) {
 
 void Thread::ExitIsolateAsHelper() {
   Thread* thread = Thread::Current();
-  // If the helper thread chose to use the store buffer, check that it has
-  // already been flushed manually.
-  ASSERT(thread->store_buffer_block_ == NULL);
   Isolate* isolate = thread->isolate();
   ASSERT(isolate != NULL);
   thread->Unschedule();
+  // TODO(koda): Move store_buffer_block_ into State.
+  thread->StoreBufferRelease();
   thread->set_thread_state(NULL);
   thread->isolate_ = NULL;
   thread->heap_ = NULL;
@@ -200,21 +200,16 @@ void Thread::ExitIsolateAsHelper() {
 
 void Thread::PrepareForGC() {

[Ivan Posva, 2015/08/18 00:28:00]

Is there a reason this is a static method on the thread? Ditto for others such
as EnterIsolateAsHelper, ...

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

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.)

   Thread* thread = Thread::Current();
-  StoreBuffer* sb = thread->isolate()->store_buffer();
-  StoreBufferBlock* block = thread->store_buffer_block_;
-  thread->store_buffer_block_ = NULL;
-  const bool kCheckThreshold = false;  // Prevent scheduling another GC.
-  sb->PushBlock(block, kCheckThreshold);
-  thread->store_buffer_block_ = sb->PopEmptyBlock();
+  const bool kDoNotCheckThreshold = false;  // Prevent scheduling another GC.
+  thread->StoreBufferRelease(/* check_threshold = */ kDoNotCheckThreshold);

[Ivan Posva, 2015/08/18 00:28:00]

Please remove the comment so it reads more naturally. Store buffer release, do
not check threshold.

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

Done.

+  thread->store_buffer_block_ =
+      thread->isolate()->store_buffer()->PopEmptyBlock();

[Ivan Posva, 2015/08/18 00:28:00]

StoreBufferAcquire?

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

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.

 }
 
 
 void Thread::StoreBufferBlockProcess(bool check_threshold) {
-  StoreBuffer* sb = isolate()->store_buffer();
-  StoreBufferBlock* block = store_buffer_block_;
-  store_buffer_block_ = NULL;
-  sb->PushBlock(block, check_threshold);
-  store_buffer_block_ = sb->PopNonFullBlock();
+  StoreBufferRelease(check_threshold);
+  store_buffer_block_ = isolate()->store_buffer()->PopNonFullBlock();

[Ivan Posva, 2015/08/18 00:28:00]

ditto: StoreBufferAcquire.

[koda, 2015/08/18 14:21:34]

Done.

 }
 
 
@@ -234,6 +229,18 @@ void Thread::StoreBufferAddObjectGC(RawObject* obj) {
 }
 
 
+void Thread::StoreBufferRelease(bool check_threshold) {
+  StoreBufferBlock* block = store_buffer_block_;
+  store_buffer_block_ = NULL;
+  isolate_->store_buffer()->PushBlock(block, check_threshold);
+}
+
+
+void Thread::StoreBufferAcquire() {
+  store_buffer_block_ = isolate()->store_buffer()->PopNonFullBlock();
+}
+
+
 CHA* Thread::cha() const {
   ASSERT(isolate_ != NULL);
   return isolate_->cha_;