Use VM tag in profile and add stack trace trie

Use VM tag in profile and add stack trace trie

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

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

[Cutch, 2014-03-17 14:37:20]

asiva for the backend changes
turnidge for the frontend

[turnidge, 2014-03-17 21:19:31]

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/elements/isolate_profile.dart (right):

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.dart:146:
sampleRate = (1000000.0 / profile['period'].toDouble()).toStringAsFixed(0);
Why 1000000.0?  Named constant?

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html (right):

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:33:
.tree-mode-switch {
Some comments in the css would help me understand all this a bit better.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:34: position:
relative; width: 121px;
Can you put one property per line?

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:51:
font-size: 14px; color: white; font-family: Trebuchet, Arial, sans-serif;
font-weight: bold;
We use a different font (Montserrat, etc.) above.  Would that look ok here too?

Also, you may want to use the combo "font" property instead of separate weight,
size, family properties.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:55: content:
"Call graph";
I think that injecting content using before/after can make it hard to figure out
how a page is built.  Is this necessary for the operation of your switch?

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:81:
.tag-mode-switch {
Can .tree-mode-switch and .tag-mode-switch (and the related classes) share a
single css class, maybe "mode-switch"?   It looks like there is a lot of
duplication that I don't think is necessary.

In fact, would it be possible to abstract out the mode-switch and put it into a
separate custom element?

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/service/cache.dart (right):

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/service/cache.dart:117: return
_cache['code/tag-0'];
Seems kinda magic.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/service/object.dart (right):

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/service/object.dart:149: CodeTrieNode
_readTrieNode(List<Code> codeTable) {
Consider adding a comment describing the trie data format.

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

https://codereview.chromium.org/201213004/diff/70001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:243: if (child->code_region_index() >
child_code_region_index) {
Just to make sure I understand -- GetChild relies on the children being sorted
by region index but later after we call RecursiveSort they will be sorted by
count.  Do we want to check that children aren't added after RecursiveSort has
been called?

https://codereview.chromium.org/201213004/diff/70001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:1313: build_trie.root()->RecursiveSort();
Maybe rename RecursiveSort to SortByCount?

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

https://codereview.chromium.org/201213004/diff/70001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1317: }
Does it make sense to do any unit testing of this part of the change?

Are you sure you want to accept weird values of "tags" here?  (like
tags=jfdkljkldfjlsd)

[siva, 2014-03-18 00:05:19]

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7373: Isolate* isolate = Isolate::Current();
Please hoist this up as the first line and use
tkns = TokenStream::Handle(isolate, tokens());

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7377: const String& src = String::Handle(Source());
Now that we have grabbed the isolate:
src = String::Handle(isolate, Source());

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7380: private_key)));
Ditto.

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

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:212: 
blank line needed to maintain 2 blank lines.

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:238: for (; i < length; i++) {
might be more readable as:
while (i < length) {
 ...
 ...
 ++i;
}

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:269: void PrintToJSONArray(JSONArray* array) {
void PrintToJSONArray(....) const {

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:287: }
Why not club this accessor with the other accessors above (count,
code_region_index)

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:1157: ASSERT(region->contains(tag));
Can this be:
ASSERT((tag_code_table_->At(index))->contains(tag));

[siva, 2014-03-18 00:05:48]

the VM changes lgtm with some comments.

[Cutch, 2014-03-18 14:39:18]

Todd: I've refactored my switch into a separate element. PTAL.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/elements/isolate_profile.dart (right):

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.dart:146:
sampleRate = (1000000.0 / profile['period'].toDouble()).toStringAsFixed(0);
On 2014/03/17 21:19:31, turnidge wrote:
> Why 1000000.0?  Named constant?

Done.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html (right):

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:33:
.tree-mode-switch {
On 2014/03/17 21:19:31, turnidge wrote:
> Some comments in the css would help me understand all this a bit better.

Done.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:34: position:
relative; width: 121px;
On 2014/03/17 21:19:31, turnidge wrote:
> Can you put one property per line?

Done.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:51:
font-size: 14px; color: white; font-family: Trebuchet, Arial, sans-serif;
font-weight: bold;
On 2014/03/17 21:19:31, turnidge wrote:
> We use a different font (Montserrat, etc.) above.  Would that look ok here
too?
> 
> Also, you may want to use the combo "font" property instead of separate
weight,
> size, family properties.

Done.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/elements/isolate_profile.html:55: content:
"Call graph";
On 2014/03/17 21:19:31, turnidge wrote:
> I think that injecting content using before/after can make it hard to figure
out
> how a page is built.  Is this necessary for the operation of your switch?

Done.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/service/cache.dart (right):

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/service/cache.dart:117: return
_cache['code/tag-0'];
On 2014/03/17 21:19:31, turnidge wrote:
> Seems kinda magic.

Done.

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/service/object.dart (right):

https://codereview.chromium.org/201213004/diff/70001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/service/object.dart:149: CodeTrieNode
_readTrieNode(List<Code> codeTable) {
On 2014/03/17 21:19:31, turnidge wrote:
> Consider adding a comment describing the trie data format.

Done.

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

https://codereview.chromium.org/201213004/diff/70001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:243: if (child->code_region_index() >
child_code_region_index) {
On 2014/03/17 21:19:31, turnidge wrote:
> Just to make sure I understand -- GetChild relies on the children being sorted
> by region index but later after we call RecursiveSort they will be sorted by
> count.  Do we want to check that children aren't added after RecursiveSort has
> been called?

You understand correctly. I've added comments about the restrictions on calling
order.

https://codereview.chromium.org/201213004/diff/70001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:1313: build_trie.root()->RecursiveSort();
On 2014/03/17 21:19:31, turnidge wrote:
> Maybe rename RecursiveSort to SortByCount?

Done.

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

https://codereview.chromium.org/201213004/diff/70001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1317: }
On 2014/03/17 21:19:31, turnidge wrote:
> Does it make sense to do any unit testing of this part of the change?
> 
> Are you sure you want to accept weird values of "tags" here?  (like
> tags=jfdkljkldfjlsd)

Added a unit test for the profile command and an error case when the tags option
value isn't "hide".

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7373: Isolate* isolate = Isolate::Current();
On 2014/03/18 00:05:20, siva wrote:
> Please hoist this up as the first line and use
> tkns = TokenStream::Handle(isolate, tokens());

Done.

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7377: const String& src = String::Handle(Source());
On 2014/03/18 00:05:20, siva wrote:
> Now that we have grabbed the isolate:
> src = String::Handle(isolate, Source());

Done.

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/object.cc#new...
runtime/vm/object.cc:7380: private_key)));
On 2014/03/18 00:05:20, siva wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:212: 
On 2014/03/18 00:05:20, siva wrote:
> blank line needed to maintain 2 blank lines.

Done.

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:238: for (; i < length; i++) {
On 2014/03/18 00:05:20, siva wrote:
> might be more readable as:
> while (i < length) {
>  ...
>  ...
>  ++i;
> }

Done.

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:269: void PrintToJSONArray(JSONArray* array) {
On 2014/03/18 00:05:20, siva wrote:
> void PrintToJSONArray(....) const {

Done.

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:287: }
On 2014/03/18 00:05:20, siva wrote:
> Why not club this accessor with the other accessors above (count,
> code_region_index)

Done.

https://codereview.chromium.org/201213004/diff/80001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:1157: ASSERT(region->contains(tag));
On 2014/03/18 00:05:20, siva wrote:
> Can this be:
> ASSERT((tag_code_table_->At(index))->contains(tag));

Done.

[turnidge, 2014-03-18 20:06:17]

lgtm

[Cutch, 2014-03-18 21:20:55]

Committed patchset #9 manually as r34064 (presubmit successful).