Implement safepointing of threads

Implement safepointing of threads :
 - uses thread execution status transition to track when a thread is in a safepoint or needs to block for a safepoint
 - introduces a monitor per Thread object
 - uses a per thread safepoint handshake between the thread requesting a safepoint and the requested thread.

The ThreadRegistry class now contains only the thread list for an isolate and the functionality of scheduling a thread onto an Isolate and unscheduling it from teh isolate. We could fold this functionality into the Isolate class in a different CL.

R=rmacnak@google.com, zra@google.com

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

[siva, 2015-12-22 22:11:53]

Description was changed from

==========
Fixed long line issue.


Implement safepointing of threads :
 - uses thread execution status transition to track when a thread is in a
safepoint or needs to block for a safepoint
 - introduces a monitor per Thread object  - uses a per thread safepoint
handshake between the thread requesting a safepoint and the requested thread.
==========

to

==========
Implement safepointing of threads :
 - uses thread execution status transition to track when a thread is in a
safepoint or needs to block for a safepoint
 - introduces a monitor per Thread object  - uses a per thread safepoint
handshake between the thread requesting a safepoint and the requested thread.
==========

[siva, 2015-12-22 23:54:25]

Description was changed from

==========
Implement safepointing of threads :
 - uses thread execution status transition to track when a thread is in a
safepoint or needs to block for a safepoint
 - introduces a monitor per Thread object  - uses a per thread safepoint
handshake between the thread requesting a safepoint and the requested thread.
==========

to

==========
Implement safepointing of threads :
 - uses thread execution status transition to track when a thread is in a
safepoint or needs to block for a safepoint
 - introduces a monitor per Thread object  - uses a per thread safepoint
handshake between the thread requesting a safepoint and the requested thread.

The ThreadRegistry class now contains only the thread list for an isolate and
the functionality of scheduling a thread onto an Isolate and unscheduling it
from teh isolate. We could fold this functionality into the Isolate class in a
different CL.
==========

[siva, 2016-01-04 20:46:34]

Description was changed from

==========
Implement safepointing of threads :
 - uses thread execution status transition to track when a thread is in a
safepoint or needs to block for a safepoint
 - introduces a monitor per Thread object  - uses a per thread safepoint
handshake between the thread requesting a safepoint and the requested thread.

The ThreadRegistry class now contains only the thread list for an isolate and
the functionality of scheduling a thread onto an Isolate and unscheduling it
from teh isolate. We could fold this functionality into the Isolate class in a
different CL.
==========

to

==========
Implement safepointing of threads :
 - uses thread execution status transition to track when a thread is in a
safepoint or needs to block for a safepoint
 - introduces a monitor per Thread object  
 - uses a per thread safepoint handshake between the thread requesting a
safepoint and the requested thread.

The ThreadRegistry class now contains only the thread list for an isolate and
the functionality of scheduling a thread onto an Isolate and unscheduling it
from teh isolate. We could fold this functionality into the Isolate class in a
different CL.
==========

[siva, 2016-01-08 01:53:52]

asiva@google.com changed reviewers:
+ iposva@google.com, rmacnak@google.com, zra@google.com

[siva, 2016-01-08 01:53:53]

(The CL looks big but most of the files have single line changes to track
execution state transitions)

[rmacnak, 2016-01-08 21:51:17]

https://chromiumcodereview.appspot.com/1541073002/diff/220001/runtime/vm/thre...
File runtime/vm/thread.h (right):

https://chromiumcodereview.appspot.com/1541073002/diff/220001/runtime/vm/thre...
runtime/vm/thread.h:576: uint32_t safepoint_state_;
Check this in NoSafepointScope.

[zra, 2016-01-08 23:32:07]

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1551: Thread* thread = Thread::Current();
It looks like TransitionNativeToVM transition(thread) and DARTSCOPE manage the
thread state and safepoints automagically, but sometimes they're managed
manually, like here.

In places where they're managed manually, and there isn't a matching enter/exit
safepoint or set/unset thread state nearby, it would be helpful to know where
the matching calls are.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/gc_sweeper.cc
File runtime/vm/gc_sweeper.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/gc_sweeper....
runtime/vm/gc_sweeper.cc:137: TransitionVMToBlocked transition(thread);
I think I'm a little confused by the meaning of the Blocked state. We
potentially block every time we try to take a lock, or Wait on a monitor, but we
don't always put the thread into the Blocked state.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/heap.cc#new...
runtime/vm/heap.cc:98: // If we are in the process of running a sweep wait for
the sweeper to free
sweep, wait

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:2355: {
Extra { }

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/os_thread.h
File runtime/vm/os_thread.h (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/os_thread.h...
runtime/vm/os_thread.h:240: friend class Isolate;
Why?

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:810: // TODO(koda): Consider moving SafepointThreads into
allocation failure/retry
Remove TODO?

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/thread.h#ne...
runtime/vm/thread.h:577: uint32_t execution_state_;
Is it possible to check that this is correct in the stub on entry to generated
code?

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

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/thread_test...
runtime/vm/thread_test.cc:265: for (int i = 0; ; ++i) {
Could starting everyone at 0 here cause the threads to be artificially synced
up? Maybe i should start at something more random. Maybe:

intptr_t i = reinterpret_cast<intptr_t>(this)

or something.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/thread_test...
runtime/vm/thread_test.cc:328: const intptr_t SafepointTestTask::kTaskCount = 5;
Is the test too slow if there are more threads? I'd think this needs to be >
CPU_COUNT to be better at exposing unfortunate schedules.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/unit_test.h...
runtime/vm/unit_test.h:46: // The TEST_CASE macro is used for tests that need an
isolate and zone
This is the same comments that's on VM_TEST_CASE. Maybe expand on the comments
to explain where they should be used.

[siva, 2016-01-12 21:26:22]

- Addressed review comments
- Synched upto TOT
- --background_compilation --optimization_counter_threshold=10 is able to run
dart2js compiling itself (benchmarks/Dart2JSBase/dart/dummy_compiler_test.dart)
consistently without any asserts or hangs.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1551: Thread* thread = Thread::Current();
On 2016/01/08 23:32:07, zra wrote:
> It looks like TransitionNativeToVM transition(thread) and DARTSCOPE manage the
> thread state and safepoints automagically, but sometimes they're managed
> manually, like here.
> 
> In places where they're managed manually, and there isn't a matching
enter/exit
> safepoint or set/unset thread state nearby, it would be helpful to know where
> the matching calls are.

DARTSCOPE has a TransitionNativeToVM in it which transitions the state on entry
and restores it back on exit. This works nicely for most calls but EnterIsolate
and ExitIsolate are special as the Thread is unscheduled on exit and we might
get scheduled on a different Thread structure on EnterIsolate so using a Stack
allocated object like TransitionNativeToVM would not work in that case and hence
the explicit code that transitions.

I will add a comment to make this clear.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/gc_sweeper.cc
File runtime/vm/gc_sweeper.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/gc_sweeper....
runtime/vm/gc_sweeper.cc:137: TransitionVMToBlocked transition(thread);
On 2016/01/08 23:32:07, zra wrote:
> I think I'm a little confused by the meaning of the Blocked state. We
> potentially block every time we try to take a lock, or Wait on a monitor, but
we
> don't always put the thread into the Blocked state.

True this was a hack I did initially to get things going and forgot to chenge.
Transition to Blocked state is used only when we go into WaitWithSafepointCheck

I have added thread->CheckForSafepoint which I use here to see if the thread
needs wait for a safepoint operation.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/heap.cc#new...
runtime/vm/heap.cc:98: // If we are in the process of running a sweep wait for
the sweeper to free
On 2016/01/08 23:32:07, zra wrote:
> sweep, wait

Done.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:2355: {
On 2016/01/08 23:32:07, zra wrote:
> Extra { }

Fixed this code by removing the extra { }

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/os_thread.h
File runtime/vm/os_thread.h (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/os_thread.h...
runtime/vm/os_thread.h:240: friend class Isolate;
On 2016/01/08 23:32:07, zra wrote:
> Why?

To access set_thread which is a private method, it is a hack that was added to
solve a Windows thread interrupter problem, see comment above.
Added comment here too.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/pages.cc#ne...
runtime/vm/pages.cc:810: // TODO(koda): Consider moving SafepointThreads into
allocation failure/retry
On 2016/01/08 23:32:07, zra wrote:
> Remove TODO?

Done.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/thread.h#ne...
runtime/vm/thread.h:576: uint32_t safepoint_state_;
On 2016/01/08 21:51:17, rmacnak wrote:
> Check this in NoSafepointScope.

I have added a check in SafepointOperationScope to make sure that a new
SafepointOperationScope is not started inside a NoSafepointState.

Not sure it it makes sense to check !thread->IsAtSafepoint inside
NoSafepointScope as the thread might already be at a safepoint and then execute
code that is scoped inside NoSafepointScope.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/thread.h#ne...
runtime/vm/thread.h:577: uint32_t execution_state_;
On 2016/01/08 23:32:07, zra wrote:
> Is it possible to check that this is correct in the stub on entry to generated
> code?

Added a note to myself for this. I do not want to expand this CL as it is
already too big. I will add the check in another CL.

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

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/thread_test...
runtime/vm/thread_test.cc:265: for (int i = 0; ; ++i) {
On 2016/01/08 23:32:07, zra wrote:
> Could starting everyone at 0 here cause the threads to be artificially synced
> up? Maybe i should start at something more random. Maybe:
> 
> intptr_t i = reinterpret_cast<intptr_t>(this)
> 
> or something.

Done, used Thread::Current instead of this.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/thread_test...
runtime/vm/thread_test.cc:328: const intptr_t SafepointTestTask::kTaskCount = 5;
On 2016/01/08 23:32:07, zra wrote:
> Is the test too slow if there are more threads? I'd think this needs to be >
> CPU_COUNT to be better at exposing unfortunate schedules.

Not sure how slow it make the thread. This test is a little contrived and works
more as a unit test. I plan on adding an option called SafepointALot which will
have threads triggering safepoint operations at almost all transition points and
one could run say dart2js or some such real program using this flag and induce
threads to stop at various points.

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1541073002/diff/220001/runtime/vm/unit_test.h...
runtime/vm/unit_test.h:46: // The TEST_CASE macro is used for tests that need an
isolate and zone
On 2016/01/08 23:32:07, zra wrote:
> This is the same comments that's on VM_TEST_CASE. Maybe expand on the comments
> to explain where they should be used.

Fixed the comments.

[zra, 2016-01-13 22:16:13]

lgtm with questions/comments

If I fail to do a TransitionXXXToYYY somewhere, I wonder if it'll be hard to
figure out. I see there are ASSERTs in the ctors and dtors for these scopes, so
that I'll get an ASSERT failure on the *next* transition, but I wonder if
there's enough info in the ASSERT failure message to be able to figure out what
to do. I'm guessing you have some experience with this since you've put these
scopes all over, and if so maybe recording that wisdom somewhere would be good.

If I'm adding new code, what will go wrong if I fail to do a safepoint check
somewhere where I should?

https://codereview.chromium.org/1541073002/diff/240001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1541073002/diff/240001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:360: static const char* GetErrorString(Thread*
thread, const Object& obj) {
Does calling this require being in a DARTSCOPE()? If so, please add a comment
here.

https://codereview.chromium.org/1541073002/diff/240001/runtime/vm/safepoint.h
File runtime/vm/safepoint.h (right):

https://codereview.chromium.org/1541073002/diff/240001/runtime/vm/safepoint.h...
runtime/vm/safepoint.h:113: ~TransitionSafepointState() {}
Does this destructor need to be virtual?

[siva, 2016-01-27 19:07:51]

The transitions are documented in safepoint.h and I have ensured that the VM
code has transitions at all the points. If there is a missing transition the
Asserts will complain about the state being invalid.

The unit test code we have is a different story as we don't seem to have a
consistent way of writing test code, we have code that uses the Dart API in one
line and then immediately switches to calling into the VM directly. Most of the
file changes were in trying to straighten out the unit test code.

https://codereview.chromium.org/1541073002/diff/240001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1541073002/diff/240001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:360: static const char* GetErrorString(Thread*
thread, const Object& obj) {
On 2016/01/13 22:16:13, zra wrote:
> Does calling this require being in a DARTSCOPE()? If so, please add a comment
> here.

Added a comment that this function assumes an Api scope exists and added a
assert to ensure that.

https://codereview.chromium.org/1541073002/diff/240001/runtime/vm/safepoint.h
File runtime/vm/safepoint.h (right):

https://codereview.chromium.org/1541073002/diff/240001/runtime/vm/safepoint.h...
runtime/vm/safepoint.h:113: ~TransitionSafepointState() {}
On 2016/01/13 22:16:13, zra wrote:
> Does this destructor need to be virtual?

There are no virtual methods in any of the subclasses here and I don't see why
the destructor would need to be virtual.

[rmacnak, 2016-01-27 21:42:32]

lgtm

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

https://codereview.chromium.org/1541073002/diff/260001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:698: Thread* thread, bool is_mutator, bool bypass_sfepoint
= false);
safepoint

[siva, 2016-01-28 18:22:46]

Ivan should I wait for your review?

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

https://codereview.chromium.org/1541073002/diff/260001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:698: Thread* thread, bool is_mutator, bool bypass_sfepoint
= false);
On 2016/01/27 21:42:32, rmacnak wrote:
> safepoint

Done.

[siva, 2016-02-01 18:57:37]

Description was changed from

==========
Implement safepointing of threads :
 - uses thread execution status transition to track when a thread is in a
safepoint or needs to block for a safepoint
 - introduces a monitor per Thread object  
 - uses a per thread safepoint handshake between the thread requesting a
safepoint and the requested thread.

The ThreadRegistry class now contains only the thread list for an isolate and
the functionality of scheduling a thread onto an Isolate and unscheduling it
from teh isolate. We could fold this functionality into the Isolate class in a
different CL.
==========

to

==========
Implement safepointing of threads :
 - uses thread execution status transition to track when a thread is in a
safepoint or needs to block for a safepoint
 - introduces a monitor per Thread object
 - uses a per thread safepoint handshake between the thread requesting a
safepoint and the requested thread.

The ThreadRegistry class now contains only the thread list for an isolate and
the functionality of scheduling a thread onto an Isolate and unscheduling it
from teh isolate. We could fold this functionality into the Isolate class in a
different CL.

R=rmacnak@google.com, zra@google.com

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

[siva, 2016-02-01 18:57:39]

Committed patchset #15 (id:280001) manually as
e72c1fb47d311a79df5fc608c76ffe6bd13c86e6 (presubmit successful).