Split Thread/Isolate instances, but still maintain 1:1 relationship.

Split Thread/Isolate instances, but still maintain 1:1 relationship.

The life cycle of Thread is still coupled with the life cycle of Isolate, but the added indirection allows the migration work to proceed, at negligible performance impact.

This is a small step towards making Thread different from Isolate and allow a many-to-one relationship. In particular, in prepares for being able to have the sweeper have some minimal separate state from the main thread (eliminating the current fake-isolate hack).

Concretely, it moves the thread-local storage key into the Thread class, but for now, keeps a "main_thread" in Isolate for quick access in native/runtime_entry.
Also moves cha_ field to Thread (the first of many fields that will move).

To summarize, this CL enables the next steps:
- moving other fields from Isolate to Thread without circular include-dependencies
- replacing sweeper's fake Isolate with a helper Thread (referencing the original Isolate)

Also add 'volatile' to local variables that clang thinks could be clobbered.

R=asiva@google.com

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

[siva, 2015-03-25 21:26:34]

LGTM.

As discussed offline we need to make the Thread structure associated to the
underlying thread as the next step.

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

https://codereview.chromium.org/1037453002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:181: Isolate* volatile isolate = Isolate::Current();
What is the rationale for adding the 'volatile' keyword?

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

https://codereview.chromium.org/1037453002/diff/100001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:172: Thread* main_thread() {
The name 'main_thread' is confusing because we already implicitly refer to the
first thread in the process as main thread.

How about renaming it as the dart_execution_thread / mutator_thread

[koda, 2015-03-25 21:49:48]

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

https://codereview.chromium.org/1037453002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:181: Isolate* volatile isolate = Isolate::Current();
On 2015/03/25 21:26:34, siva wrote:
> What is the rationale for adding the 'volatile' keyword?

Without it, some compilers issue a warning about this local possibly being
clobbered between the setjmp/longjmp. However, I believe this warning is
unfounded, since the spec says this only happens if the local is actually
modified between the setjmp and longjmp, which is not the case here.

We already have a similar issue in flow_graph_inliner.cc:
https://github.com/dart-lang/bleeding_edge/blob/master/dart/runtime/vm/flow_g...

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

https://codereview.chromium.org/1037453002/diff/100001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:172: Thread* main_thread() {
On 2015/03/25 21:26:34, siva wrote:
> The name 'main_thread' is confusing because we already implicitly refer to the
> first thread in the process as main thread.
> 
> How about renaming it as the dart_execution_thread / mutator_thread

Renamed to mutator_thread.

[koda, 2015-03-25 22:41:44]

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

[Ivan Posva, 2015-03-26 14:36:52]

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

[Ivan Posva, 2015-03-26 14:36:53]

Thanks,
-Ivan

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

https://codereview.chromium.org/1037453002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:181: Isolate* volatile isolate = Isolate::Current();
On 2015/03/25 21:49:47, koda wrote:
> On 2015/03/25 21:26:34, siva wrote:
> > What is the rationale for adding the 'volatile' keyword?
> 
> Without it, some compilers issue a warning about this local possibly being
> clobbered between the setjmp/longjmp. However, I believe this warning is
> unfounded, since the spec says this only happens if the local is actually
> modified between the setjmp and longjmp, which is not the case here.
> 
> We already have a similar issue in flow_graph_inliner.cc:
>
https://github.com/dart-lang/bleeding_edge/blob/master/dart/runtime/vm/flow_g...

Please make this local a const, which it really is. That should remove the
warnings without the unclear meaning of volatile here.

https://codereview.chromium.org/1037453002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:282: Isolate* volatile isolate = Isolate::Current();
ditto

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

https://codereview.chromium.org/1037453002/diff/100001/runtime/vm/thread.h#ne...
runtime/vm/thread.h:45: static ThreadLocalKey thread_key;
thread_key_