Use list of isolates in profiler

Keep list of existing isolates.
List is only updated when isolates are created / destroyed.
ThreadInterrupter now walks list of isolates.
Scheduling an isolate for interrupts is now lockless.
Setting a current isolate is now lockless.
Working on Linux, Mac, and Windows.
Assert that the isolate isn't on two threads at once.

R=asiva@google.com

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

[Cutch, 2014-01-09 22:49:28]

PTAL.

[Cutch, 2014-02-04 22:50:51]

PTAL

[siva, 2014-02-05 19:09:15]

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/benchmark_te...
File runtime/vm/benchmark_test.cc (right):

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/benchmark_te...
runtime/vm/benchmark_test.cc:503: }
I think we also need a benchmark to measure Creation of Isolates in order to
figure out how costly adding an Isolate to a list during creation is going to
be.

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

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:354: if (current != NULL) {
We also need
ASSERT(current->thread_state() == NULL);

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:359:
current->set_thread_state(ThreadInterrupter::GetCurrentThreadState());
Why not have a local variable
InterruptibleThreadState* current_thread_state =
ThreadInterrupter::GetCurrentThreadState();

and use that local in the two spots.

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:924: }
See comment on using a linked list for isolates list.

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:935: }
If a singly linked list is used for isolates this function could also
be under
#if defined(DEBUG)
....
#endif
and could be named CheckForDuplicateIsolates

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:938: void
Isolate::CheckNoIsolateHasThreadData(InterruptableThreadState* state) {
I would call this CheckForDuplicateThreadState and this function can be under
#if defined(DEBUG)
...
#endif
I suppose.

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

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:506: static intptr_t isolates_size_;
Why not just make the isolate list a singly linked list instead of a growable
array with the need to maintain size, capacity and grow it.

You could add a next pointer to the Isolate class and chain them up.

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:514: friend class ThreadInterrupterWin;
Seems weird that you are having to friend a bunch of OS specific classes in this
target independent class.

Maybe instead you should define an Iterator that takes a Visitor function as a
parameter similar to the heap iterator and have the Thread Interruptor provide a
visitor that interrupts isolates that have a non null thread state.

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/thread_inter...
File runtime/vm/thread_interrupter.cc (right):

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:67: void ThreadInterrupter::Startup() {
If --no-profile is specified this should be a noop right? do we need this
functionality of interrupting threads periodically for anything other than the
profiler?

[Cutch, 2014-02-05 23:00:31]

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/benchmark_te...
File runtime/vm/benchmark_test.cc (right):

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/benchmark_te...
runtime/vm/benchmark_test.cc:503: }
On 2014/02/05 19:09:15, siva wrote:
> I think we also need a benchmark to measure Creation of Isolates in order to
> figure out how costly adding an Isolate to a list during creation is going to
> be.

With a linked list approach the only bottleneck will be contention from creating
isolates on different threads. We already track single isolate startup time so
adding another benchmark probably isn't worth it.

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

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:354: if (current != NULL) {
On 2014/02/05 19:09:15, siva wrote:
> We also need
> ASSERT(current->thread_state() == NULL);

Done.

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:359:
current->set_thread_state(ThreadInterrupter::GetCurrentThreadState());
On 2014/02/05 19:09:15, siva wrote:
> Why not have a local variable
> InterruptibleThreadState* current_thread_state =
> ThreadInterrupter::GetCurrentThreadState();
> 
> and use that local in the two spots.

Done.

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:938: void
Isolate::CheckNoIsolateHasThreadData(InterruptableThreadState* state) {
On 2014/02/05 19:09:15, siva wrote:
> I would call this CheckForDuplicateThreadState and this function can be under
> #if defined(DEBUG)
> ...
> #endif
> I suppose.

Done.

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

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:506: static intptr_t isolates_size_;
On 2014/02/05 19:09:15, siva wrote:
> Why not just make the isolate list a singly linked list instead of a growable
> array with the need to maintain size, capacity and grow it.
> 
> You could add a next pointer to the Isolate class and chain them up.

Done.

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:514: friend class ThreadInterrupterWin;
On 2014/02/05 19:09:15, siva wrote:
> Seems weird that you are having to friend a bunch of OS specific classes in
this
> target independent class.
> 
> Maybe instead you should define an Iterator that takes a Visitor function as a
> parameter similar to the heap iterator and have the Thread Interruptor provide
a
> visitor that interrupts isolates that have a non null thread state.

Done.

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/thread_inter...
File runtime/vm/thread_interrupter.cc (right):

https://codereview.chromium.org/128653004/diff/260001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:67: void ThreadInterrupter::Startup() {
On 2014/02/05 19:09:15, siva wrote:
> If --no-profile is specified this should be a noop right? do we need this
> functionality of interrupting threads periodically for anything other than the
> profiler?

Correct, with --no-profile this function never gets called.

[siva, 2014-02-06 00:37:23]

LGTM with some comments.

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/benchmark_te...
File runtime/vm/benchmark_test.cc (right):

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/benchmark_te...
runtime/vm/benchmark_test.cc:503: }
To get this running on go/darta you will have to add this to the function
loadVMBenchmarks() in golem4/config/benchmarks.dart

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

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:931: }
Might be more efficient to write this as:

if (isolate == isolates_head_) {
  isolates_head_ = isolates_head_->next_;
  return;
}
Isolate* previous = isolates_head_;
Isolate* current = isolates_head_->next_;
while (current) {
  if (isolate == current) {
    ASSERT(previous != NULL);
    previous->next_ = current->next_;
    return;
  }
  previous = current;
  current = current->next_;
}
UNREACHABLE();

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

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:86: private:
DISALLOW stuff here?

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:517: static void IsolateShutdown(Isolate* isolate);
Why not call these functions
AddIsolateToList and RemoveIsolateFromList

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:520: static Isolate* isolates_head_;
should we call these isolates_list_monitor_ and isolates_list_ ?

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:521: Isolate* next_;
style guide states this instance field should be grouped with the other instance
fields above. You could add a comment stating it is used to chain the isolate
into the isolate list.

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:529: friend class ThreadInterrupterWin;
Are these friends still needed?

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
File runtime/vm/thread_interrupter.cc (right):

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:62: monitor_ = new Monitor();
ASSERT(monitor_ != NULL);

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:160: state = new InterruptableThreadState();
ASSERT(state != NULL);

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
File runtime/vm/thread_interrupter.h (right):

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.h:81: friend class ThreadInterrupterWin;
Do these OS specific classes need to be friends here? You could make
CurrentThreadState() a public static method, it is just a read only access.

[Cutch, 2014-02-06 17:51:51]

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

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:931: }
On 2014/02/06 00:37:24, siva wrote:
> Might be more efficient to write this as:
> 
> if (isolate == isolates_head_) {
>   isolates_head_ = isolates_head_->next_;
>   return;
> }
> Isolate* previous = isolates_head_;
> Isolate* current = isolates_head_->next_;
> while (current) {
>   if (isolate == current) {
>     ASSERT(previous != NULL);
>     previous->next_ = current->next_;
>     return;
>   }
>   previous = current;
>   current = current->next_;
> }
> UNREACHABLE();

Done.

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

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:86: private:
On 2014/02/06 00:37:24, siva wrote:
> DISALLOW stuff here?

Done.

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:517: static void IsolateShutdown(Isolate* isolate);
On 2014/02/06 00:37:24, siva wrote:
> Why not call these functions
> AddIsolateToList and RemoveIsolateFromList

Done.

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:520: static Isolate* isolates_head_;
On 2014/02/06 00:37:24, siva wrote:
> should we call these isolates_list_monitor_ and isolates_list_ ?

Done.

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:521: Isolate* next_;
On 2014/02/06 00:37:24, siva wrote:
> style guide states this instance field should be grouped with the other
instance
> fields above. You could add a comment stating it is used to chain the isolate
> into the isolate list.

Done.

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:529: friend class ThreadInterrupterWin;
On 2014/02/06 00:37:24, siva wrote:
> Are these friends still needed?

Nope. Removed.

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
File runtime/vm/thread_interrupter.cc (right):

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:62: monitor_ = new Monitor();
On 2014/02/06 00:37:24, siva wrote:
> ASSERT(monitor_ != NULL);

Done.

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:160: state = new InterruptableThreadState();
On 2014/02/06 00:37:24, siva wrote:
> ASSERT(state != NULL);

Done.

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
File runtime/vm/thread_interrupter.h (right):

https://codereview.chromium.org/128653004/diff/330001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.h:81: friend class ThreadInterrupterWin;
On 2014/02/06 00:37:24, siva wrote:
> Do these OS specific classes need to be friends here? You could make
> CurrentThreadState() a public static method, it is just a read only access.

Done.

[Cutch, 2014-02-07 17:43:31]

Committed patchset #14 manually as r32439 (presubmit successful).