Profiler Take 2

* Introduce ThreadInterrupter which calls a TLS set callback when thread is interrupted.
* Threads can only register and unregister themselves with ThreadInterrupter.
* Profiler is no longer involved in interrupting threads. It's just a callback and the buffer.
* Profiler operates lock free using an atomic operation to reserve sample in sample buffer.
* Linux, Mac, and Windows done.

R=asiva@google.com

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

[siva, 2013-12-11 02:52:20]

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
File runtime/platform/thread_android.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
runtime/platform/thread_android.cc:152: intptr_t
Thread::ThreadIdToIntPtr(ThreadId id) {
We should probably assert that sizeof(ThreadId) is equal to sizeof(intptr_t)
otherwise we could get a truncation here.

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
File runtime/platform/thread_linux.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
runtime/platform/thread_linux.cc:153: intptr_t Thread::ThreadIdToIntPtr(ThreadId
id) {
Ditto comment about asserting sizes.

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
File runtime/platform/thread_macos.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
runtime/platform/thread_macos.cc:145: intptr_t Thread::ThreadIdToIntPtr(ThreadId
id) {
Ditto.

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
File runtime/platform/thread_win.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
runtime/platform/thread_win.cc:137: intptr_t Thread::ThreadIdToIntPtr(ThreadId
id) {
Ditto.

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

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:82: void Profiler::ShutdownIsolateForProfiling(Isolate*
isolate) {
The names InitIsolateForProfiling and ShutdownIsolateForProfiling seem weirds,
sounds like the isolate is being shutdown for profiling

why not use
InitProfilingForIsolate
ShutdownProfilingForIsolate

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:98: profiler_data->set_sample_buffer(NULL);
if shared_buffer is not true how is the per isolate sample buffer deleted?

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:121: }
Are the two checks above more an assertion, i.e:

ASSERT(profiler_data != NULL);
ASSERT(sample_buffer != NULL);

because BeginExecution should only be done after InitProfilingForIsolate();

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:144: }
Ditto comment about ASSERT vs hard checks.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:352: fclose(f);
We don't operate on files directly like this inside the VM.
There will be issues when used inside Dartium.

We have these callbacks 
    Dart_FileOpenCallback file_open,
    Dart_FileReadCallback file_read,
    Dart_FileWriteCallback file_write,
    Dart_FileCloseCallback file_close,
In Dart_Initialize(...).

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:409: uintptr_t cursor =
AtomicOperations::FetchAndIncrement(&cursor_);
what happens when cursor overflows?
Also there is a corner case here where a thread picks up a cursor gets scheduled
out and then we have other threads incrementing samples till we get another
round through capacity_. At this point two isolates will get the same
SampleBuffer.

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

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:5: #include <cstdio>
what is this?

We don't include c++ header files normally.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:158: ml.Notify();
Why is this notify needed? The next wake up of the interrupt thread will pick up
this isolate right?

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:187: ml.Notify();
Ditto comment about this Notify.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:237: void
ThreadInterrupter::ResizeThreads(intptr_t new_capacity) {
// Must be called with monitor_ locked.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
File runtime/vm/thread_interrupter_linux.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_linux.cc:58:
SignalHandler::Install(ThreadInterrupterLinux::ThreadInterruptSignalHandler);
The ThreadMain function in the 4 versions seem identical except for the
signalHandler installation part, why not have a
ThreadInterrupter::InstallSignalHandler() function which is implemented per OS
and move the ThreadMain code into thread_interrupter.cc

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

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_win.cc:109: }
Why is the pattern different here?
We have a MutexLocker for Windows too.

[Cutch, 2013-12-11 17:44:56]

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
File runtime/platform/thread_android.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
runtime/platform/thread_android.cc:152: intptr_t
Thread::ThreadIdToIntPtr(ThreadId id) {
On 2013/12/11 02:52:21, siva wrote:
> We should probably assert that sizeof(ThreadId) is equal to sizeof(intptr_t)
> otherwise we could get a truncation here.

Done.

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
File runtime/platform/thread_linux.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
runtime/platform/thread_linux.cc:153: intptr_t Thread::ThreadIdToIntPtr(ThreadId
id) {
On 2013/12/11 02:52:21, siva wrote:
> Ditto comment about asserting sizes.

Done.

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
File runtime/platform/thread_macos.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
runtime/platform/thread_macos.cc:145: intptr_t Thread::ThreadIdToIntPtr(ThreadId
id) {
On 2013/12/11 02:52:21, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
File runtime/platform/thread_win.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/platform/thread...
runtime/platform/thread_win.cc:137: intptr_t Thread::ThreadIdToIntPtr(ThreadId
id) {
On 2013/12/11 02:52:21, siva wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:82: void Profiler::ShutdownIsolateForProfiling(Isolate*
isolate) {
On 2013/12/11 02:52:21, siva wrote:
> The names InitIsolateForProfiling and ShutdownIsolateForProfiling seem weirds,
> sounds like the isolate is being shutdown for profiling
> 
> why not use
> InitProfilingForIsolate
> ShutdownProfilingForIsolate

Done.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:98: profiler_data->set_sample_buffer(NULL);
On 2013/12/11 02:52:21, siva wrote:
> if shared_buffer is not true how is the per isolate sample buffer deleted?

IsolateProfilerData tracks whether it owns the buffer or not. Deletion occurs in
IsolateProfilerData::~IsolateProfilerData().

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:121: }
On 2013/12/11 02:52:21, siva wrote:
> Are the two checks above more an assertion, i.e:
> 
> ASSERT(profiler_data != NULL);
> ASSERT(sample_buffer != NULL);
> 
> because BeginExecution should only be done after InitProfilingForIsolate();

No. BeginExecution can be called before an isolate has profiling enabled.
BeginExecution and EndExecution are called indiscriminately inside SetCurrent.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:352: fclose(f);
On 2013/12/11 02:52:21, siva wrote:
> We don't operate on files directly like this inside the VM.
> There will be issues when used inside Dartium.
> 
> We have these callbacks 
>     Dart_FileOpenCallback file_open,
>     Dart_FileReadCallback file_read,
>     Dart_FileWriteCallback file_write,
>     Dart_FileCloseCallback file_close,
> In Dart_Initialize(...).

Done.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:409: uintptr_t cursor =
AtomicOperations::FetchAndIncrement(&cursor_);
On 2013/12/11 02:52:21, siva wrote:
> what happens when cursor overflows?

We wrap around back to zero.

> Also there is a corner case here where a thread picks up a cursor gets
scheduled
> out and then we have other threads incrementing samples till we get another
> round through capacity_. At this point two isolates will get the same
> SampleBuffer.

With a shared buffer there are two corner cases. The one you just mentioned,
although I think the odds of it happening are slim because we would need a
thread to be descheduled for minutes. The second corner case I think will be
more common and that is when reading from a shared buffer we will scan the
entire buffer looking for samples for a specific isolate. It's possible that
other threads are overwriting samples as we are reading them.

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

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:5: #include <cstdio>
On 2013/12/11 02:52:21, siva wrote:
> what is this?
> 
> We don't include c++ header files normally.

Done.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:158: ml.Notify();
On 2013/12/11 02:52:21, siva wrote:
> Why is this notify needed? The next wake up of the interrupt thread will pick
up
> this isolate right?

The notify is there in case we switch the thread to a more sophisticated
scheduling loop.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:237: void
ThreadInterrupter::ResizeThreads(intptr_t new_capacity) {
On 2013/12/11 02:52:21, siva wrote:
> // Must be called with monitor_ locked.

Done.

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
File runtime/vm/thread_interrupter_linux.cc (right):

https://codereview.chromium.org/109803002/diff/120001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter_linux.cc:58:
SignalHandler::Install(ThreadInterrupterLinux::ThreadInterruptSignalHandler);
On 2013/12/11 02:52:21, siva wrote:
> The ThreadMain function in the 4 versions seem identical except for the
> signalHandler installation part, why not have a
> ThreadInterrupter::InstallSignalHandler() function which is implemented per OS
> and move the ThreadMain code into thread_interrupter.cc

Done.

[siva, 2013-12-13 21:29:13]

LGTM with some comments.

https://codereview.chromium.org/109803002/diff/230001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/109803002/diff/230001/runtime/bin/file.cc#new...
runtime/bin/file.cc:34: Dart_DisableThreadInterrupter();
I am wondering if we should have a macro around all blocking calls
and put DisableInterrupts/EnableInterrupts in that macro.

Maybe we could leverage TEMP_FAILURE_RETRY (we seem to wrap all
system calls with this macro). We can't replace TEMP_FAILURE_RETRY
as it is a predefined macro but we could add a new macro which
does the interrupt disable/enable stuff and then calls
TEMP_FAILURE_RETRY

s/TEMP_FAILURE_RETRY/BLOCKING_SYSTEM_CALL/g

#define BLOCKING_SYSTEM_CALL(expression)
  Dart_DisableThreadInterrupter();
  TEMP_FAILURE_RETRY(expression);
  Dart_EnableThreadInterrupter();

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/file.cc#new...
runtime/bin/file.cc:14: #include "include/dart_native_api.h"
Leftover from a previous version? Not needed anymore I guess.

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/file_androi...
File runtime/bin/file_android.cc (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/file_androi...
runtime/bin/file_android.cc:71: return TEMP_FAILURE_RETRY(read(handle_->fd(),
buffer, num_bytes));
We should consider moving this blocking stuff into TEMP_FAILURE_RETRY using a
new macro (I made that comment in a previous version of your upload).

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/file_linux.cc
File runtime/bin/file_linux.cc (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/file_linux....
runtime/bin/file_linux.cc:27: 
???

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/process.cc
File runtime/bin/process.cc (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/process.cc#...
runtime/bin/process.cc:213: Dart_ShutdownThreadInterrupter();
Could we just call Dart_Cleanup() here, that would make things more consistent?

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/signal_bloc...
File runtime/bin/signal_blocker.h (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/signal_bloc...
runtime/bin/signal_blocker.h:26: pthread_sigmask(SIG_BLOCK, &signal_mask, &old);
int result = pthread_sigmask(....);
ASSERT(!retval);

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/signal_bloc...
runtime/bin/signal_blocker.h:31: pthread_sigmask(SIG_SETMASK, &old, NULL);
Ditto here.

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/socket.cc
File runtime/bin/socket.cc (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/socket.cc#n...
runtime/bin/socket.cc:17: #include "include/dart_native_api.h"
Leftover from a previous version, can be removed.

https://codereview.chromium.org/109803002/diff/270001/runtime/include/dart_na...
File runtime/include/dart_native_api.h (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/include/dart_na...
runtime/include/dart_native_api.h:148: DART_EXPORT void
Dart_ShutdownThreadInterrupter();
If we called Dart_Cleanup in process exit this would not be needed?

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

https://codereview.chromium.org/109803002/diff/270001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:66: if (!FLAG_thread_interrupter) {
If FLAG_profiler is false will FLAG_thread_interrupter also be false
and similarly if FLAG_thread_interrupter is false will FLAG_profiler be false?

Should they just be one flag.

https://codereview.chromium.org/109803002/diff/270001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:183: ml.Notify();
Is this notify necessary? This thread will get ticked on the next round right.

https://codereview.chromium.org/109803002/diff/270001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:196: ml.Notify();
Ditto question.

https://codereview.chromium.org/109803002/diff/270001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:383: }
Could this be:

MonitorLocker loop(monitor_);
while (!shutdown) {
  ....
  ....
  loop.Wait(interrupt_period_);
}

[Cutch, 2013-12-13 22:40:17]

https://codereview.chromium.org/109803002/diff/230001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/109803002/diff/230001/runtime/bin/file.cc#new...
runtime/bin/file.cc:34: Dart_DisableThreadInterrupter();
I'm not sure how to build a macro like that in a portable way. For now I'm going
to explicitly add the ThreadSignalBlocker calls.

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/file_androi...
File runtime/bin/file_android.cc (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/file_androi...
runtime/bin/file_android.cc:71: return TEMP_FAILURE_RETRY(read(handle_->fd(),
buffer, num_bytes));
On 2013/12/13 21:29:14, siva wrote:
> We should consider moving this blocking stuff into TEMP_FAILURE_RETRY using a
> new macro (I made that comment in a previous version of your upload).

Not sure how to do that. For now I'm going with ThreadSignalBlocker calls.

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/process.cc
File runtime/bin/process.cc (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/process.cc#...
runtime/bin/process.cc:213: Dart_ShutdownThreadInterrupter();
On 2013/12/13 21:29:14, siva wrote:
> Could we just call Dart_Cleanup() here, that would make things more
consistent?

Done.

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/signal_bloc...
File runtime/bin/signal_blocker.h (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/signal_bloc...
runtime/bin/signal_blocker.h:26: pthread_sigmask(SIG_BLOCK, &signal_mask, &old);
On 2013/12/13 21:29:14, siva wrote:
> int result = pthread_sigmask(....);
> ASSERT(!retval);

Done.

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/signal_bloc...
runtime/bin/signal_blocker.h:31: pthread_sigmask(SIG_SETMASK, &old, NULL);
On 2013/12/13 21:29:14, siva wrote:
> Ditto here.

Done.

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/socket.cc
File runtime/bin/socket.cc (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/bin/socket.cc#n...
runtime/bin/socket.cc:17: #include "include/dart_native_api.h"
On 2013/12/13 21:29:14, siva wrote:
> Leftover from a previous version, can be removed.

Done.

https://codereview.chromium.org/109803002/diff/270001/runtime/include/dart_na...
File runtime/include/dart_native_api.h (right):

https://codereview.chromium.org/109803002/diff/270001/runtime/include/dart_na...
runtime/include/dart_native_api.h:148: DART_EXPORT void
Dart_ShutdownThreadInterrupter();
On 2013/12/13 21:29:14, siva wrote:
> If we called Dart_Cleanup in process exit this would not be needed?

Done.

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

https://codereview.chromium.org/109803002/diff/270001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:66: if (!FLAG_thread_interrupter) {
On 2013/12/13 21:29:14, siva wrote:
> If FLAG_profiler is false will FLAG_thread_interrupter also be false
> and similarly if FLAG_thread_interrupter is false will FLAG_profiler be false?
> 
> Should they just be one flag.

Eventually.

https://codereview.chromium.org/109803002/diff/270001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:183: ml.Notify();
On 2013/12/13 21:29:14, siva wrote:
> Is this notify necessary? This thread will get ticked on the next round right.

Done.

https://codereview.chromium.org/109803002/diff/270001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:196: ml.Notify();
On 2013/12/13 21:29:14, siva wrote:
> Ditto question.

Done.

https://codereview.chromium.org/109803002/diff/270001/runtime/vm/thread_inter...
runtime/vm/thread_interrupter.cc:383: }
On 2013/12/13 21:29:14, siva wrote:
> Could this be:
> 
> MonitorLocker loop(monitor_);
> while (!shutdown) {
>   ....
>   ....
>   loop.Wait(interrupt_period_);
> }

Done.

[Cutch, 2013-12-16 17:03:55]

Latest patch set removes the separate thread interrupter flag, adds the
TEMP_FAILURE_RETRY_BLOCK_SIGNALS macro, and only writes the json data file when
--profile-dir is specified.

[Cutch, 2013-12-16 18:52:37]

Committed patchset #25 manually as r31170 (presubmit successful).