Refactor Thread lifecycle interface, add Thread::Enter/ExitIsolate.

Prepares for multiple threads by further decoupling Thread from Isolate.

- Replace Isolate::SetCurrent with more explicit Thread::Enter/ExitIsolate.
- Lazily initialize Thread instances when entering an isolate, to avoid new API calls for embedders.

R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=44835

[koda, 2015-03-30 16:13:51]

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

[siva, 2015-04-01 04:08:34]

LGTM.

I have one question regarding the EnterIsolate/ExitIsolate API Vs having just
one function SetCurrentIsolate.

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/include...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/include...
runtime/include/dart_api.h:952: * Requires there to be no current isolate.
Multiple threads may not be in
Maybe the comment should say
"Multiple threads may not execute dart code in the same isolate simultaneously"

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/lib/iso...
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/lib/iso...
runtime/lib/isolate.cc:167: char** error) {
Maybe assert here?

ASSERT(thread->isolate() == NULL);

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/lib/iso...
runtime/lib/isolate.cc:202: if (!CreateIsolate(parent_isolate, state, &error)) {
maybe 
Thread::EnterIsolate(parent_isolate);
here and not bother with all that setting in CreateIsolate.

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/lib/iso...
runtime/lib/isolate.cc:207: }
Thread::EnterIsolate(parent_isolate);

Seems odd that we are exiting the parent_isolate and entering it back again. Not
sure what the contract is for the isolate create callback, need to check if we
guarantee that the current isolate

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/vm/dart...
File runtime/vm/dart_api_impl.h (right):

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/vm/dart...
runtime/vm/dart_api_impl.h:297: }
shouldn't we just have an ASSERT in the constructor instead of this NULL check?

ASSERT(current_isolate != NULL);

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

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/vm/thre...
runtime/vm/thread.h:31: static void ExitIsolate();
Why did you choose the EnterIsolate/ExitIsolate model as opposed to having a
SetCurrentIsolate function?

[koda, 2015-04-01 16:05:59]

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/include...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/include...
runtime/include/dart_api.h:952: * Requires there to be no current isolate.
Multiple threads may not be in
On 2015/04/01 04:08:34, siva wrote:
> Maybe the comment should say
> "Multiple threads may not execute dart code in the same isolate
simultaneously"

But we (currently) do forbid them to even just enter isolates simultaneously,
even if they never actually invoke anything. If we want to relax the restriction
in the future, we can then update the comment and provide an exact definition of
what "executing dart code" means.

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/lib/iso...
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/lib/iso...
runtime/lib/isolate.cc:167: char** error) {
On 2015/04/01 04:08:34, siva wrote:
> Maybe assert here?
> 
> ASSERT(thread->isolate() == NULL);

Already done by Thread::EnterIsolate (that's how it differs from the old
Isolate::SetCurrent).

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/lib/iso...
runtime/lib/isolate.cc:202: if (!CreateIsolate(parent_isolate, state, &error)) {
On 2015/04/01 04:08:34, siva wrote:
> maybe 
> Thread::EnterIsolate(parent_isolate);
> here and not bother with all that setting in CreateIsolate.

Done.

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/lib/iso...
runtime/lib/isolate.cc:207: }
On 2015/04/01 04:08:34, siva wrote:
> Thread::EnterIsolate(parent_isolate);
> 
> Seems odd that we are exiting the parent_isolate and entering it back again.
Not
> sure what the contract is for the isolate create callback, need to check if we
> guarantee that the current isolate 

This stems from the spec of the API method Dart_CreateIsolate: it enters the
newly created isolate, and requires there to be no current isolate when called.
And the spec for the callback says it's expected to call Dart_CreateIsolate, so
it's natural that the same pre/post-conditions apply (although we could be more
explicit about it).

I agree that the current interface is awkward in this case. We could add a flag
to Dart_CreateIsolate that inhibits entering the isolate.

But, since this is meant to be a mostly refactoring CL, I'd rather save changes
to the external API spec for later.

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/vm/dart...
File runtime/vm/dart_api_impl.h (right):

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/vm/dart...
runtime/vm/dart_api_impl.h:297: }
On 2015/04/01 04:08:34, siva wrote:
> shouldn't we just have an ASSERT in the constructor instead of this NULL
check?
> 
> ASSERT(current_isolate != NULL);

Yes, we should, but our tests currently fail such an ASSERT.

The TODO is to update them and/or document the requirement that there be a
current isolate.

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

https://chromiumcodereview.appspot.com/1041523002/diff/200001/runtime/vm/thre...
runtime/vm/thread.h:31: static void ExitIsolate();
On 2015/04/01 04:08:34, siva wrote:
> Why did you choose the EnterIsolate/ExitIsolate model as opposed to having a
> SetCurrentIsolate function?

Enter/Exit clarifies the intent of the caller, and lets us add assertions to
that effect. Special values like NULL do make for compact code, but it's
impossible to know at runtime whether a passed in NULL was intentional or not.

In general, I think it's a good rule that the caller should be aware of whether
they are currently in an isolate.

Also, I moved the methods to Thread, since I believe the concept/phrasing "a
thread operates in/on an isolate" generalizes better than the old "isolate runs
on a thread".

[koda, 2015-04-01 17:48:26]

Committed patchset #12 (id:220001) manually as r44835 (presubmit successful).