Switch profiler from isolates to threads

Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked -> Dart_ThreadDisableProfiling, Dart_ThreadEnableProfiling.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
- Only record sample if thread is the current mutator of an isolate.
- Refactor ThreadInterrupterCallback to ensure that Thread* is valid.

Threads are only ever sent signals if ThreadInterruptsEnabled is true. Which is controlled by two functions:

void DisableThreadInterrupts();
void EnableThreadInterrupts();

R=asiva@google.com, iposva@google.com

Committed: https://github.com/dart-lang/sdk/commit/6365c27f81c02605ac9d9ec879022ceacc0bdea8

[Cutch, 2015-10-19 21:51:17]

Description was changed from

==========
Profile threads not isolates

BUG=
==========

to

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadBlocked, Dart_ThreadUnblocked.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
==========

[Cutch, 2015-10-19 21:53:40]

Description was changed from

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadBlocked, Dart_ThreadUnblocked.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
==========

to

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadBlocked, Dart_ThreadUnblocked.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
==========

[Cutch, 2015-10-19 21:53:40]

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

[Cutch, 2015-10-19 21:54:58]

Description was changed from

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadBlocked, Dart_ThreadUnblocked.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
==========

to

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadBlocked, Dart_ThreadUnblocked.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
- Only record sample if thread is the current mutator of an isolate.
==========

[Cutch, 2015-10-23 18:41:43]

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

[Cutch, 2015-10-23 18:41:49]

+iposva

[Cutch, 2015-10-23 18:43:47]

Description was changed from

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadBlocked, Dart_ThreadUnblocked.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
- Only record sample if thread is the current mutator of an isolate.
==========

to

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadBlocked, Dart_ThreadUnblocked.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
- Only record sample if thread is the current mutator of an isolate.

Threads are only ever sent signals if ThreadInterruptsEnabled is true. Which is
controlled by two functions:

void DisableThreadInterrupts();
void EnableThreadInterrupts();
==========

[siva, 2015-10-26 23:45:26]

I have one global question, we have a thread registry which tracks all threads
that have done a Thread::EnterIsolate, in addition we have the global thread
list which has all the threads for which Thread::EnsureInit has been called and
is used by the profiler. Can I unify these in the changes I am making?

https://codereview.chromium.org/1412733008/diff/20001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/1412733008/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:1015: DART_EXPORT void Dart_ThreadBlocked();
With the new safepointing code a Blocked thread is going to have a special
meaning and tracking state, calling this Dart_ThreadBlocked may  confuse people
with safepoint stuff.

Can we call this
 - Dart_DisableThreadProfiling
 - Dart_EnableThreadProfiling
instead?

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

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:160: }
Both these functions
  MutatorThreadIsCurrentThread
  IsMutatorThread
should be removed from here and we should have a function in Thread
  IsMutatorThread

(I have submitted a cl that cleans up MutatorThreadIsCurrentThread() and added a
IsMutatorThread method in the thread).

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

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:1074: Thread* thread = reinterpret_cast<Thread*>(data);
ASSERT this is a valid thread in the thread list?

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:1075: const bool in_dart_code = ExecutingDart(thread);
ExecutingDart should be in class Thread now?

thread->ExecutingDart();

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:1110: SampleMutator(thread, pc, fp, sp);
So we will not be profiling the background compilation threads, parallel marker
threads etc. initially, I guess that is intentional.

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

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:182: 
Do we need to be holding the monitor_ lock here, we have only one interrupter
thread right so access and updates of variable current_wait_time_ should be
safe.

I am just trying to see if we can avoid the nesting of locks here as
ThreadIterator will be taking the global lock Thread::thread_list_lock_. (Will
keep the code simpler)

[Cutch, 2015-10-27 22:20:21]

Description was changed from

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadBlocked, Dart_ThreadUnblocked.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
- Only record sample if thread is the current mutator of an isolate.

Threads are only ever sent signals if ThreadInterruptsEnabled is true. Which is
controlled by two functions:

void DisableThreadInterrupts();
void EnableThreadInterrupts();
==========

to

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadDisableProfiling, Dart_ThreadEnableProfiling.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
- Only record sample if thread is the current mutator of an isolate.

Threads are only ever sent signals if ThreadInterruptsEnabled is true. Which is
controlled by two functions:

void DisableThreadInterrupts();
void EnableThreadInterrupts();
==========

[Cutch, 2015-10-27 22:21:20]

Description was changed from

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadDisableProfiling, Dart_ThreadEnableProfiling.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
- Only record sample if thread is the current mutator of an isolate.

Threads are only ever sent signals if ThreadInterruptsEnabled is true. Which is
controlled by two functions:

void DisableThreadInterrupts();
void EnableThreadInterrupts();
==========

to

==========
Switch profiler from isolates to threads

- API breakage: Dart_IsolateBlocked, Dart_IsolateUnblocked ->
Dart_ThreadDisableProfiling, Dart_ThreadEnableProfiling.
- Remove IsolateProfilerData.
- Move thread at blocking call count from isolate to thread.
- Always interrupt threads unless they are blocked.
- We can no longer count "idle" ticks.
- Only record sample if thread is the current mutator of an isolate.
- Refactor ThreadInterrupterCallback to ensure that Thread* is valid.

Threads are only ever sent signals if ThreadInterruptsEnabled is true. Which is
controlled by two functions:

void DisableThreadInterrupts();
void EnableThreadInterrupts();
==========

[Cutch, 2015-10-27 22:36:24]

https://codereview.chromium.org/1412733008/diff/20001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/1412733008/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:1015: DART_EXPORT void Dart_ThreadBlocked();
On 2015/10/26 23:45:26, siva wrote:
> With the new safepointing code a Blocked thread is going to have a special
> meaning and tracking state, calling this Dart_ThreadBlocked may  confuse
people
> with safepoint stuff.
> 
> Can we call this
>  - Dart_DisableThreadProfiling
>  - Dart_EnableThreadProfiling
> instead?

Done.

Dart_ThreadDisableProfiling
Dart_ThreadEnableProfiling

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

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:160: }
On 2015/10/26 23:45:26, siva wrote:
> Both these functions
>   MutatorThreadIsCurrentThread
>   IsMutatorThread
> should be removed from here and we should have a function in Thread
>   IsMutatorThread
> 
> (I have submitted a cl that cleans up MutatorThreadIsCurrentThread() and added
a
> IsMutatorThread method in the thread).

I've switched to using thread->IsMutatorThread() and removed 'IsMutatorThread'
from my CL.

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

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:1075: const bool in_dart_code = ExecutingDart(thread);
On 2015/10/26 23:45:26, siva wrote:
> ExecutingDart should be in class Thread now?
> 
> thread->ExecutingDart();

Done.

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:1110: SampleMutator(thread, pc, fp, sp);
On 2015/10/26 23:45:26, siva wrote:
> So we will not be profiling the background compilation threads, parallel
marker
> threads etc. initially, I guess that is intentional.

This is intentional and temporary.

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

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:182: 
On 2015/10/26 23:45:26, siva wrote:
> Do we need to be holding the monitor_ lock here, we have only one interrupter
> thread right so access and updates of variable current_wait_time_ should be
> safe.
> 
> I am just trying to see if we can avoid the nesting of locks here as
> ThreadIterator will be taking the global lock Thread::thread_list_lock_. (Will
> keep the code simpler)

Done.

[Cutch, 2015-10-27 22:47:50]

A couple comments got lost somehow:

1) As discussed offline. The thread interrupter could be driven from a different
list of threads so long as it includes all threads in isolates. The Timeline
system needs a list of all threads that have been in the VM.

2) I've refactored the ThreadInterrupter so that we can guarantee that Thread*
is valid. This removes the need to verify that it is present in the list (which
is slightly racy).

PTAL.

Thanks,
John

[Ivan Posva, 2015-10-28 06:45:16]

Sending out my pending comments. Not sure how many still apply after the
patches...

-Ivan

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (left):

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2550: if (!isolate_->single_step()) {
Where is the equivalent for single step handling in this thread based version?

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

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:109: void Profiler::BeginExecution(Isolate* isolate) {
Is this API even needed given that we are profiling based on threads?

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:122: void Profiler::EndExecution(Isolate* isolate) {
ditto.

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:973: void Profiler::SampleMutator(Thread* thread,
Not only mutator, right?

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

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/profiler.h#n...
runtime/vm/profiler.h:44: static void SampleMutator(Thread* thread,
Shouldn't this be SampleThread or is the mutator handled in a special way from
the other threads?

[Ivan Posva, 2015-10-28 08:27:47]

I am not 100% convinced how this wakes up from "deep sleep".

-Ivan

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/dart.cc
File runtime/vm/dart.cc (left):

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/dart.cc#old...
runtime/vm/dart.cc:301: Profiler::InitProfilingForIsolate(I);
There is no equivalent needed because the fact that this thread is associated
with an isolate makes it profilable, correct?

How do we ensure that the profiler is ticking though?

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:134: thread->SetThreadInterruptCallback(NULL);
It looks like we have several redundant ways of disabling profiles on a
particular thread. Can we consolidate them?

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:896: const bool exited_dart_code =
thread->HasExitedDartCode();
Shouldn't this be an assert? The thread has better exited Dart code for it to be
running in C++.

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:979: if (isolate->HasDebugger()) {
Please add a comment that this is really only relevant during bring up of the
isolate. All isolates have a debugger object at all times.

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:987: MessageHandler* msg_handler =
isolate->message_handler();
This and the debugger check are only relevant for the mutator thread.

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:995: const bool exited_dart_code =
thread->HasExitedDartCode();
Is there any particular reason why this should be done early vs at the call site
to CollectSample.

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter.cc (right):

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter.cc:127: void ThreadInterrupter::WakeUp() {
It is not clear to me from this CL when the ThreadInterrupter is being woken up.
Obviously when a new thread is created and added to the list of threads and when
the API marks a thread as unblocked. But in regular message handler execution?

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter.cc:146: void ThreadInterruptNoOp(Thread* thread,
const InterruptedThreadState& state) {
Where is this used?

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter.cc:171: if ((r == Monitor::kNotified) &&
shutdown_) {
Shouldn't we break out of the loop regardless whether we were notified or timed
out?

[Cutch, 2015-10-28 14:39:19]

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/dart.cc
File runtime/vm/dart.cc (left):

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/dart.cc#old...
runtime/vm/dart.cc:301: Profiler::InitProfilingForIsolate(I);
On 2015/10/28 08:27:47, Ivan Posva wrote:
> There is no equivalent needed because the fact that this thread is associated
> with an isolate makes it profilable, correct?
> 

Correct.

> How do we ensure that the profiler is ticking though?

I'm doing a pass now to ensure that the profiler is ticking threads and stops
ticking when a thread exists an isolate.

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:896: const bool exited_dart_code =
thread->HasExitedDartCode();
On 2015/10/28 08:27:47, Ivan Posva wrote:
> Shouldn't this be an assert? The thread has better exited Dart code for it to
be
> running in C++.

This is called from Object::Allocate which means we could be here before having
ever entered Dart code (an allocation made from C++ code before invoking Dart).

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:979: if (isolate->HasDebugger()) {
On 2015/10/28 08:27:47, Ivan Posva wrote:
> Please add a comment that this is really only relevant during bring up of the
> isolate. All isolates have a debugger object at all times.

Done.

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:987: MessageHandler* msg_handler =
isolate->message_handler();
On 2015/10/28 08:27:47, Ivan Posva wrote:
> This and the debugger check are only relevant for the mutator thread.

I've factored both of these checks into a helper function 'ShouldSampleMutator'.

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:995: const bool exited_dart_code =
thread->HasExitedDartCode();
On 2015/10/28 08:27:47, Ivan Posva wrote:
> Is there any particular reason why this should be done early vs at the call
site
> to CollectSample.

No. Moved down closer to their use.

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter.cc (right):

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter.cc:146: void ThreadInterruptNoOp(Thread* thread,
const InterruptedThreadState& state) {
On 2015/10/28 08:27:47, Ivan Posva wrote:
> Where is this used?

Nowhere. It's gone now.

https://codereview.chromium.org/1412733008/diff/160001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter.cc:171: if ((r == Monitor::kNotified) &&
shutdown_) {
On 2015/10/28 08:27:47, Ivan Posva wrote:
> Shouldn't we break out of the loop regardless whether we were notified or
timed
> out?

Done.

[Cutch, 2015-10-28 14:57:52]

PTAL at Patch Set 12.

Threads no longer receive interrupts by default.

Thread::EnterIsolate
Thread::EnterIsolateAsHelper

Enable interrupts for the thread.

Thread::ExitIsolate
Thread::ExitIsolateAsHelper

Disable interrupts for the thread.

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (left):

https://codereview.chromium.org/1412733008/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2550: if (!isolate_->single_step()) {
On 2015/10/28 06:45:16, Ivan Posva wrote:
> Where is the equivalent for single step handling in this thread based version?

I've moved this into Debugger::Pause which disables interrupts while the thread
waits for debugger commands.

[Ivan Posva, 2015-10-28 19:31:54]

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

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:310: bool HasDebugger() const {
Still needed?

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1031: if (debugger->IsPaused()) {
Is this still needed given the fact that the debugger disables sampling?

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1040: (msg_handler->paused_on_start() ||
If the isolate is paused on start/exit then it is not actively running on a
thread so we should not even get here.

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler_se...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler_se...
runtime/vm/profiler_service.cc:2200: thread->DisableThreadInterrupts();
Should we have a DisableInterruptsScope?

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

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread.cc#n...
runtime/vm/thread.cc:71: ThreadInterrupter::WakeUp();
Does not match our discussion from earlier today. Why does the ThreadInterrupter
get kicked here?

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter_macos.cc (right):

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_macos.cc:42: callback(thread, its);
Profiler::SampleThread() or some similar non-callback call?

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

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_win.cc:85: callback(thread, its);
ditto, here and other places.

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_test.cc
File runtime/vm/thread_test.cc (left):

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_test...
runtime/vm/thread_test.cc:43: // Thread interrupter interferes with this test,
disable interrupts.
We might want to keep this comment.

[Cutch, 2015-10-28 19:50:34]

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

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:310: bool HasDebugger() const {
On 2015/10/28 19:31:53, Ivan Posva wrote:
> Still needed?

Done.

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1031: if (debugger->IsPaused()) {
On 2015/10/28 19:31:53, Ivan Posva wrote:
> Is this still needed given the fact that the debugger disables sampling?

This whole function is now gone.

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler.cc...
runtime/vm/profiler.cc:1040: (msg_handler->paused_on_start() ||
On 2015/10/28 19:31:53, Ivan Posva wrote:
> If the isolate is paused on start/exit then it is not actively running on a
> thread so we should not even get here.

Done.

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler_se...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/profiler_se...
runtime/vm/profiler_service.cc:2200: thread->DisableThreadInterrupts();
On 2015/10/28 19:31:53, Ivan Posva wrote:
> Should we have a DisableInterruptsScope?

Done here and below.

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

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread.cc#n...
runtime/vm/thread.cc:71: ThreadInterrupter::WakeUp();
On 2015/10/28 19:31:53, Ivan Posva wrote:
> Does not match our discussion from earlier today. Why does the
ThreadInterrupter
> get kicked here?

Forgot to remove this. It's gone now.

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter_macos.cc (right):

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_macos.cc:42: callback(thread, its);
On 2015/10/28 19:31:53, Ivan Posva wrote:
> Profiler::SampleThread() or some similar non-callback call?

Done.

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

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_win.cc:85: callback(thread, its);
On 2015/10/28 19:31:53, Ivan Posva wrote:
> ditto, here and other places.

Done.

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_test.cc
File runtime/vm/thread_test.cc (left):

https://codereview.chromium.org/1412733008/diff/240001/runtime/vm/thread_test...
runtime/vm/thread_test.cc:43: // Thread interrupter interferes with this test,
disable interrupts.
On 2015/10/28 19:31:54, Ivan Posva wrote:
> We might want to keep this comment.

Done.

[Cutch, 2015-10-29 20:01:05]

*ping*

[Ivan Posva, 2015-10-29 21:45:05]

LGTM -ip

[siva, 2015-10-29 21:47:41]

lgtm

https://chromiumcodereview.appspot.com/1412733008/diff/280001/runtime/vm/thre...
File runtime/vm/thread_interrupter.cc (right):

https://chromiumcodereview.appspot.com/1412733008/diff/280001/runtime/vm/thre...
runtime/vm/thread_interrupter.cc:197: // Now that we have the lock, check if we
were signal to wake up while
we were signalled

https://chromiumcodereview.appspot.com/1412733008/diff/280001/runtime/vm/thre...
runtime/vm/thread_interrupter.cc:210: 
Why is it necessary to set the local variable woken_up up above, wouldn't is
suffice to just set
woken_up_ = false;
here?

https://chromiumcodereview.appspot.com/1412733008/diff/280001/runtime/vm/thre...
File runtime/vm/thread_interrupter_win.cc (right):

https://chromiumcodereview.appspot.com/1412733008/diff/280001/runtime/vm/thre...
runtime/vm/thread_interrupter_win.cc:13: 
extra blank line?

[Cutch, 2015-11-02 20:47:12]

https://codereview.chromium.org/1412733008/diff/280001/runtime/vm/thread_inte...
File runtime/vm/thread_interrupter.cc (right):

https://codereview.chromium.org/1412733008/diff/280001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter.cc:197: // Now that we have the lock, check if we
were signal to wake up while
On 2015/10/29 21:47:41, siva wrote:
> we were signalled

Done.

https://codereview.chromium.org/1412733008/diff/280001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter.cc:210: 
On 2015/10/29 21:47:41, siva wrote:
> Why is it necessary to set the local variable woken_up up above, wouldn't is
> suffice to just set
> woken_up_ = false;
> here?

Done.

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

https://codereview.chromium.org/1412733008/diff/280001/runtime/vm/thread_inte...
runtime/vm/thread_interrupter_win.cc:13: 
On 2015/10/29 21:47:41, siva wrote:
> extra blank line?

Done.

[Cutch, 2015-11-02 20:47:57]

Committed patchset #17 (id:300001) manually as
6365c27f81c02605ac9d9ec879022ceacc0bdea8 (presubmit successful).