Expose allocation tracing over service protocol

Expose allocation tracing over service protocol

BUG=
R=turnidge@google.com

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

[Cutch, 2015-07-13 18:12:07]

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

[turnidge, 2015-07-13 23:37:49]

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/service/object.dart:2001: return
isolate.invokeRpc('_getCpuProfile', params);
Calling this _getCpuProfile doesn't seem right.  Perhaps this should get its own
rpc.

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
File runtime/observatory/tests/service/get_cpu_profile_test.dart (right):

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/get_cpu_profile_test.dart:32:
hasStoppedAtBreakpoint,
Can you add blank lines between tests?  it helps me to parse it visually.

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/get_cpu_profile_test.dart:43:
hasStoppedAtBreakpoint,
It looks to me like this resume/breakpoint is unnecessary -- there is no need to
advance program state to check that setTraceAllocations works.

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/get_cpu_profile_test.dart:56: // Step.
Calling this Step is inaccurate, since we aren't stepping.

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/get_cpu_profile_test.dart:70: CpuProfile
cpuProfile = new CpuProfile();
I think we should change the name CpuProfile to something else now.  Not sure
what, maybe TreeProfile, CallTreeProfile, SampleProfile, ...

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
File runtime/observatory/tests/service/test_helper.dart (right):

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/test_helper.dart:199: Future<Class>
getRootClass(Isolate isolate, String className) async {
Maybe getClassFromRootLib since I don't know what a root class means?

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:267: intptr_t* service_id) {
The word Directory here bothers me some.

How about changing the function name, maybe to something like
GetPrefixedIntegerId?  And directory -> prefix.  Also, instead of requiring the
/, how about the function just knows to expect a / separator?

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:287: static bool ValidateClassId(Isolate* isolate,
intptr_t cid) {
Change name to IsValidClassId?

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:301: }
I would like you to get rid of the overloaded versions of ValidateClassId and
GetClassForId that take a const char*, as they encourage duplicate parsing.  See
the note in GetCpuProfile, below.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:315: ASSERT(GetIntegerIdFromDirectory(class_id,
"classes/", &cid));
Do ASSERTs get removed in release builds?  I don't think that required
functionality should be in an ASSERT.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:2301: static bool GetCpuProfile(Isolate* isolate,
JSONStream* js) {
As mentioned elsewhere, consider separating cpu and class allocation profiling
rpcs or rename this to something more general.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:2306: const Class& cls =
Class::Handle(GetClassForId(isolate, class_id));
You are double-parsing the class id, once in ValidateClassId and once in
GetClassForId.

I would rather see something like this:

intptr_t cid = GetPrefixedIntegerId(..., "classes", class_id);
if (IsValidClassId(cid) {
  const Class& cls = Class::Handle(GetClassForId(isolate, cid);
}

Similarly in TraceClassAllocation

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:2934: { "_traceClassAllocation", TraceClassAllocation,
Consider changing rpc name for consistency with setLibraryDebuggable -- perhaps
setTraceClassAllocation?

[Cutch, 2015-07-14 15:53:48]

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/service/object.dart:2001: return
isolate.invokeRpc('_getCpuProfile', params);
On 2015/07/13 23:37:48, turnidge wrote:
> Calling this _getCpuProfile doesn't seem right.  Perhaps this should get its
own
> rpc.

_getAllocationSamples

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
File runtime/observatory/tests/service/get_cpu_profile_test.dart (right):

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/get_cpu_profile_test.dart:32:
hasStoppedAtBreakpoint,
On 2015/07/13 23:37:48, turnidge wrote:
> Can you add blank lines between tests?  it helps me to parse it visually.

Done.

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/get_cpu_profile_test.dart:43:
hasStoppedAtBreakpoint,
On 2015/07/13 23:37:48, turnidge wrote:
> It looks to me like this resume/breakpoint is unnecessary -- there is no need
to
> advance program state to check that setTraceAllocations works.

There is an issue between debugger stepping and allocation tracing- I need to
step twice (or allocate twice) in order for the allocation stub code to switch
over.

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/get_cpu_profile_test.dart:56: // Step.
On 2015/07/13 23:37:48, turnidge wrote:
> Calling this Step is inaccurate, since we aren't stepping.

Done.

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/get_cpu_profile_test.dart:70: CpuProfile
cpuProfile = new CpuProfile();
On 2015/07/13 23:37:48, turnidge wrote:
> I think we should change the name CpuProfile to something else now.  Not sure
> what, maybe TreeProfile, CallTreeProfile, SampleProfile, ...

I agree. The CPU profile is slowly becoming a general stack trace sampler. I've
added a TODO (in cpu_profile.dart) because doing the rename right will touch
lots of files (and rename some files).

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
File runtime/observatory/tests/service/test_helper.dart (right):

https://codereview.chromium.org/1231603008/diff/1/runtime/observatory/tests/s...
runtime/observatory/tests/service/test_helper.dart:199: Future<Class>
getRootClass(Isolate isolate, String className) async {
On 2015/07/13 23:37:49, turnidge wrote:
> Maybe getClassFromRootLib since I don't know what a root class means?

Done.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:267: intptr_t* service_id) {
On 2015/07/13 23:37:49, turnidge wrote:
> The word Directory here bothers me some.
> 
> How about changing the function name, maybe to something like
> GetPrefixedIntegerId?  And directory -> prefix.  Also, instead of requiring
the
> /, how about the function just knows to expect a / separator?

Done.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:287: static bool ValidateClassId(Isolate* isolate,
intptr_t cid) {
On 2015/07/13 23:37:49, turnidge wrote:
> Change name to IsValidClassId?

Done.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:301: }
On 2015/07/13 23:37:49, turnidge wrote:
> I would like you to get rid of the overloaded versions of ValidateClassId and
> GetClassForId that take a const char*, as they encourage duplicate parsing. 
See
> the note in GetCpuProfile, below.

Done.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:315: ASSERT(GetIntegerIdFromDirectory(class_id,
"classes/", &cid));
On 2015/07/13 23:37:49, turnidge wrote:
> Do ASSERTs get removed in release builds?  I don't think that required
> functionality should be in an ASSERT.

Good catch.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:2301: static bool GetCpuProfile(Isolate* isolate,
JSONStream* js) {
On 2015/07/13 23:37:49, turnidge wrote:
> As mentioned elsewhere, consider separating cpu and class allocation profiling
> rpcs or rename this to something more general.

Done.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:2306: const Class& cls =
Class::Handle(GetClassForId(isolate, class_id));
On 2015/07/13 23:37:49, turnidge wrote:
> You are double-parsing the class id, once in ValidateClassId and once in
> GetClassForId.
> 
> I would rather see something like this:
> 
> intptr_t cid = GetPrefixedIntegerId(..., "classes", class_id);
> if (IsValidClassId(cid) {
>   const Class& cls = Class::Handle(GetClassForId(isolate, cid);
> }
> 
> Similarly in TraceClassAllocation

Done.

https://codereview.chromium.org/1231603008/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:2934: { "_traceClassAllocation", TraceClassAllocation,
On 2015/07/13 23:37:49, turnidge wrote:
> Consider changing rpc name for consistency with setLibraryDebuggable --
perhaps
> setTraceClassAllocation?

Done.

[turnidge, 2015-07-14 18:34:20]

lgtm w/ 2 minor comments.

https://codereview.chromium.org/1231603008/diff/20001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_allocation_samples_test.dart (right):

https://codereview.chromium.org/1231603008/diff/20001/runtime/observatory/tes...
runtime/observatory/tests/service/get_allocation_samples_test.dart:44:
hasStoppedAtBreakpoint,
Delete this?

https://codereview.chromium.org/1231603008/diff/20001/runtime/observatory/tes...
runtime/observatory/tests/service/get_allocation_samples_test.dart:60: // over.
Say that this is a bug, but low priority...

[Cutch, 2015-07-14 18:42:04]

Committed patchset #3 (id:40001) manually as
a92a3c8af3cae5751d2f8eed5a1bd1631ec3b4de (presubmit successful).