Process v8 sampling data into the trace model.

tracing/tracing/extras/importer/trace_event_importer.html

Index: tracing/tracing/extras/importer/trace_event_importer.html
diff --git a/tracing/tracing/extras/importer/trace_event_importer.html b/tracing/tracing/extras/importer/trace_event_importer.html
index 6e24d4ac0866eb3fad6c2f5bbdfea7075e3e0f15..9080267565038abad8c2de708a9ef6ad6907d705 100644
--- a/tracing/tracing/extras/importer/trace_event_importer.html
+++ b/tracing/tracing/extras/importer/trace_event_importer.html
@@ -5,11 +5,12 @@ Use of this source code is governed by a BSD-style license that can be
 found in the LICENSE file.
 -->
 
-<link rel="import" href="/tracing/ui/base/color_scheme.html">
 <link rel="import" href="/tracing/base/quad.html">
 <link rel="import" href="/tracing/base/range.html">
 <link rel="import" href="/tracing/base/utils.html">
 <link rel="import" href="/tracing/base/units/time.html">
+<link rel="import" href="/tracing/extras/importer/trace_code_entry.html">
+<link rel="import" href="/tracing/extras/importer/v8/codemap.html">
 <link rel="import" href="/tracing/importer/importer.html">
 <link rel="import" href="/tracing/model/attribute.html">
 <link rel="import" href="/tracing/model/comment_box_annotation.html">
@@ -23,6 +24,7 @@ found in the LICENSE file.
 <link rel="import" href="/tracing/model/rect_annotation.html">
 <link rel="import" href="/tracing/model/x_marker_annotation.html">
 <link rel="import" href="/tracing/model/model.html">
+<link rel="import" href="/tracing/ui/base/color_scheme.html">
 
 <script>
 'use strict';
@@ -32,9 +34,7 @@ found in the LICENSE file.
  * into the provided model.
  */
 tr.exportTo('tr.e.importer', function() {
-
   var Importer = tr.importer.Importer;
-
   var deepCopy = tr.b.deepCopy;
 
   function getEventColor(event, opt_customName) {
@@ -59,11 +59,16 @@ tr.exportTo('tr.e.importer', function() {
     this.battorData_ = undefined;
     this.eventsWereFromString_ = false;
     this.softwareMeasuredCpuCount_ = undefined;
+
     this.allAsyncEvents_ = [];
     this.allFlowEvents_ = [];
     this.allObjectEvents_ = [];
+
     this.traceEventSampleStackFramesByName_ = {};
 
+    this.v8ProcessCodeMaps_ = {};
+    this.v8ProcessRootStackFrame_ = {};
+
     // Dump ID -> {global: (event | undefined), process: [events]}
     this.allMemoryDumpEvents_ = {};
 
@@ -402,6 +407,32 @@ tr.exportTo('tr.e.importer', function() {
     },
 
     processInstantEvent: function(event) {
+      // V8 JIT events are logged as phase 'I' so we need to separate them out

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

can we file a chromium bug to rename them and assign to devtools team, and list
the bug here?

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

hoist to its own function?

[dsinclair, 2015/08/13 14:55:32]

Done.

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

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.

+      // and handle specially.
+      if (event.name === 'JitCodeAdded' || event.name === 'JitCodeMoved') {
+        if (this.v8ProcessCodeMaps_[event.pid] === undefined)
+          this.v8ProcessCodeMaps_[event.pid] = new tr.e.importer.v8.CodeMap();
+        var map = this.v8ProcessCodeMaps_[event.pid];
+
+        var data = event.args.data;
+        var start_addr = parseInt(data.code_start, 16);
+        if (event.name === 'JitCodeMoved') {
+          try {
+            map.moveCode(start_addr, parseInt(data.new_code_start, 16));
+          } catch (err) {
+            // Ignore moved error. Means we didn't find the original block.
+            // We won't know the name in if we try to insert the code here.

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

nit: I don't understand what you mean by "the name in". Do you mean "the name
inside it" ("it" being the "original block")?

[dsinclair, 2015/08/13 14:55:32]

Done. s/in//

+          }
+        } else {
+          map.addCode(start_addr,
+              new tr.e.importer.TraceCodeEntry(data.code_len, data.name));
+        }
+
+        // TODO(dsinclair): There are _a lot_ of these so I'm skipping the

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

nit: What do you mean by "these"? "V8 JIT events"?

[dsinclair, 2015/08/13 14:55:32]

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

+        //   display for now. Can revisit later if we want to show them.

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

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.

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

Done.

+        return;
+      }
+
       // Thread-level instant events are treated as zero-duration slices.
       if (event.s == 't' || event.s === undefined) {
         this.processDurationEvent(event);
@@ -447,6 +478,92 @@ tr.exportTo('tr.e.importer', function() {
       var thread = this.model_.getOrCreateProcess(event.pid)
         .getOrCreateThread(event.tid);
 
+      if (event.name === 'V8Sample') {
+        var stack = [];
+        var data = event.args.data;
+
+        var rootStackFrame = this.v8ProcessRootStackFrame_[event.pid];
+        if (!rootStackFrame) {
+          rootStackFrame = new tr.model.StackFrame(
+              undefined, 'v8-root-stack-frame',

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

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?

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

Done.

+              'v8', 'v8-root-stack-frame', 0);
+          this.v8ProcessRootStackFrame_[event.pid] = rootStackFrame;
+        }
+
+        function findChildWithEntryID(stackFrame, entryID) {
+          for (var i = 0; i < stackFrame.children.length; i++) {

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

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;
  });
}

[dsinclair, 2015/08/13 14:55:32]

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

+            if (stackFrame.children[i].entryID == entryID)

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

===

[dsinclair, 2015/08/13 14:55:32]

Acknowledged.

+              return stackFrame.children[i];
+          }
+          return undefined;
+        }
+
+        var lastStackFrame = rootStackFrame;
+        if (data.stack.length > 0 &&
+            this.v8ProcessCodeMaps_[event.pid]) {

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

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)?

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

Done.

+
+          var map = this.v8ProcessCodeMaps_[event.pid];
+
+          // Stacks have the leaf node first, flip them around so the root
+          // comes first.
+          data.stack = data.stack.reverse();

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

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.

[dsinclair, 2015/08/13 14:55:32]

Done.

+          for (var i in data.stack) {

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

I think that for ... in ... over arrays is generally discouraged in JS. Any
reason why you don't use a regular for loop?

[dsinclair, 2015/08/13 14:55:32]

Done.

+            var addr = parseInt(data.stack[i], 16);
+            var entry = map.findEntry(addr);
+
+            if (entry === null) {
+              entry = {
+                id: 'unknown',
+                name: 'unknown',
+                sourceInfo: undefined
+              };
+            }
+
+            var childFrame = findChildWithEntryID(lastStackFrame, entry.id);
+            if (!childFrame) {
+              lastStackFrame = new tr.model.StackFrame(
+                  lastStackFrame, tr.b.GUID.allocate(),
+                  'v8', entry.name,
+                  tr.ui.b.getColorIdForGeneralPurposeString(entry.name),
+                  entry.sourceInfo);
+
+              lastStackFrame.entryID = entry.id;
+
+              this.model_.addStackFrame(lastStackFrame);
+            } else {
+              lastStackFrame = childFrame;
+            }
+
+            stack.push(entry);
+          }
+        } else {
+            var entry = {
+              id: data.vm_state,
+              name: data.vm_state
+            };
+            var childFrame = findChildWithEntryID(lastStackFrame, entry.id);

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

Lines 522-535 and 544-555 seem identical (bar one extra blank line). Could they
perhaps be factored out into a local function?

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

Done.

+            if (!childFrame) {
+              lastStackFrame = new tr.model.StackFrame(
+                  lastStackFrame, tr.b.GUID.allocate(),
+                  'v8', entry.name,
+                  tr.ui.b.getColorIdForGeneralPurposeString(entry.name));
+              lastStackFrame.entryID = entry.id;
+
+              this.model_.addStackFrame(lastStackFrame);
+            } else {
+              lastStackFrame = childFrame;
+            }
+        }
+
+        var sample = new tr.model.Sample(
+            undefined /* cpu */, thread, 'V8 Sample',
+            timestampFromUs(event.ts), lastStackFrame, 1 /* weight */,
+            this.deepCopyIfNeeded_(event.args));
+        this.model_.samples.push(sample);
+
+        return;
+      }
+
       var stackFrame = this.getStackFrameForEvent_(event);
       if (stackFrame === undefined) {
         stackFrame = this.traceEventSampleStackFramesByName_[
@@ -463,7 +580,7 @@ tr.exportTo('tr.e.importer', function() {
       }
 
       var sample = new tr.model.Sample(
-          undefined, thread, 'TRACE_EVENT_SAMPLE',
+          undefined, thread, 'Trace Event Sample',
           timestampFromUs(event.ts), stackFrame, 1,
           this.deepCopyIfNeeded_(event.args));
       this.model_.samples.push(sample);
@@ -570,6 +687,12 @@ tr.exportTo('tr.e.importer', function() {
           });
         }
       }
+
+      // Remove all the root stack frame children as they should
+      // already be added.
+      var keys = Object.keys(this.v8ProcessRootStackFrame_);

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

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();

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

Done.

+      for (var i = 0; i < keys.length; i++)
+        this.v8ProcessRootStackFrame_[keys[i]].removeAllChildren();
     },
 
     importStackFrames_: function() {