Process v8 sampling data into the trace model.

Process v8 sampling data into the trace model.

This CL updates the importer to handle sampling data coming from
v8. The data is munged into the format needed by the sample and
stack frame format used by tracing.

BUG=#1123

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/e8848956680e76ff264038671aa4705f55ddd6ae

[dsinclair, 2015-08-07 15:54:03]

Patchset #2 (id:10001) has been deleted

[dsinclair, 2015-08-10 15:20:13]

Patchset #2 (id:20001) has been deleted

[dsinclair, 2015-08-10 15:20:18]

Patchset #1 (id:1) has been deleted

[dsinclair, 2015-08-11 17:06:16]

Patchset #2 (id:60001) has been deleted

[dsinclair, 2015-08-11 17:06:22]

Patchset #1 (id:40001) has been deleted

[dsinclair, 2015-08-11 20:51:37]

Patchset #2 (id:100001) has been deleted

[dsinclair, 2015-08-11 20:51:42]

Patchset #1 (id:80001) has been deleted

[dsinclair, 2015-08-11 20:52:47]

dsinclair@chromium.org changed reviewers:
+ nduca@chromium.org, petrcermak@chromium.org

[dsinclair, 2015-08-11 20:52:47]

PTAL.

[nduca, 2015-08-12 00:22:09]

lgtm

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:410: // V8 JIT events
are logged as phase 'I' so we need to separate them out
can we file a chromium bug to rename them and assign to devtools team, and list
the bug here?

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:410: // V8 JIT events
are logged as phase 'I' so we need to separate them out
hoist to its own function?

[petrcermak, 2015-08-12 10:53:24]

A couple of comments.

Thanks,
Petr

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_code_entry.html (right):

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_code_entry.html:15: * @fileoverview
TraceCodeEntry is a wrapper around the V8 CodeEntry to extract
nit: s/to extract/which extracts/ ?

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_code_entry.html:20: function
TraceCodeEntry(size, name, script_id) {
It seems that we generally use camel case for functionArgumentNames and
localVariableNames. Could you please update this patch accordingly?

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_code_entry.html:44: method_name =
method_name.replace(/\s+native\s*$/, '');
nit: No need for '\s*' in the regex as you check that the method name ends with
' native' (i.e. it cannot contain any trailing spaces)

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:424: // We won't know
the name in if we try to insert the code here.
nit: I don't understand what you mean by "the name in". Do you mean "the name
inside it" ("it" being the "original block")?

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:431: //
TODO(dsinclair): There are _a lot_ of these so I'm skipping the
nit: What do you mean by "these"? "V8 JIT events"?

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:432: //   display for
now. Can revisit later if we want to show them.
nit: It seems to me
(https://code.google.com/p/chromium/codesearch#search/&q=file:catapult%20%5C/%...
that we don't indent lines in TODO comments:

// TODO(name): First line of the to do comment,
// second line of the to do comment.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:488: undefined,
'v8-root-stack-frame',
Could you please add inline comments to the literals explaining which arguments
they correspond to (e.g. 'undefined /* cpu */') like you do on line 558?

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:494: for (var i = 0; i
< stackFrame.children.length; i++) {
Unless you do this for performance reasons, you could use tr.b.findFirstInArray:

function findChildWithEntryID(stackFrame, entryID) {
  return tr.b.findFirstInArray(stackFrame.children, function(child) {
    return child.entryID === entryID;
  });
}

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:495: if
(stackFrame.children[i].entryID == entryID)
===

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:503:
this.v8ProcessCodeMaps_[event.pid]) {
Could you perhaps add a short comment explaining what the two branches of the
if-statement do (or one comment at the beginning of each branch)?

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:509: data.stack =
data.stack.reverse();
According to
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...,
Array.prototype.reverse modifies the array in place so there is no need for the
assignment.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:510: for (var i in
data.stack) {
I think that for ... in ... over arrays is generally discouraged in JS. Any
reason why you don't use a regular for loop?

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:544: var childFrame =
findChildWithEntryID(lastStackFrame, entry.id);
Lines 522-535 and 544-555 seem identical (bar one extra blank line). Could they
perhaps be factored out into a local function?

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:693: var keys =
Object.keys(this.v8ProcessRootStackFrame_);
nit: this is the same as:

tr.b.iterItems(this.v8ProcessRootStackFrame_, function(name, frame) {
  frame.removeAllChildren();
});

It seems that you could also use:

for (var key in this.v8ProcessRootStackFrame_)
  this.v8ProcessRootStackFrame_[key].removeAllChildren();

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/model/...
File tracing/tracing/model/stack_frame.html (right):

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/model/...
tracing/tracing/model/stack_frame.html:36: 
personal taste (feel free to ignore): I would drop the blank line here (as it
visually breaks the if-statement into two blocks).

[dsinclair, 2015-08-13 14:55:33]

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_code_entry.html (right):

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_code_entry.html:15: * @fileoverview
TraceCodeEntry is a wrapper around the V8 CodeEntry to extract
On 2015/08/12 10:53:24, petrcermak wrote:
> nit: s/to extract/which extracts/ ?

I think to was also correct, but changed to that for clarity.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_code_entry.html:20: function
TraceCodeEntry(size, name, script_id) {
On 2015/08/12 10:53:24, petrcermak wrote:
> It seems that we generally use camel case for functionArgumentNames and
> localVariableNames. Could you please update this patch accordingly?

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_code_entry.html:44: method_name =
method_name.replace(/\s+native\s*$/, '');
On 2015/08/12 10:53:24, petrcermak wrote:
> nit: No need for '\s*' in the regex as you check that the method name ends
with
> ' native' (i.e. it cannot contain any trailing spaces)

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:410: // V8 JIT events
are logged as phase 'I' so we need to separate them out
On 2015/08/12 00:22:09, nduca wrote:
> can we file a chromium bug to rename them and assign to devtools team, and
list
> the bug here?

Rename which, the V8 events? I think JitCodeAdded and JitCodeMoved are the
correct names and I think it's correct for them to be instant events.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:410: // V8 JIT events
are logged as phase 'I' so we need to separate them out
On 2015/08/12 00:22:09, nduca wrote:
> hoist to its own function?

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:424: // We won't know
the name in if we try to insert the code here.
On 2015/08/12 10:53:24, petrcermak wrote:
> nit: I don't understand what you mean by "the name in". Do you mean "the name
> inside it" ("it" being the "original block")?

Done. s/in//

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:431: //
TODO(dsinclair): There are _a lot_ of these so I'm skipping the
On 2015/08/12 10:53:24, petrcermak wrote:
> nit: What do you mean by "these"? "V8 JIT events"?

Done. Yes, was implying the jit code events that I'm handling in this block.
Clarified.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:432: //   display for
now. Can revisit later if we want to show them.
On 2015/08/12 10:53:24, petrcermak wrote:
> nit: It seems to me
>
(https://code.google.com/p/chromium/codesearch#search/&q=file:catapult%20%5C/%...
> that we don't indent lines in TODO comments:
> 
> // TODO(name): First line of the to do comment,
> // second line of the to do comment.

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:488: undefined,
'v8-root-stack-frame',
On 2015/08/12 10:53:24, petrcermak wrote:
> Could you please add inline comments to the literals explaining which
arguments
> they correspond to (e.g. 'undefined /* cpu */') like you do on line 558?

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:494: for (var i = 0; i
< stackFrame.children.length; i++) {
On 2015/08/12 10:53:24, petrcermak wrote:
> Unless you do this for performance reasons, you could use
tr.b.findFirstInArray:
> 
> function findChildWithEntryID(stackFrame, entryID) {
>   return tr.b.findFirstInArray(stackFrame.children, function(child) {
>     return child.entryID === entryID;
>   });
> }

I did it for copy-pasta reasons. This is from the v8 importer.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:495: if
(stackFrame.children[i].entryID == entryID)
On 2015/08/12 10:53:24, petrcermak wrote:
> ===

Acknowledged.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:503:
this.v8ProcessCodeMaps_[event.pid]) {
On 2015/08/12 10:53:24, petrcermak wrote:
> Could you perhaps add a short comment explaining what the two branches of the
> if-statement do (or one comment at the beginning of each branch)?

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:509: data.stack =
data.stack.reverse();
On 2015/08/12 10:53:24, petrcermak wrote:
> According to
>
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...,
> Array.prototype.reverse modifies the array in place so there is no need for
the
> assignment.

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:510: for (var i in
data.stack) {
On 2015/08/12 10:53:24, petrcermak wrote:
> I think that for ... in ... over arrays is generally discouraged in JS. Any
> reason why you don't use a regular for loop?

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:544: var childFrame =
findChildWithEntryID(lastStackFrame, entry.id);
On 2015/08/12 10:53:24, petrcermak wrote:
> Lines 522-535 and 544-555 seem identical (bar one extra blank line). Could
they
> perhaps be factored out into a local function?

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:693: var keys =
Object.keys(this.v8ProcessRootStackFrame_);
On 2015/08/12 10:53:24, petrcermak wrote:
> nit: this is the same as:
> 
> tr.b.iterItems(this.v8ProcessRootStackFrame_, function(name, frame) {
>   frame.removeAllChildren();
> });
> 
> It seems that you could also use:
> 
> for (var key in this.v8ProcessRootStackFrame_)
>   this.v8ProcessRootStackFrame_[key].removeAllChildren();

Done.

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/model/...
File tracing/tracing/model/stack_frame.html (right):

https://codereview.chromium.org/1276003004/diff/120001/tracing/tracing/model/...
tracing/tracing/model/stack_frame.html:36: 
On 2015/08/12 10:53:24, petrcermak wrote:
> personal taste (feel free to ignore): I would drop the blank line here (as it
> visually breaks the if-statement into two blocks).

Done.

[petrcermak, 2015-08-13 15:15:57]

LGTM with a few more comments.

Thanks,
Petr

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_code_entry.html (right):

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_code_entry.html:16: * extract extra
context information for each item. This includes things like
nit: s/extract/extracts/ (one wrapper)

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:482: var stack = [];
nit: It seems to me that there is no need to declare this variable until before
line 523.

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:494: var
findChildWithEntryID = function(stackFrame, entryID) {
nit: I think that the following is preferred:

function findChildWithEntryId(...) {
  ...
}

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:500: var addStackFrame
= function(lastStackFrame, entry) {
ditto

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:519: // Some of these
types have stacks and some dones, we handle those two
s/dones/don't/ ?

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:521: // we synthesize
one base on the type of thing that's happening so when
nit: s/base/based/

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:525:
this.v8ProcessCodeMaps_[event.pid]) {
nit: I think that this will fit on the previous line.

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:533: for (var i = 0; i
< data.stack.length; i++) {
Observation: I've just realized that this function is a simple map:

data.stack.reverse();
stack = data.stack.map(function(item) {
  ...
}, this);

[dsinclair, 2015-08-13 15:43:38]

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_code_entry.html (right):

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_code_entry.html:16: * extract extra
context information for each item. This includes things like
On 2015/08/13 15:15:56, petrcermak wrote:
> nit: s/extract/extracts/ (one wrapper)

Done.

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:482: var stack = [];
On 2015/08/13 15:15:56, petrcermak wrote:
> nit: It seems to me that there is no need to declare this variable until
before
> line 523.

Actually, I don't need this at all anymore. I was using it as a temporary but
returned the thing I was assigning it back too.

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:494: var
findChildWithEntryID = function(stackFrame, entryID) {
On 2015/08/13 15:15:56, petrcermak wrote:
> nit: I think that the following is preferred:
> 
> function findChildWithEntryId(...) {
>   ...
> }

Done.

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:500: var addStackFrame
= function(lastStackFrame, entry) {
On 2015/08/13 15:15:56, petrcermak wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:519: // Some of these
types have stacks and some dones, we handle those two
On 2015/08/13 15:15:56, petrcermak wrote:
> s/dones/don't/ ?

I .... don't know what that was supposed to be ..... Fixed.

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:521: // we synthesize
one base on the type of thing that's happening so when
On 2015/08/13 15:15:56, petrcermak wrote:
> nit: s/base/based/

Done.

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:525:
this.v8ProcessCodeMaps_[event.pid]) {
On 2015/08/13 15:15:56, petrcermak wrote:
> nit: I think that this will fit on the previous line.

Done.

https://codereview.chromium.org/1276003004/diff/160001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:533: for (var i = 0; i
< data.stack.length; i++) {
On 2015/08/13 15:15:56, petrcermak wrote:
> Observation: I've just realized that this function is a simple map:
> 
> data.stack.reverse();
> stack = data.stack.map(function(item) {
>   ...
> }, this);


I actually removed the stack assignments.

[dsinclair, 2015-08-13 15:43:45]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2015-08-13 15:43:45]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org,
petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/1276003004/#ps170001
(title: " ")

[commit-bot: I haz the power, 2015-08-13 15:43:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1276003004/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1276003004/170001

[commit-bot: I haz the power, 2015-08-13 15:48:21]

Committed patchset #4 (id:170001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...