Bug 1211839 - Don't allow off the main thread markers to nest under main thread markers, r=smaug, jsantell

devtools/client/performance/docs/markers.md

@@ -7,6 +7,15 @@
 * DOMString name
 * object? stack
 * object? endStack
+* unsigned short processType;
+* boolean isOffMainThread;
+
+The `processType` a GeckoProcessType enum listed in xpcom/build/nsXULAppAPI.h,
+specifying if this marker originates in a content, chrome, plugin etc. process.
+Additionally, markers may be created from any thread on those processes, and
+`isOffMainThread` highights whether or not they're from the main thread. The most
+common type of marker is probably going to be from a GeckoProcessType_Content's
+main thread when debugging content.
 
 ## DOMEvent
 

devtools/client/performance/modules/logic/waterfall-utils.js

@@ -32,7 +32,11 @@ function createParentNode (marker) {
  * @param array filter
  */
 function collapseMarkersIntoNode({ rootNode, markersList, filter }) {
-  let { getCurrentParentNode, pushNode, popParentNode } = createParentNodeFactory(rootNode);
+  let {
+    getCurrentParentNode,
+    pushNode,
+    popParentNode
+  } = createParentNodeFactory(rootNode);
 
   for (let i = 0, len = markersList.length; i < len; i++) {
     let curr = markersList[i];
@@ -48,7 +52,7 @@ function collapseMarkersIntoNode({ rootNode, markersList, filter }) {
     let nestable = "nestable" in blueprint ? blueprint.nestable : true;
     let collapsible = "collapsible" in blueprint ? blueprint.collapsible : true;
 
-    let finalized = null;
+    let finalized = false;
 
     // If this marker is collapsible, turn it into a parent marker.
     // If there are no children within it later, it will be turned
@@ -57,9 +61,14 @@ function collapseMarkersIntoNode({ rootNode, markersList, filter }) {
       curr = createParentNode(curr);
     }
 
-    // If not nestible, just push it inside the root node,
-    // like console.time/timeEnd.
-    if (!nestable) {
+    // If not nestible, just push it inside the root node. Additionally,
+    // markers originating outside the main thread are considered to be
+    // "never collapsible", to avoid confusion.
+    // A beter solution would be to collapse every marker with its siblings
+    // from the same thread, but that would require a thread id attached
+    // to all markers, which is potentially expensive and rather useless at
+    // the moment, since we don't really have that many OTMT markers.
+    if (!nestable || curr.isOffMainThread) {
       pushNode(rootNode, curr);
       continue;
     }
@@ -68,9 +77,13 @@ function collapseMarkersIntoNode({ rootNode, markersList, filter }) {
     // recursively upwards if this marker is outside their ranges and nestable.
     while (!finalized && parentNode) {
       // If this marker is eclipsed by the current parent marker,
-      // make it a child of the current parent and stop
-      // going upwards.
-      if (nestable && curr.end <= parentNode.end) {
+      // make it a child of the current parent and stop going upwards.
+      // If the markers aren't from the same process, attach them to the root
+      // node as well. Every process has its own main thread.
+      if (nestable &&
+          curr.start >= parentNode.start &&
+          curr.end <= parentNode.end &&
+          curr.processType == parentNode.processType) {
         pushNode(parentNode, curr);
         finalized = true;
         break;
@@ -112,6 +125,7 @@ function createParentNodeFactory (root) {
       }
 
       let lastParent = parentMarkers.pop();
+
       // If this finished parent marker doesn't have an end time,
       // so probably a synthesized marker, use the last marker's end time.
       if (lastParent.end == void 0) {
@@ -119,7 +133,7 @@ function createParentNodeFactory (root) {
       }
 
       // If no children were ever pushed into this parent node,
-      // remove it's submarkers so it behaves like a non collapsible
+      // remove its submarkers so it behaves like a non collapsible
       // node.
       if (!lastParent.submarkers.length) {
         delete lastParent.submarkers;
@@ -131,7 +145,9 @@ function createParentNodeFactory (root) {
     /**
      * Returns the most recent parent node.
      */
-    getCurrentParentNode: () => parentMarkers.length ? parentMarkers[parentMarkers.length - 1] : null,
+    getCurrentParentNode: () => parentMarkers.length
+      ? parentMarkers[parentMarkers.length - 1]
+      : null,
 
     /**
      * Push this marker into the most recent parent node.

devtools/client/performance/test/unit/test_waterfall-utils-collapse-05.js

@@ -0,0 +1,163 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Tests if the waterfall collapsing logic works properly
+ * when dealing with OTMT markers.
+ */
+
+function run_test() {
+  run_next_test();
+}
+
+add_task(function test() {
+  const WaterfallUtils = require("devtools/client/performance/modules/logic/waterfall-utils");
+
+  let rootMarkerNode = WaterfallUtils.createParentNode({ name: "(root)" });
+
+  WaterfallUtils.collapseMarkersIntoNode({
+    rootNode: rootMarkerNode,
+    markersList: gTestMarkers
+  });
+
+  compare(rootMarkerNode, gExpectedOutput);
+
+  function compare (marker, expected) {
+    for (let prop in expected) {
+      if (prop === "submarkers") {
+        for (let i = 0; i < expected.submarkers.length; i++) {
+          compare(marker.submarkers[i], expected.submarkers[i]);
+        }
+      } else if (prop !== "uid") {
+        equal(marker[prop], expected[prop], `${expected.name} matches ${prop}`);
+      }
+    }
+  }
+});
+
+const gTestMarkers = [
+  { start: 1, end: 4, name: "A1-mt", processType: 1, isOffMainThread: false },
+  // This should collapse only under A1-mt
+  { start: 2, end: 3, name: "B1", processType: 1, isOffMainThread: false },
+  // This should never collapse.
+  { start: 2, end: 3, name: "C1", processType: 1, isOffMainThread: true },
+
+  { start: 5, end: 8, name: "A1-otmt", processType: 1, isOffMainThread: true },
+  // This should collapse only under A1-mt
+  { start: 6, end: 7, name: "B2", processType: 1, isOffMainThread: false },
+  // This should never collapse.
+  { start: 6, end: 7, name: "C2", processType: 1, isOffMainThread: true },
+
+  { start: 9, end: 12, name: "A2-mt", processType: 2, isOffMainThread: false },
+  // This should collapse only under A2-mt
+  { start: 10, end: 11, name: "D1", processType: 2, isOffMainThread: false },
+  // This should never collapse.
+  { start: 10, end: 11, name: "E1", processType: 2, isOffMainThread: true },
+
+  { start: 13, end: 16, name: "A2-otmt", processType: 2, isOffMainThread: true },
+  // This should collapse only under A2-mt
+  { start: 14, end: 15, name: "D2", processType: 2, isOffMainThread: false },
+  // This should never collapse.
+  { start: 14, end: 15, name: "E2", processType: 2, isOffMainThread: true },
+
+  // This should not collapse, because there's no parent in this process.
+  { start: 14, end: 15, name: "F", processType: 3, isOffMainThread: false },
+
+  // This should never collapse.
+  { start: 14, end: 15, name: "G", processType: 3, isOffMainThread: true },
+];
+
+const gExpectedOutput = {
+  name: "(root)",
+  submarkers: [{
+    start: 1,
+    end: 4,
+    name: "A1-mt",
+    processType: 1,
+    isOffMainThread: false,
+    submarkers: [{
+      start: 2,
+      end: 3,
+      name: "B1",
+      processType: 1,
+      isOffMainThread: false
+    }]
+  }, {
+    start: 2,
+    end: 3,
+    name: "C1",
+    processType: 1,
+    isOffMainThread: true
+  }, {
+    start: 5,
+    end: 8,
+    name: "A1-otmt",
+    processType: 1,
+    isOffMainThread: true,
+    submarkers: [{
+      start: 6,
+      end: 7,
+      name: "B2",
+      processType: 1,
+      isOffMainThread: false
+    }]
+  }, {
+    start: 6,
+    end: 7,
+    name: "C2",
+    processType: 1,
+    isOffMainThread: true
+  }, {
+    start: 9,
+    end: 12,
+    name: "A2-mt",
+    processType: 2,
+    isOffMainThread: false,
+    submarkers: [{
+      start: 10,
+      end: 11,
+      name: "D1",
+      processType: 2,
+      isOffMainThread: false
+    }]
+  }, {
+    start: 10,
+    end: 11,
+    name: "E1",
+    processType: 2,
+    isOffMainThread: true
+  }, {
+    start: 13,
+    end: 16,
+    name: "A2-otmt",
+    processType: 2,
+    isOffMainThread: true,
+    submarkers: [{
+      start: 14,
+      end: 15,
+      name: "D2",
+      processType: 2,
+      isOffMainThread: false
+    }]
+  }, {
+    start: 14,
+    end: 15,
+    name: "E2",
+    processType: 2,
+    isOffMainThread: true
+  }, {
+    start: 14,
+    end: 15,
+    name: "F",
+    processType: 3,
+    isOffMainThread: false,
+    submarkers: []
+  }, {
+    start: 14,
+    end: 15,
+    name: "G",
+    processType: 3,
+    isOffMainThread: true,
+    submarkers: []
+  }]
+};

devtools/client/performance/test/unit/xpcshell.ini

@@ -33,3 +33,4 @@ skip-if = toolkit == 'android' || toolkit == 'gonk'
 [test_waterfall-utils-collapse-02.js]
 [test_waterfall-utils-collapse-03.js]
 [test_waterfall-utils-collapse-04.js]
+[test_waterfall-utils-collapse-05.js]

docshell/base/timeline/AbstractTimelineMarker.cpp

@@ -5,7 +5,10 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "AbstractTimelineMarker.h"
+
 #include "mozilla/TimeStamp.h"
+#include "MainThreadUtils.h"
+#include "nsAppRunner.h"
 
 namespace mozilla {
 
@@ -13,6 +16,8 @@ AbstractTimelineMarker::AbstractTimelineMarker(const char* aName,
                                                MarkerTracingType aTracingType)
   : mName(aName)
   , mTracingType(aTracingType)
+  , mProcessType(XRE_GetProcessType())
+  , mIsOffMainThread(!NS_IsMainThread())
 {
   MOZ_COUNT_CTOR(AbstractTimelineMarker);
   SetCurrentTime();
@@ -23,6 +28,8 @@ AbstractTimelineMarker::AbstractTimelineMarker(const char* aName,
                                                MarkerTracingType aTracingType)
   : mName(aName)
   , mTracingType(aTracingType)
+  , mProcessType(XRE_GetProcessType())
+  , mIsOffMainThread(!NS_IsMainThread())
 {
   MOZ_COUNT_CTOR(AbstractTimelineMarker);
   SetCustomTime(aTime);

docshell/base/timeline/AbstractTimelineMarker.h

@@ -48,11 +48,17 @@ public:
   DOMHighResTimeStamp GetTime() const { return mTime; }
   MarkerTracingType GetTracingType() const { return mTracingType; }
 
+  const uint8_t GetProcessType() const { return mProcessType; };
+  const bool IsOffMainThread() const { return mIsOffMainThread; };
+
 private:
   const char* mName;
   DOMHighResTimeStamp mTime;
   MarkerTracingType mTracingType;
 
+  uint8_t mProcessType; // @see `enum GeckoProcessType`.
+  bool mIsOffMainThread;
+
 protected:
   void SetCurrentTime();
   void SetCustomTime(const TimeStamp& aTime);

docshell/base/timeline/ConsoleTimelineMarker.h

@@ -40,6 +40,8 @@ public:
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     if (GetTracingType() == MarkerTracingType::START) {
       aMarker.mCauseName.Construct(mCause);
     } else {

docshell/base/timeline/EventTimelineMarker.h

@@ -25,6 +25,8 @@ public:
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     if (GetTracingType() == MarkerTracingType::START) {
       aMarker.mType.Construct(mType);
       aMarker.mEventPhase.Construct(mPhase);

docshell/base/timeline/JavascriptTimelineMarker.h

@@ -31,6 +31,8 @@ public:
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     aMarker.mCauseName.Construct(mCause);
 
     if (!mFunctionName.IsEmpty() || !mFileName.IsEmpty()) {

docshell/base/timeline/RestyleTimelineMarker.h

@@ -26,6 +26,8 @@ public:
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     if (GetTracingType() == MarkerTracingType::START) {
       aMarker.mRestyleHint.Construct(mRestyleHint);
     }

docshell/base/timeline/TimelineMarker.cpp

@@ -28,7 +28,10 @@ TimelineMarker::TimelineMarker(const char* aName,
 void
 TimelineMarker::AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker)
 {
-  // Nothing to do here for plain markers.
+  if (GetTracingType() == MarkerTracingType::START) {
+    aMarker.mProcessType.Construct(GetProcessType());
+    aMarker.mIsOffMainThread.Construct(IsOffMainThread());
+  }
 }
 
 JSObject*

docshell/base/timeline/TimestampTimelineMarker.h

@@ -22,6 +22,8 @@ public:
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     if (!mCause.IsEmpty()) {
       aMarker.mCauseName.Construct(mCause);
     }

docshell/base/timeline/WorkerTimelineMarker.h

@@ -30,6 +30,8 @@ public:
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     if (GetTracingType() == MarkerTracingType::START) {
       aMarker.mWorkerOperation.Construct(mOperationType);
     }

dom/webidl/ProfileTimelineMarker.webidl

@@ -38,6 +38,9 @@ dictionary ProfileTimelineMarker {
   DOMHighResTimeStamp end = 0;
   object? stack = null;
 
+  unsigned short processType;
+  boolean isOffMainThread;
+
   /* For ConsoleTime, Timestamp and Javascript markers.  */
   DOMString causeName;