- Add an OSThread structure which is the generic TLS structure for all C++

- Add an OSThread structure which is the generic TLS structure for all C++
  fields in a thread (i.e fields that are not Dart VM related)
- Split the Thread structure to be a pure Dart per thread structure and add
  a pointer to os_thread which points to the OSThread structure
- Change Schedule/UnSchedule to set the Dart Thread structure as the TLS of
  the thread when it is inside the Dart world and reset the TLS back to the
  OSThread strcuture when is exits the Dart World.
- Moved the stack_base and few stack size related functions to OSThread from Isolate

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

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

[siva, 2015-11-13 18:15:59]

Description was changed from

==========
- Add an OSThread structure which is the generic TLS structure for all C++
  fields in a thread (i.e fields that are not Dart VM related)
- Split the Thread structure to be a pure Dart per thread structure and add
  a pointer to os_thread which points to the OSThread structure
- Change Schedule/UnSchedule to set the Dart Thread structure as the TLS of
  the thread when it is inside the Dart world and reset the TLS back to the
  OSThread strcuture when is exits the Dart World.
==========

to

==========
- Add an OSThread structure which is the generic TLS structure for all C++
  fields in a thread (i.e fields that are not Dart VM related)
- Split the Thread structure to be a pure Dart per thread structure and add
  a pointer to os_thread which points to the OSThread structure
- Change Schedule/UnSchedule to set the Dart Thread structure as the TLS of
  the thread when it is inside the Dart world and reset the TLS back to the
  OSThread strcuture when is exits the Dart World.
- Moved the stack_base and few stack size related functions to OSThread from
Isolate
==========

[siva, 2015-11-13 22:00:26]

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

[Cutch, 2015-11-14 00:31:56]

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:41: ASSERT(Timeline::recorder() != NULL);
Replace the ASSERT with:
if (Timeline::recorder() != NULL) {
}

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:66: FATAL("Invalid call to
Thread::EnableThreadInterrupts()");
s/Thread/OSThread

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter_linux.cc (left):

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_linux.cc:28: Thread* thread = Thread::Current();
I think this should be OSThread and not Thread.

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter_linux.cc (right):

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_linux.cc:46: void
ThreadInterrupter::InterruptThread(OSThread* thread) {
This change needs to be made in os_thread_interrupter_*.cc

[siva, 2015-11-14 01:38:26]

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:41: ASSERT(Timeline::recorder() != NULL);
On 2015/11/14 00:31:56, Cutch wrote:
> Replace the ASSERT with:
> if (Timeline::recorder() != NULL) {
> }

Done.

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:66: FATAL("Invalid call to
Thread::EnableThreadInterrupts()");
On 2015/11/14 00:31:56, Cutch wrote:
> s/Thread/OSThread

Done.

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter_linux.cc (left):

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_linux.cc:28: Thread* thread = Thread::Current();
On 2015/11/14 00:31:56, Cutch wrote:
> I think this should be OSThread and not Thread.

Currently SampleThread in the profiler only seems to sample threads that are
associated with an isolate and that is why I retained the signature of
Profiler::SampleThread(Thread* thr, const InterruptedThreadState& state).

We could at a later stage extend this for all threads.

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter_linux.cc (right):

https://codereview.chromium.org/1439483003/diff/150001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_linux.cc:46: void
ThreadInterrupter::InterruptThread(OSThread* thread) {
On 2015/11/14 00:31:56, Cutch wrote:
> This change needs to be made in os_thread_interrupter_*.cc

Good catch.

Done.

[zra, 2015-11-16 20:12:28]

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:107: 
Does thread need to be deleted?

After that, I think we should also be able to assert here that
OSThread::thread_list_head_ == NULL.

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread_w...
File runtime/vm/os_thread_win.cc (right):

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread_w...
runtime/vm/os_thread_win.cc:130: return ::GetCurrentThreadId();
Now that the join_id is stored on the OSThread in TLS, we can make a more
reliable thread join on Windows by changing ThreadJoinId to be a HANDLE for the
thread. If you'd like, I can take a look at doing that and you can add a TODO
for me here or where the ThreadJoinId type is defined.

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread_w...
runtime/vm/os_thread_win.cc:139: // reclaimed, and joining will fail. This can
be avoided in a couple of ways.
Stuff after "and joining will fail." is stale after this CL.

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/thread.cc
File runtime/vm/thread.cc (left):

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/thread.cc#o...
runtime/vm/thread.cc:156: ThreadPruner pruner(this);
What prunes threads from the registry now?

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:424: ThreadJoinId join_id =
OSThread::GetCurrentThreadJoinId();
In the future on Windows we might want to ensure that
OSThread::GetCurrentThreadJoinId() is called only once, so here we'd want to do
something like 'OSThread::Current()->join_id();' to get the value cached from
when the thread started up.

[siva, 2015-11-17 20:52:25]

Addressed comments, PTAL.

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:107: 
On 2015/11/16 20:12:28, zra wrote:
> Does thread need to be deleted?
> 
> After that, I think we should also be able to assert here that
> OSThread::thread_list_head_ == NULL.

As discussed offline moved the deletion of TLS to shutdown and the last thread
that dies calls Cleanup which deletes the thread local key and the associated
monitor.

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread_w...
File runtime/vm/os_thread_win.cc (right):

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread_w...
runtime/vm/os_thread_win.cc:130: return ::GetCurrentThreadId();
On 2015/11/16 20:12:28, zra wrote:
> Now that the join_id is stored on the OSThread in TLS, we can make a more
> reliable thread join on Windows by changing ThreadJoinId to be a HANDLE for
the
> thread. If you'd like, I can take a look at doing that and you can add a TODO
> for me here or where the ThreadJoinId type is defined.

I have added a TODO(zra) here.

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/os_thread_w...
runtime/vm/os_thread_win.cc:139: // reclaimed, and joining will fail. This can
be avoided in a couple of ways.
On 2015/11/16 20:12:28, zra wrote:
> Stuff after "and joining will fail." is stale after this CL.

Will let you change this comment after you make the change of using the handle
as a join id, as it is possible that the handle maybe closed before the join id
is extracted.

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/thread.cc
File runtime/vm/thread.cc (left):

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/thread.cc#o...
runtime/vm/thread.cc:156: ThreadPruner pruner(this);
On 2015/11/16 20:12:28, zra wrote:
> What prunes threads from the registry now?

The thread registry destructor deletes any remaining VM thread objects in it's
list. The PruneThread method used to go over all isolates and remove them
because the same TLS was shared across isolates. This is not the case anymore,
each isolate gets it's own VM Thread object and hence when the isolate dies and
it's thread registry is destroyed we just delete the VM thread objects in it's
freelist.

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1439483003/diff/170001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:424: ThreadJoinId join_id =
OSThread::GetCurrentThreadJoinId();
On 2015/11/16 20:12:28, zra wrote:
> In the future on Windows we might want to ensure that
> OSThread::GetCurrentThreadJoinId() is called only once, so here we'd want to
do
> something like 'OSThread::Current()->join_id();' to get the value cached from
> when the thread started up.

Changed this to os_thread->Id() and os_thread->JoinId()

[Cutch, 2015-11-17 23:19:01]

https://codereview.chromium.org/1439483003/diff/190001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter_win.cc (right):

https://codereview.chromium.org/1439483003/diff/190001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_win.cc:58: Thread* thread = Thread::Current();
On Windows this is running on a different thread than we are interrupting so
Thread::Current() will *always* be NULL.

[siva, 2015-11-18 20:25:10]

Addressed windows profiler issue.

PTAL.

https://codereview.chromium.org/1439483003/diff/190001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter_win.cc (right):

https://codereview.chromium.org/1439483003/diff/190001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_win.cc:58: Thread* thread = Thread::Current();
On 2015/11/17 23:19:01, Cutch wrote:
> On Windows this is running on a different thread than we are interrupting so
> Thread::Current() will *always* be NULL.

Addressed this as discussed offline.

[Cutch, 2015-11-18 22:30:53]

lgtm

[zra, 2015-11-18 22:40:36]

lgtm

[Ivan Posva, 2015-11-19 06:23:24]

DBC -ip

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/dart_entry....
runtime/vm/dart_entry.cc:53: saved_stack_limit_ = isolate->saved_stack_limit();
Please add a TODO to move the stack_limit from Isolate to Thread.

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

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:802: // to move to the OSThread structure.
VM tags should absolutely move to the OSThread. Then we can set them even if a
OSThread is not yet associated with a Thread and running in native code.

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:46: delete timeline_block_lock_;
Who deletes name_? This also means that we have to strdup the name being passed
in.

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:64: }
} else if (old == 0) {

Should we FATAL out in all conditions or only in debug builds and otherwise just
limit to zero?

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/thread_regi...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/thread_regi...
runtime/vm/thread_registry.cc:173: while (iterator != NULL) {
iterator is a weird name for this. How about using current, which goes well with
previous?

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/thread_regi...
runtime/vm/thread_registry.cc:219: Thread* iterator = active_list_;
ditto

[Cutch, 2015-11-19 15:12:21]

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

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:802: // to move to the OSThread structure.
On 2015/11/19 06:23:23, Ivan Posva wrote:
> VM tags should absolutely move to the OSThread. Then we can set them even if a
> OSThread is not yet associated with a Thread and running in native code.

Agreed. The UserTag should be moved from Isolate to Thread (so we can better
support multiple mutators) and the VmTag should live on OSThread.

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:46: delete timeline_block_lock_;
On 2015/11/19 06:23:23, Ivan Posva wrote:
> Who deletes name_? This also means that we have to strdup the name being
passed
> in.

name_ must always be a statically allocated const char* and it isn't copied.

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:64: }
On 2015/11/19 06:23:23, Ivan Posva wrote:
> } else if (old == 0) {
> 
> Should we FATAL out in all conditions or only in debug builds and otherwise
just
> limit to zero?

I wrote this code so that it would FATAL in all configurations. The embedder or
the VM has a serious bug if they don't properly pair calls to
DisableThreadInterrupts with calls to EnableThreadInterrupts.

[siva, 2015-11-19 20:54:05]

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/dart_entry....
runtime/vm/dart_entry.cc:53: saved_stack_limit_ = isolate->saved_stack_limit();
On 2015/11/19 06:23:23, Ivan Posva wrote:
> Please add a TODO to move the stack_limit from Isolate to Thread.

Added a TODO in isolate.h near the stack_limit_ field declaration.

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

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:802: // to move to the OSThread structure.
On 2015/11/19 15:12:21, Cutch wrote:
> On 2015/11/19 06:23:23, Ivan Posva wrote:
> > VM tags should absolutely move to the OSThread. Then we can set them even if
a
> > OSThread is not yet associated with a Thread and running in native code.
> 
> Agreed. The UserTag should be moved from Isolate to Thread (so we can better
> support multiple mutators) and the VmTag should live on OSThread.

I totally agree that vm_tag should move, not for this CL, I will leave the TODO
here and make it more strong (ie. instead of maybe will state these tags needs
to move).

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/os_thread.cc
File runtime/vm/os_thread.cc (right):

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/os_thread.c...
runtime/vm/os_thread.cc:64: }
On 2015/11/19 15:12:21, Cutch wrote:
> On 2015/11/19 06:23:23, Ivan Posva wrote:
> > } else if (old == 0) {
> > 
> > Should we FATAL out in all conditions or only in debug builds and otherwise
> just
> > limit to zero?
> 
> I wrote this code so that it would FATAL in all configurations. The embedder
or
> the VM has a serious bug if they don't properly pair calls to
> DisableThreadInterrupts with calls to EnableThreadInterrupts.

I am going to leave it the way it is now, we can always revisit.

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/thread_regi...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/thread_regi...
runtime/vm/thread_registry.cc:173: while (iterator != NULL) {
On 2015/11/19 06:23:23, Ivan Posva wrote:
> iterator is a weird name for this. How about using current, which goes well
with
> previous?

Done.

https://codereview.chromium.org/1439483003/diff/210001/runtime/vm/thread_regi...
runtime/vm/thread_registry.cc:219: Thread* iterator = active_list_;
On 2015/11/19 06:23:23, Ivan Posva wrote:
> ditto

Done.

[siva, 2015-11-19 21:45:14]

Committed patchset #13 (id:230001) manually as
9e19d236ca83136d49cf54f38a8b551927be70bf (presubmit successful).