Safepoint interface and unit tests.

Safepoint interface and unit tests.

Add infrastructure to enable all threads in an isolate to rendezvous at GC-safe points, and exercise it through unit tests.

* Use existing interrupt mechanism for Dart threads, but a cooperative approach for non-Dart threads (concurrent compiler, etc.): they must periodically call CheckSafepoint (may block).

* Add unit tests to exercise and verify various scenarios: organizing the rendezvous from helper/main thread, with/without Dart code executing, etc.

The next step is to use this interface in old-space allocation and garbage collection, respectively, to allow helper threads to perform allocations that potentially trigger GC (needed for concurrent compiler).

Limitations:

* A thread that is executing a long-running native function is currently not considered at a safepoint, and will delay the rendezvous until it returns to Dart or indirectly calls CheckSafepoint. In a future CL, we can use the saved stack pointer to detect and support this case, and intercept the thread if/when it re-enters Dart or the VM.



BUG=
R=iposva@google.com

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

[koda, 2015-07-29 15:23:50]

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

[Ivan Posva, 2015-07-31 20:28:10]

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/code_generat...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1304: isolate->thread_registry()->CheckSafepoint();
Regardless of the interrupt reason we should check for pending safepoint
rendezvous. Before throwing stack overflow exceptions and other code which can
consume an arbitrary time.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:29:
isolate->ScheduleInterrupts(Isolate::kSafepointInterrrupt);
Talk like a pirate constants?

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:57: monitor_->NotifyAll();
Having to use NotifyAll generally points at some problematic assumptions in the
algorithm. There should be no reason to wake up the threads that just made it
into the Wait for the safepoint to be over.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.h (right):

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:26: // Bring all threads in this isolate, except
the caller, to a safepoint. The
Please add a comment that the caller is expected to be implicitly at a
safepoint.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:31: void SafepointAllThreads();
Isn't the All redundant here?

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:82: // Exiting an isolate must always be a
safepoint.
As already discussed I strongly disagree with this point of view.

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

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:322: const intptr_t num_tasks =
SafepointTestTask::kTaskCount;  // Please compiler.
Comment does not parse.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:337: "  for (dummy = 0; dummy < 1234567890; ++dummy)
{\n"
How long does this test execute on simulated architectures?

[koda, 2015-07-31 22:40:44]

Also fixed unit test (done -> total_done + local_done) as discussed offline.

PTAL

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/code_generat...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1304: isolate->thread_registry()->CheckSafepoint();
On 2015/07/31 20:28:09, Ivan Posva wrote:
> Regardless of the interrupt reason we should check for pending safepoint
> rendezvous. Before throwing stack overflow exceptions and other code which can
> consume an arbitrary time.

Done by creating single VM interrupt for all these kinds of checks. The new
version also avoids scheduling redundant GC when the store buffer is no longer
overflowed after the CheckSafepoint.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:29:
isolate->ScheduleInterrupts(Isolate::kSafepointInterrrupt);
On 2015/07/31 20:28:09, Ivan Posva wrote:
> Talk like a pirate constants?

Rrrenamed to kVMInterrupt.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:57: monitor_->NotifyAll();
On 2015/07/31 20:28:09, Ivan Posva wrote:
> Having to use NotifyAll generally points at some problematic assumptions in
the
> algorithm. There should be no reason to wake up the threads that just made it
> into the Wait for the safepoint to be over.

I agree that it is inelegant that they all wake up, but I do not see any bugs.
The sync strategy will be revisited together with allowing threads to exit the
isolate without blocking.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.h (right):

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:26: // Bring all threads in this isolate, except
the caller, to a safepoint. The
On 2015/07/31 20:28:09, Ivan Posva wrote:
> Please add a comment that the caller is expected to be implicitly at a
> safepoint.

Done.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:31: void SafepointAllThreads();
On 2015/07/31 20:28:09, Ivan Posva wrote:
> Isn't the All redundant here?

Done.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:82: // Exiting an isolate must always be a
safepoint.
On 2015/07/31 20:28:09, Ivan Posva wrote:
> As already discussed I strongly disagree with this point of view.

You convinced me that this should change in a future CL.

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

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:322: const intptr_t num_tasks =
SafepointTestTask::kTaskCount;  // Please compiler.
On 2015/07/31 20:28:09, Ivan Posva wrote:
> Comment does not parse.

Linker was unhappy with the inlined static constant for some reason. Fixed now
by defining it outside the class declaration instead.

https://codereview.chromium.org/1259223005/diff/60001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:337: "  for (dummy = 0; dummy < 1234567890; ++dummy)
{\n"
On 2015/07/31 20:28:09, Ivan Posva wrote:
> How long does this test execute on simulated architectures?

Now using smaller loop count if USING_SIMULATOR.

debugsimarm runs in < 10 s.

[Ivan Posva, 2015-07-31 23:40:16]

https://codereview.chromium.org/1259223005/diff/80001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1259223005/diff/80001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:330: kVMInterrupt = 0x4,  // Internal VM checks:
safepoints, store buffers, etc.
Since we gave up on aligning comments, then please switch to two spaces between
end-of-code and //.

Or align comments with shorter message such as "Trigger VM internal checks."

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

https://codereview.chromium.org/1259223005/diff/80001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:279: // if this is the first time.
if -> If

[Ivan Posva, 2015-08-01 00:06:50]

Now with official LGTM -ip

[koda, 2015-08-01 02:53:19]

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

[zra, 2015-08-01 03:28:21]

zra@google.com changed reviewers:
+ zra@google.com

[zra, 2015-08-01 03:28:22]

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

https://codereview.chromium.org/1259223005/diff/100001/runtime/vm/thread_test...
runtime/vm/thread_test.cc:259: Zone* zone = thread->zone();
DBC

Extra indent.

[koda, 2015-08-01 06:06:52]

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

https://codereview.chromium.org/1259223005/diff/100001/runtime/vm/thread_test...
runtime/vm/thread_test.cc:259: Zone* zone = thread->zone();
On 2015/08/01 03:28:22, zra wrote:
> DBC
> 
> Extra indent.

Done in follow up CL.