Add ThreadIterator for iterating over all Threads

Add ThreadIterator for iterating over all Threads

- Register all Threads in a central list.
- Introduce ThreadIterator to iterate over threads.

This will be used by the profiler when it switches from iterating over isolates to threads. It will also be used to simplify the timeline backend.

R=asiva@google.com

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

[Cutch, 2015-10-12 21:34:08]

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

[siva, 2015-10-12 23:03:14]

lgtm with some comments.

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:94: #endif
missing 'break;' here?


Maybe you could restructure as
thread->thread_list_next_ = NULL;
return;

and add an UNREACHABLE(); at the end of the function.

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:510: bool ThreadIterator::HasNext() {
ASSERT(Thread::thread_list_lock_->IsOwnedByCurrentThread());

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:515: Thread* ThreadIterator::Next() {
ASSERT(Thread::thread_list_lock_->IsOwnedByCurrentThread());

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

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:420: static Mutex thread_list_lock_;
In general we avoid declaring a static Mutex like this as the order in which
static destructors are run by the C++ compiler can lead to race conditions where
we might be shutting down the VM, e.g: calling RemoveThreadFromList after this
mutex has been destroyed.

Maybe change to
static Mutext* thread_list_lock_ and inside
Thread::InitOnceBeforeIsolate() add a line
thread_list_lock_ = new Mutex();

and delete it in Dart::Cleanup

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:423: static void RemoveThreadFromList(Thread* thread);
these functions should be private functions to ensure that it is only called
from Thread() and ~Thread() and not from any random place?

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:474: bool HasNext();
bool HasNext() const;

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

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:475:
ThreadIteratorTestHelper::RemoveThreadFromList(current);
This test does not seem right, we should not allow a thread that is currently
part of an isolate to be removed from the thread list (reason why it should not
be possible to call this from any random place).

Similarly we should not allow a thread to randomly add itself to the list.

For this test you could create a thread, have it call Dart_EnterIsolate and see
if it has been added to the iterator list and then ensure the thread calls
Dart_ExitIsolate(), the thread dies  and then ensure it is not in the iterator
list.

[Cutch, 2015-10-13 16:41:24]

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:94: #endif
On 2015/10/12 23:03:14, siva wrote:
> missing 'break;' here?
> 
> 
> Maybe you could restructure as
> thread->thread_list_next_ = NULL;
> return;
> 
> and add an UNREACHABLE(); at the end of the function.

Done.

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:510: bool ThreadIterator::HasNext() {
On 2015/10/12 23:03:14, siva wrote:
> ASSERT(Thread::thread_list_lock_->IsOwnedByCurrentThread());

Done.

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:515: Thread* ThreadIterator::Next() {
On 2015/10/12 23:03:14, siva wrote:
> ASSERT(Thread::thread_list_lock_->IsOwnedByCurrentThread());

Done.

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

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:420: static Mutex thread_list_lock_;
On 2015/10/12 23:03:14, siva wrote:
> In general we avoid declaring a static Mutex like this as the order in which
> static destructors are run by the C++ compiler can lead to race conditions
where
> we might be shutting down the VM, e.g: calling RemoveThreadFromList after this
> mutex has been destroyed.
> 
> Maybe change to
> static Mutext* thread_list_lock_ and inside
> Thread::InitOnceBeforeIsolate() add a line
> thread_list_lock_ = new Mutex();
> 
> and delete it in Dart::Cleanup

Done.

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:423: static void RemoveThreadFromList(Thread* thread);
On 2015/10/12 23:03:14, siva wrote:
> these functions should be private functions to ensure that it is only called
> from Thread() and ~Thread() and not from any random place?

These are private.

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:474: bool HasNext();
On 2015/10/12 23:03:14, siva wrote:
> bool HasNext() const;

Done.

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

https://codereview.chromium.org/1393423005/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:475:
ThreadIteratorTestHelper::RemoveThreadFromList(current);
On 2015/10/12 23:03:14, siva wrote:
> This test does not seem right, we should not allow a thread that is currently
> part of an isolate to be removed from the thread list (reason why it should
not
> be possible to call this from any random place).
> 
> Similarly we should not allow a thread to randomly add itself to the list.
> 
> For this test you could create a thread, have it call Dart_EnterIsolate and
see
> if it has been added to the iterator list and then ensure the thread calls
> Dart_ExitIsolate(), the thread dies  and then ensure it is not in the iterator
> list.

I've rewritten the test and added assertions that a thread doesn't have an
isolate when it is added / removed from the list.

[Cutch, 2015-10-13 16:49:25]

Committed patchset #3 (id:40001) manually as
321d64a849d48d12507bb030633ffa81c9340c8d (presubmit successful).