Sampling profiler

Sampling profiler

BUG=4350
R=asiva@google.com

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

[Cutch, 2013-10-24 14:07:30]

PTAL (windows work pending my return)

[siva, 2013-10-28 05:19:20]

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/utils_linux.cc
File runtime/bin/utils_linux.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/utils_linux.c...
runtime/bin/utils_linux.cc:105: int64_t delta = now - start;
ASSERT(delta >= 0);

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/utils_macos.cc
File runtime/bin/utils_macos.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/utils_macos.c...
runtime/bin/utils_macos.cc:107: int64_t delta = now - start;
Ditto.

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/vmservice.h
File runtime/bin/vmservice.h (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/vmservice.h#n...
runtime/bin/vmservice.h:18: 
extra blank line?

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread.h
File runtime/platform/thread.h (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread.h...
runtime/platform/thread.h:44: static void GetThreadCPUUsage(int64_t* cpu_usage);
maybe GetThreadCpuUsage to avoid UU ?

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_l...
File runtime/platform/thread_linux.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_l...
runtime/platform/thread_linux.cc:69: }
This seems a repeat of the code above why not replace the code above
with ComputeTimeSpecMicros(ts, millis * 1000);

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_l...
runtime/platform/thread_linux.cc:311: if (micros == 0) {
should be if (micros == dart::Monitor::kNoTimeout)

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_l...
runtime/platform/thread_linux.cc:322: }
else {
  VALIDATE_PTHREAD_RESULT(result); ?
}

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_m...
File runtime/platform/thread_macos.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_m...
runtime/platform/thread_macos.cc:292: if (micros == 0) {
ditto comment about using the named constant.

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_m...
runtime/platform/thread_macos.cc:309: }
else {
  VALIDATE_PTHREAD_RESULT(result);
}

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_w...
File runtime/platform/thread_win.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_w...
runtime/platform/thread_win.cc:102: *cpu_usage = 0;
UNIMPLEMENTED();

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

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:676: #endif
Why is this #ifdefed out.

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

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:64: 
extra blank line?

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:372: Mutex* profiler_data_mutex() const { return
profiler_data_mutex_; }
ASSERT(profiler_data_mutex_ != NULL);

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/native_symbol.cc
File runtime/vm/native_symbol.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/native_symbol....
runtime/vm/native_symbol.cc:99: Mutex* NativeSymbolResolver::lock_ = NULL;
Why not have files
native_symbol_windows.cc
native_symbol_linux.cc
native_symbol_macos.cc
native_symbol_android.cc
and avoid these #ifdef stuff

also running_ and lock_ could be static variables only inside
native_symbol_windows.cc

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/object_test.cc...
runtime/vm/object_test.cc:21: return lib.raw();
why not just
return Library::New(library_name);

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

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:38: int walk(uintptr_t pc_, uintptr_t fp_) {
why underscrore for these param names?

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:82: uintptr_t stack_upper_;
DISALLOW stuff

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:117: Isolate* isolate = isolates_[i];
ASSERT(isolate != NULL);

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:141: void
ProfilerManager::FreeIsolateProfilingData(Isolate* isolate) {
why not move the lock here
ScopedMutexLock profiler_data_lock(isolate->profiler_data_mutex());

and remove it from the other two call points.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:174: // TODO(johnmccutchan): Make real use of vm_tags and
runtime_tags.
open an issue and use the issue number in the TODO.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:204: uintptr_t stack_upper = stack_lower +
isolate->GetSpecifiedStackSize();
There is an implicit assumption here that the stack is growing to lower
addresses.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:212: }
Not sure I understand the logic here.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:311: isolates_[i] = temp;
why do they have to be swapped why not just
isolates_[i] = isolates_[last];

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:336: // the increment in the for loop.
This is tricky, why not replace the for loop with a while loop and
increment the counter only if there is no sampling

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:378: #endif
I would prefer if we moved this code to indiviual OS files instead of the #ifdef
stuff. I understand that there will be some duplication amongst linux/macos and
android but that is how we have done it for all the stuff.
We are always worried about subtle changes that may have to be done for these
flavors and pretty soon there will be more #ifdefs....

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

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:16: 
extra blank line?

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:24: static void ShutdownIsolate(Isolate* isolate);
should be ShutdownIsolateProfiling

ShutdownIsolate sounds like the isolate is being shut down.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:49: static void ThreadMain(uword parameters);
DISALLOW_COPY stuff?

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:75: int64_t TimeUntilExpiration(int64_t current_time) {
maybe call it current_time_in_micros?

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:115: SampleBuffer* sample_buffer_;
DISALLOW stuff.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:159: intptr_t WrapIncrement(intptr_t i) const;
DISALLOW_ stuff.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:162: 
extra blank line?

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/signal_handler.cc
File runtime/vm/signal_handler.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/signal_handler...
runtime/vm/signal_handler.cc:95: 
Ditto comment about using individual files for each os

[Cutch, 2013-11-04 20:36:04]

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/utils_linux.cc
File runtime/bin/utils_linux.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/utils_linux.c...
runtime/bin/utils_linux.cc:105: int64_t delta = now - start;
On 2013/10/28 05:19:21, siva wrote:
> ASSERT(delta >= 0);

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/utils_macos.cc
File runtime/bin/utils_macos.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/utils_macos.c...
runtime/bin/utils_macos.cc:107: int64_t delta = now - start;
On 2013/10/28 05:19:21, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/vmservice.h
File runtime/bin/vmservice.h (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/bin/vmservice.h#n...
runtime/bin/vmservice.h:18: 
On 2013/10/28 05:19:21, siva wrote:
> extra blank line?

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread.h
File runtime/platform/thread.h (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread.h...
runtime/platform/thread.h:44: static void GetThreadCPUUsage(int64_t* cpu_usage);
On 2013/10/28 05:19:21, siva wrote:
> maybe GetThreadCpuUsage to avoid UU ?

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_l...
File runtime/platform/thread_linux.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_l...
runtime/platform/thread_linux.cc:69: }
On 2013/10/28 05:19:21, siva wrote:
> This seems a repeat of the code above why not replace the code above
> with ComputeTimeSpecMicros(ts, millis * 1000);

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_l...
runtime/platform/thread_linux.cc:311: if (micros == 0) {
On 2013/10/28 05:19:21, siva wrote:
> should be if (micros == dart::Monitor::kNoTimeout)

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_l...
runtime/platform/thread_linux.cc:322: }
On 2013/10/28 05:19:21, siva wrote:
> else {
>   VALIDATE_PTHREAD_RESULT(result); ?
> }

The above ASSERT((result == 0) || (result == ETIMEDOUT));

Handles this.

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_m...
File runtime/platform/thread_macos.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_m...
runtime/platform/thread_macos.cc:292: if (micros == 0) {
On 2013/10/28 05:19:21, siva wrote:
> ditto comment about using the named constant.

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_m...
runtime/platform/thread_macos.cc:309: }
On 2013/10/28 05:19:21, siva wrote:
> else {
>   VALIDATE_PTHREAD_RESULT(result);
> }

Above assert handles this.

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_w...
File runtime/platform/thread_win.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/platform/thread_w...
runtime/platform/thread_win.cc:102: *cpu_usage = 0;
On 2013/10/28 05:19:21, siva wrote:
> UNIMPLEMENTED();

Done, but will be implemented soon.

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

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:676: #endif
On 2013/10/28 05:19:21, siva wrote:
> Why is this #ifdefed out.

Debug only dumping of data in about:tracing format.

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

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:64: 
On 2013/10/28 05:19:21, siva wrote:
> extra blank line?

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:372: Mutex* profiler_data_mutex() const { return
profiler_data_mutex_; }
On 2013/10/28 05:19:21, siva wrote:
> ASSERT(profiler_data_mutex_ != NULL);

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/native_symbol.cc
File runtime/vm/native_symbol.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/native_symbol....
runtime/vm/native_symbol.cc:99: Mutex* NativeSymbolResolver::lock_ = NULL;
On 2013/10/28 05:19:21, siva wrote:
> Why not have files
> native_symbol_windows.cc
> native_symbol_linux.cc
> native_symbol_macos.cc
> native_symbol_android.cc
> and avoid these #ifdef stuff
> 
> also running_ and lock_ could be static variables only inside
> native_symbol_windows.cc

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/object_test.cc...
runtime/vm/object_test.cc:21: return lib.raw();
On 2013/10/28 05:19:21, siva wrote:
> why not just
> return Library::New(library_name);

Done.

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

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:38: int walk(uintptr_t pc_, uintptr_t fp_) {
On 2013/10/28 05:19:21, siva wrote:
> why underscrore for these param names?

Cleaned up.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:82: uintptr_t stack_upper_;
On 2013/10/28 05:19:21, siva wrote:
> DISALLOW stuff

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:117: Isolate* isolate = isolates_[i];
On 2013/10/28 05:19:21, siva wrote:
> ASSERT(isolate != NULL);

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:141: void
ProfilerManager::FreeIsolateProfilingData(Isolate* isolate) {
On 2013/10/28 05:19:21, siva wrote:
> why not move the lock here
> ScopedMutexLock profiler_data_lock(isolate->profiler_data_mutex());
> 
> and remove it from the other two call points.

I've added a TODO. I need to think about this again when I'm not jet lagged.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:174: // TODO(johnmccutchan): Make real use of vm_tags and
runtime_tags.
On 2013/10/28 05:19:21, siva wrote:
> open an issue and use the issue number in the TODO.

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:204: uintptr_t stack_upper = stack_lower +
isolate->GetSpecifiedStackSize();
On 2013/10/28 05:19:21, siva wrote:
> There is an implicit assumption here that the stack is growing to lower
> addresses.

Yes. When would this not be true?

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:212: }
On 2013/10/28 05:19:21, siva wrote:
> Not sure I understand the logic here.

I'm trying to get the address space bounds for the isolate's stack. I've moved
this logic into:

Isolate::GetStackBounds(uintptr_t* lower, uintptr_t* upper);

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:311: isolates_[i] = temp;
On 2013/10/28 05:19:21, siva wrote:
> why do they have to be swapped why not just
> isolates_[i] = isolates_[last];

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:336: // the increment in the for loop.
On 2013/10/28 05:19:21, siva wrote:
> This is tricky, why not replace the for loop with a while loop and
> increment the counter only if there is no sampling

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.cc#ne...
runtime/vm/profiler.cc:378: #endif
On 2013/10/28 05:19:21, siva wrote:
> I would prefer if we moved this code to indiviual OS files instead of the
#ifdef
> stuff. I understand that there will be some duplication amongst linux/macos
and
> android but that is how we have done it for all the stuff.
> We are always worried about subtle changes that may have to be done for these
> flavors and pretty soon there will be more #ifdefs....

Agreed and Done.

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

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:16: 
On 2013/10/28 05:19:21, siva wrote:
> extra blank line?

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:24: static void ShutdownIsolate(Isolate* isolate);
On 2013/10/28 05:19:21, siva wrote:
> should be ShutdownIsolateProfiling
> 
> ShutdownIsolate sounds like the isolate is being shut down.

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:49: static void ThreadMain(uword parameters);
On 2013/10/28 05:19:21, siva wrote:
> DISALLOW_COPY stuff?

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:75: int64_t TimeUntilExpiration(int64_t current_time) {
On 2013/10/28 05:19:21, siva wrote:
> maybe call it current_time_in_micros?

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:115: SampleBuffer* sample_buffer_;
On 2013/10/28 05:19:21, siva wrote:
> DISALLOW stuff.

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:159: intptr_t WrapIncrement(intptr_t i) const;
On 2013/10/28 05:19:21, siva wrote:
> DISALLOW_ stuff.

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/profiler.h#new...
runtime/vm/profiler.h:162: 
On 2013/10/28 05:19:21, siva wrote:
> extra blank line?

Done.

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/signal_handler.cc
File runtime/vm/signal_handler.cc (right):

https://codereview.chromium.org/25909002/diff/64001/runtime/vm/signal_handler...
runtime/vm/signal_handler.cc:95: 
On 2013/10/28 05:19:21, siva wrote:
> Ditto comment about using individual files for each os 

Done.

[siva, 2013-11-11 03:51:53]

LGTM with some comments.

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread.h
File runtime/platform/thread.h (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread....
runtime/platform/thread.h:94: class ScopedMutexLock {
Should we just call this ScopedMutex ?

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread....
runtime/platform/thread.h:112: class ScopedMonitorLock {
Ditto about calling this ScopedMonitor...

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread_...
File runtime/platform/thread_android.cc (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread_...
runtime/platform/thread_android.cc:288: }
We need a Monitor::WaitMicros implementation here ?

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

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:714: // ProfilerManager::WriteTracing(this, this->name(),
this->main_port());
why is this commented out?

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

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/isolate.h#new...
runtime/vm/isolate.h:463: Mutex* profiler_data_mutex_;
can the mutex be a value object instead?

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

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:53: isolates_size_ = 0;
a free of isolates_ also needs to be done here?

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:106: ScopedMutexLock
profiler_data_lock(isolate->profiler_data_mutex());
Maybe later you should consider adding a locking order value to the mutexes and
ensure that this locking order is always maintained.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:154: realloc(isolates_, sizeof(isolate) * new_capacity));
we need to ensure that this does not overflow, maybe we need a limit for the
number of isolates we will support.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:159: void ProfilerManager::AddIsolate(Isolate* isolate) {
you should add a comment here and in FindIsolate, RemoveIsolate that the
monitor_ lock needs to be held when these functions are called.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:509: return cursor != 0;
Seems simplistic...

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

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:16: class JSONStream;
Add comment:
// Forward declarations.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:62: int64_t sample_interval() const { return
sample_interval_micros_; }
normally we have been naming the setters and accessors to match the field name
if they are simple accessor/setters, e.g:

int64_t sample_internal_micros() const { return sample.... }

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:68: bool CanExpire() {
const?

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:72: bool ShouldSample(int64_t current_time) {
const

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:76: int64_t TimeUntilExpiration(int64_t
current_time_micros) {
const, maybe there are others too in these new classes.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:93: int64_t set_and_delta_cpu_usage(int64_t cpu_usage) {
ComputeDeltaAndSetCpuUsage(int64_t cpu_usage) ?

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_andr...
File runtime/vm/profiler_android.cc (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_andr...
runtime/vm/profiler_android.cc:63: // the increment in the for loop.
Seems hacky why not use a while loop
while (i < isolates_size_) {
   ....
   .....
   conditionally increment i;
}

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_linu...
File runtime/vm/profiler_linux.cc (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_linu...
runtime/vm/profiler_linux.cc:88: // the increment in the for loop.
Ditto comment here.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_maco...
File runtime/vm/profiler_macos.cc (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_maco...
runtime/vm/profiler_macos.cc:86: // the increment in the for loop.
Ditto comment.

https://codereview.chromium.org/25909002/diff/284001/tools/gyp/configurations...
File tools/gyp/configurations_make.gypi (right):

https://codereview.chromium.org/25909002/diff/284001/tools/gyp/configurations...
tools/gyp/configurations_make.gypi:29: # '-fvisibility-inlines-hidden',
Will this result in all  the VM symbols now being visible to the embedder?

[siva, 2013-11-11 21:41:23]

Adding Zach to the cc list so he could look at the Android stuff.

[zra, 2013-11-11 22:26:14]

On 2013/11/11 21:41:23, siva wrote:
> Adding Zach to the cc list so he could look at the Android stuff.

Of the things that are implemented for Android, there's no reason they should be
any different from Linux, but I'll patch these changes in to double check that
it builds. Because of unimplemented signal handler functions for ARM/MIPS and
Android, I won't be able to run the code, though.

[zra, 2013-11-11 22:34:29]

On 2013/11/11 22:26:14, zra wrote:
> On 2013/11/11 21:41:23, siva wrote:
> > Adding Zach to the cc list so he could look at the Android stuff.
> 
> Of the things that are implemented for Android, there's no reason they should
be
> any different from Linux, but I'll patch these changes in to double check that
> it builds. Because of unimplemented signal handler functions for ARM/MIPS and
> Android, I won't be able to run the code, though.

Getting lots of:

../../dart/runtime/vm/isolate.h:282:3: error: 'Random' does not name a type

Did I not apply the patch correctly?

[siva, 2013-11-12 16:01:55]

One question regarding dartium integration, will this work inside chrome?

I remember there were some restrictions with respect to signal handling and
signals in general, don't remember the details.

[Cutch, 2013-11-18 20:48:54]

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread.h
File runtime/platform/thread.h (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread....
runtime/platform/thread.h:94: class ScopedMutexLock {
On 2013/11/11 03:51:54, siva wrote:
> Should we just call this ScopedMutex ?

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread....
runtime/platform/thread.h:112: class ScopedMonitorLock {
On 2013/11/11 03:51:54, siva wrote:
> Ditto about calling this ScopedMonitor...

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread_...
File runtime/platform/thread_android.cc (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/platform/thread_...
runtime/platform/thread_android.cc:288: }
On 2013/11/11 03:51:54, siva wrote:
> We need a Monitor::WaitMicros implementation here ?

Done.

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

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:714: // ProfilerManager::WriteTracing(this, this->name(),
this->main_port());
On 2013/11/11 03:51:54, siva wrote:
> why is this commented out?

Was for debugging. Removed.

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

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/isolate.h#new...
runtime/vm/isolate.h:463: Mutex* profiler_data_mutex_;
On 2013/11/11 03:51:54, siva wrote:
> can the mutex be a value object instead?

Done.

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

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:53: isolates_size_ = 0;
On 2013/11/11 03:51:54, siva wrote:
> a free of isolates_ also needs to be done here?

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:154: realloc(isolates_, sizeof(isolate) * new_capacity));
On 2013/11/11 03:51:54, siva wrote:
> we need to ensure that this does not overflow, maybe we need a limit for the
> number of isolates we will support.

Done. For now I just assert we aren't monitoring more than 4K isolates. In the
future we may need to spin up a new ProfilerManager to spread the load.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:159: void ProfilerManager::AddIsolate(Isolate* isolate) {
On 2013/11/11 03:51:54, siva wrote:
> you should add a comment here and in FindIsolate, RemoveIsolate that the
> monitor_ lock needs to be held when these functions are called.

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:509: return cursor != 0;
On 2013/11/11 03:51:54, siva wrote:
> Seems simplistic...

Done.

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

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:16: class JSONStream;
On 2013/11/11 03:51:54, siva wrote:
> Add comment:
> // Forward declarations.

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:62: int64_t sample_interval() const { return
sample_interval_micros_; }
On 2013/11/11 03:51:54, siva wrote:
> normally we have been naming the setters and accessors to match the field name
> if they are simple accessor/setters, e.g:
> 
> int64_t sample_internal_micros() const { return sample.... }

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:68: bool CanExpire() {
On 2013/11/11 03:51:54, siva wrote:
> const? 

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:72: bool ShouldSample(int64_t current_time) {
On 2013/11/11 03:51:54, siva wrote:
> const

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:76: int64_t TimeUntilExpiration(int64_t
current_time_micros) {
On 2013/11/11 03:51:54, siva wrote:
> const, maybe there are others too in these new classes.

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:93: int64_t set_and_delta_cpu_usage(int64_t cpu_usage) {
On 2013/11/11 03:51:54, siva wrote:
> ComputeDeltaAndSetCpuUsage(int64_t cpu_usage) ?

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_andr...
File runtime/vm/profiler_android.cc (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_andr...
runtime/vm/profiler_android.cc:63: // the increment in the for loop.
On 2013/11/11 03:51:54, siva wrote:
> Seems hacky why not use a while loop
> while (i < isolates_size_) {
>    ....
>    .....
>    conditionally increment i;
> }

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_linu...
File runtime/vm/profiler_linux.cc (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_linu...
runtime/vm/profiler_linux.cc:88: // the increment in the for loop.
On 2013/11/11 03:51:54, siva wrote:
> Ditto comment here.

Done.

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_maco...
File runtime/vm/profiler_macos.cc (right):

https://codereview.chromium.org/25909002/diff/284001/runtime/vm/profiler_maco...
runtime/vm/profiler_macos.cc:86: // the increment in the for loop.
On 2013/11/11 03:51:54, siva wrote:
> Ditto comment.

Done.

https://codereview.chromium.org/25909002/diff/284001/tools/gyp/configurations...
File tools/gyp/configurations_make.gypi (right):

https://codereview.chromium.org/25909002/diff/284001/tools/gyp/configurations...
tools/gyp/configurations_make.gypi:29: # '-fvisibility-inlines-hidden',
On 2013/11/11 03:51:54, siva wrote:
> Will this result in all  the VM symbols now being visible to the embedder?

Yes, although the embedder could always build Dart however they want.

[Cutch, 2013-11-19 18:26:41]

Committed patchset #29 manually as r30419 (presubmit successful).