Completely remove InterruptableThreadState and Fix ThreadRegistry leak

Completely remove InterruptableThreadState

- InterruptableThreadState is gone.
- Moved InterruptableThreadState fields directly into Thread.
- Iterate over all threads in an isolate when profiling.
- Still only sample the mutator thread.

Fix ThreadRegistry leak

- When deleting a Thread, iterate over all isolates and remove it from the isolate's thread registry.

R=iposva@google.com

Committed: https://github.com/dart-lang/sdk/commit/664742f0162a13e0d60c65de0a713df8acd7d8e9

[Cutch, 2015-08-20 14:30:29]

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

[Cutch, 2015-08-20 14:32:03]

Some sort of miscommunication last night led me to believe I should do this CL.

This CL is a super-set of https://codereview.chromium.org/1294823004/

[koda, 2015-08-20 14:55:54]

Great work (and the overlap with my CL is unfortunate, but serves as
validation). I'm a bit worried about races in the update, though.

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:24: // Remove |thread| from all isolate's thread registry.
(I'm not a native speaker, but shouldn't this be either "each isolate's
registry" or "all isolates' registries"?)

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:98: : id_(OSThread::GetCurrentThreadId()),
This should be in the same place as the call to SetCurrent, to make sure the TLS
matches the id_.

Consider initializing id_ to the invalid id here and then update it in
SetCurrent (or, possibly, move the SetCurrent to the end of this constructor,
although having such a side-effect might be surprising/problematic, even if
documented).

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:276: thread_interrupt_callback_ = callback;
This update is not done atomically. Thus, if we get a signal while the update
(triggered by enter/exit isolate) is partially complete, the callback/data might
be in an inconsistent state.

Previously, this was avoided (in practice, albeit not in theory) by temporarily
clearing the ITS* while updating.

We need to think through how to make this as safe as possible without expensive
locking.

[koda, 2015-08-20 15:18:33]

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:276: thread_interrupt_callback_ = callback;
On 2015/08/20 14:55:53, koda wrote:
> This update is not done atomically. Thus, if we get a signal while the update
> (triggered by enter/exit isolate) is partially complete, the callback/data
might
> be in an inconsistent state.
> 
> Previously, this was avoided (in practice, albeit not in theory) by
temporarily
> clearing the ITS* while updating.
> 
> We need to think through how to make this as safe as possible without
expensive
> locking.

One way to make the interface safer is to remove the unsynchronized accessors to
the individual fields and instead change ShouldInterrupt to:

1. Also take two output arguments (i.e., a ThreadInterruptCallback* and a
void**), and
2. Read the fields as volatiles (they should probably be declared volatile in
the class)
3. Only set the results and return true if both are non-NULL

And 4. Actually use ShouldInterrupt; it's currently unused in your CL.

(Actually, 2 is probably not strictly needed in our current implementation,
since, as iposva pointed out, the thread is either paused or the current thread,
so the values won't change beneath you. But assuming this will always be the
case is fragile.)

[koda, 2015-08-20 15:45:15]

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:274: void
Thread::SetThreadInterrupter(ThreadInterruptCallback callback,
Consider ASSERT that this is only called on the current thread.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:281: bool Thread::ShouldInterrupt() const {
Consider ASSERT that this is only called on the current thread unless we're on
Windows.

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:56: //   * Allocating memory.
To catch violations of this in DEBUG mode, consider adding a NoSafepointScope
around the callback invocation.

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_win.cc:65: OS::Print("ThreadInterrupted failed to
suspend thread %p\n",
ThreadInterrupted -> ThreadInterrupter

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_win.cc:77: OS::Print("ThreadInterrupted failed to
get registers for %p\n",
Ditto.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_win.cc:90: callback(its,
state->thread_interrupt_data());
Compile error: state -> thread

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:97: Reset(registry);
Wouldn't it be simpler to have a MonitorLocker field in EntryIterator and
initialize it with registry->monitor_ in the initializer list? (You don't
actually reuse the iterator anywhere.)

[Ivan Posva, 2015-08-20 18:00:10]

LGTM with EntryIterator adjusted.

Thanks!
-Ivan

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1775: // TODO(johnmccutchan): Sample all threads, not just
the mutator thread.
Please make a note that sampling should be happening on the threads and not the
isolates.

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:173:
thread->SetThreadInterrupter(RecordSampleInterruptCallback, isolate);
How often does the first value change while a process is running?

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:55: //   * Accessing TLS.
Can you please explain in the comments why these things are "verboten"?

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_test.cc:24: OS::Sleep(run_time *
kMillisecondsPerSecond);
Different CL: If the thread is sleeping we should not get any actual CPU ticks.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:106: void
ThreadRegistry::EntryIterator::Reset(ThreadRegistry* registry) {
Personally I find resettable iterators confusing to use as it becomes entirely
unclear what data structure you are iterating over. Especially since it can be
Reset at any time.

[Cutch, 2015-08-20 20:40:19]

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1775: // TODO(johnmccutchan): Sample all threads, not just
the mutator thread.
On 2015/08/20 18:00:10, Ivan Posva wrote:
> Please make a note that sampling should be happening on the threads and not
the
> isolates.

Done.

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:173:
thread->SetThreadInterrupter(RecordSampleInterruptCallback, isolate);
On 2015/08/20 18:00:10, Ivan Posva wrote:
> How often does the first value change while a process is running?

Aside from tests it never changes.

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:24: // Remove |thread| from all isolate's thread registry.
On 2015/08/20 14:55:53, koda wrote:
> (I'm not a native speaker, but shouldn't this be either "each isolate's
> registry" or "all isolates' registries"?)

Done.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:98: : id_(OSThread::GetCurrentThreadId()),
On 2015/08/20 14:55:53, koda wrote:
> This should be in the same place as the call to SetCurrent, to make sure the
TLS
> matches the id_.
> 
> Consider initializing id_ to the invalid id here and then update it in
> SetCurrent (or, possibly, move the SetCurrent to the end of this constructor,
> although having such a side-effect might be surprising/problematic, even if
> documented).

As discussed. I've moved the SetCurrent call into the constructor and the thread
id is const.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:274: void
Thread::SetThreadInterrupter(ThreadInterruptCallback callback,
On 2015/08/20 15:45:14, koda wrote:
> Consider ASSERT that this is only called on the current thread.

Done.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:276: thread_interrupt_callback_ = callback;
On 2015/08/20 15:18:33, koda wrote:
> On 2015/08/20 14:55:53, koda wrote:
> > This update is not done atomically. Thus, if we get a signal while the
update
> > (triggered by enter/exit isolate) is partially complete, the callback/data
> might
> > be in an inconsistent state.
> > 
> > Previously, this was avoided (in practice, albeit not in theory) by
> temporarily
> > clearing the ITS* while updating.
> > 
> > We need to think through how to make this as safe as possible without
> expensive
> > locking.
> 
> One way to make the interface safer is to remove the unsynchronized accessors
to
> the individual fields and instead change ShouldInterrupt to:
> 
> 1. Also take two output arguments (i.e., a ThreadInterruptCallback* and a
> void**), and
> 2. Read the fields as volatiles (they should probably be declared volatile in
> the class)
> 3. Only set the results and return true if both are non-NULL
> 
> And 4. Actually use ShouldInterrupt; it's currently unused in your CL.
> 
> (Actually, 2 is probably not strictly needed in our current implementation,
> since, as iposva pointed out, the thread is either paused or the current
thread,
> so the values won't change beneath you. But assuming this will always be the
> case is fragile.)

I've removed the individual accessors. New API:

bool IsThreadInterrupterEnabled(ThreadInterruptCallback* callback, void** data)
const;

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:281: bool Thread::ShouldInterrupt() const {
On 2015/08/20 15:45:14, koda wrote:
> Consider ASSERT that this is only called on the current thread unless we're on
> Windows.

Done.

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:55: //   * Accessing TLS.
On 2015/08/20 18:00:10, Ivan Posva wrote:
> Can you please explain in the comments why these things are "verboten"?

Done.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:56: //   * Allocating memory.
On 2015/08/20 15:45:14, koda wrote:
> To catch violations of this in DEBUG mode, consider adding a NoSafepointScope
> around the callback invocation.

This comment was about native C allocations.

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_test.cc:24: OS::Sleep(run_time *
kMillisecondsPerSecond);
On 2015/08/20 18:00:10, Ivan Posva wrote:
> Different CL: If the thread is sleeping we should not get any actual CPU
ticks.

Acknowledged.

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

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_win.cc:65: OS::Print("ThreadInterrupted failed to
suspend thread %p\n",
On 2015/08/20 15:45:14, koda wrote:
> ThreadInterrupted -> ThreadInterrupter

Done.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_win.cc:77: OS::Print("ThreadInterrupted failed to
get registers for %p\n",
On 2015/08/20 15:45:14, koda wrote:
> Ditto.

Done.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_win.cc:90: callback(its,
state->thread_interrupt_data());
On 2015/08/20 15:45:14, koda wrote:
> Compile error: state -> thread

Done.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:97: Reset(registry);
On 2015/08/20 15:45:14, koda wrote:
> Wouldn't it be simpler to have a MonitorLocker field in EntryIterator and
> initialize it with registry->monitor_ in the initializer list? (You don't
> actually reuse the iterator anywhere.)

Acknowledged.

https://codereview.chromium.org/1293253005/diff/20001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:106: void
ThreadRegistry::EntryIterator::Reset(ThreadRegistry* registry) {
On 2015/08/20 18:00:10, Ivan Posva wrote:
> Personally I find resettable iterators confusing to use as it becomes entirely
> unclear what data structure you are iterating over. Especially since it can be
> Reset at any time.

Reset is now private.

[Cutch, 2015-08-20 21:38:42]

Committed patchset #4 (id:60001) manually as
664742f0162a13e0d60c65de0a713df8acd7d8e9 (presubmit successful).