Bug 1211839 - Don't allow off the main thread markers to nest under main thread markers, r=smaug, jsantell
authorVictor Porof <vporof@mozilla.com>
Sat, 24 Oct 2015 17:10:22 +0200
changeset 269940 a5a31b8a5383c1683e73e7e470e2e95b55aff8a6
parent 269939 75cb7b35f0213a448121f15f50511099fb8c58cb
child 269941 594d0213bc5c87f2286ddb8ae486dc3f8e5b9ac5
push id29595
push userkwierso@gmail.com
push dateWed, 28 Oct 2015 23:41:46 +0000
treeherdermozilla-central@769f29c92bb2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, jsantell
bugs1211839
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
devtools/client/performance/modules/logic/waterfall-utils.js
devtools/client/performance/test/unit/test_waterfall-utils-collapse-05.js
devtools/client/performance/test/unit/xpcshell.ini
docshell/base/timeline/AbstractTimelineMarker.cpp
docshell/base/timeline/AbstractTimelineMarker.h
docshell/base/timeline/ConsoleTimelineMarker.h
docshell/base/timeline/EventTimelineMarker.h
docshell/base/timeline/JavascriptTimelineMarker.h
docshell/base/timeline/RestyleTimelineMarker.h
docshell/base/timeline/TimelineMarker.cpp
docshell/base/timeline/TimestampTimelineMarker.h
docshell/base/timeline/WorkerTimelineMarker.h
dom/webidl/ProfileTimelineMarker.webidl
--- a/devtools/client/performance/docs/markers.md
+++ b/devtools/client/performance/docs/markers.md
@@ -2,16 +2,25 @@
 
 ## Common
 
 * DOMHighResTimeStamp start
 * DOMHighResTimeStamp end
 * 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
 
 Triggered when a DOM event occurs, like a click or a keypress.
 
 * unsigned short eventPhase - a number indicating what phase this event is
   in (target, bubbling, capturing, maps to Ci.nsIDOMEvent constants)
 * DOMString type - the type of event, like "keypress" or "click"
--- a/devtools/client/performance/modules/logic/waterfall-utils.js
+++ b/devtools/client/performance/modules/logic/waterfall-utils.js
@@ -27,55 +27,68 @@ function createParentNode (marker) {
 
 /**
  * Collapses markers into a tree-like structure.
  * @param object rootNode
  * @param array markersList
  * @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];
 
     // If this marker type should not be displayed, just skip
     if (!MarkerUtils.isMarkerValid(curr, filter)) {
       continue;
     }
 
     let parentNode = getCurrentParentNode();
     let blueprint = MarkerUtils.getBlueprintFor(curr);
 
     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
     // back into a normal node.
     if (collapsible) {
       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;
     }
 
     // First off, if any parent nodes exist, finish them off
     // 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;
       }
 
       // If this marker is still nestable, but outside of the range
       // of the current parent, iterate upwards on the next parent
       // and finalize the current parent.
@@ -107,36 +120,39 @@ function createParentNodeFactory (root) 
      * Sets the `end` time based on the most recent child if not defined.
      */
     popParentNode: () => {
       if (parentMarkers.length === 0) {
         throw new Error("Cannot pop parent markers when none exist.");
       }
 
       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) {
         lastParent.end = lastParent.submarkers[lastParent.submarkers.length - 1].end;
       }
 
       // 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;
       }
 
       return lastParent;
     },
 
     /**
      * 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.
      */
     pushNode: (parent, marker) => {
       parent.submarkers.push(marker);
 
       // If pushing a parent marker, track it as the top of
new file mode 100644
--- /dev/null
+++ b/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: []
+  }]
+};
--- a/devtools/client/performance/test/unit/xpcshell.ini
+++ b/devtools/client/performance/test/unit/xpcshell.ini
@@ -28,8 +28,9 @@ skip-if = toolkit == 'android' || toolki
 [test_tree-model-12.js]
 [test_tree-model-13.js]
 [test_tree-model-allocations-01.js]
 [test_tree-model-allocations-02.js]
 [test_waterfall-utils-collapse-01.js]
 [test_waterfall-utils-collapse-02.js]
 [test_waterfall-utils-collapse-03.js]
 [test_waterfall-utils-collapse-04.js]
+[test_waterfall-utils-collapse-05.js]
--- a/docshell/base/timeline/AbstractTimelineMarker.cpp
+++ b/docshell/base/timeline/AbstractTimelineMarker.cpp
@@ -1,33 +1,40 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * 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 {
 
 AbstractTimelineMarker::AbstractTimelineMarker(const char* aName,
                                                MarkerTracingType aTracingType)
   : mName(aName)
   , mTracingType(aTracingType)
+  , mProcessType(XRE_GetProcessType())
+  , mIsOffMainThread(!NS_IsMainThread())
 {
   MOZ_COUNT_CTOR(AbstractTimelineMarker);
   SetCurrentTime();
 }
 
 AbstractTimelineMarker::AbstractTimelineMarker(const char* aName,
                                                const TimeStamp& aTime,
                                                MarkerTracingType aTracingType)
   : mName(aName)
   , mTracingType(aTracingType)
+  , mProcessType(XRE_GetProcessType())
+  , mIsOffMainThread(!NS_IsMainThread())
 {
   MOZ_COUNT_CTOR(AbstractTimelineMarker);
   SetCustomTime(aTime);
 }
 
 UniquePtr<AbstractTimelineMarker>
 AbstractTimelineMarker::Clone()
 {
--- a/docshell/base/timeline/AbstractTimelineMarker.h
+++ b/docshell/base/timeline/AbstractTimelineMarker.h
@@ -43,21 +43,27 @@ public:
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) = 0;
   virtual JSObject* GetStack() = 0;
 
   const char* GetName() const { return mName; }
   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);
   void SetCustomTime(DOMHighResTimeStamp aTime);
 };
 
 } // namespace mozilla
 
--- a/docshell/base/timeline/ConsoleTimelineMarker.h
+++ b/docshell/base/timeline/ConsoleTimelineMarker.h
@@ -35,16 +35,18 @@ public:
     // Console markers must have matching causes as well. It is safe to perform
     // a static_cast here as the previous equality check ensures that this is
     // a console marker instance.
     return mCause == static_cast<const ConsoleTimelineMarker*>(&aOther)->mCause;
   }
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     if (GetTracingType() == MarkerTracingType::START) {
       aMarker.mCauseName.Construct(mCause);
     } else {
       aMarker.mEndStack = GetStack();
     }
   }
 
 private:
--- a/docshell/base/timeline/EventTimelineMarker.h
+++ b/docshell/base/timeline/EventTimelineMarker.h
@@ -20,16 +20,18 @@ public:
                                MarkerTracingType aTracingType)
     : TimelineMarker("DOMEvent", aTracingType)
     , mType(aType)
     , mPhase(aPhase)
   {}
 
   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);
     }
   }
 
 private:
   nsString mType;
--- a/docshell/base/timeline/JavascriptTimelineMarker.h
+++ b/docshell/base/timeline/JavascriptTimelineMarker.h
@@ -26,16 +26,18 @@ public:
     , mCause(NS_ConvertUTF8toUTF16(aReason))
     , mFunctionName(aFunctionName)
     , mFileName(aFileName)
     , mLineNumber(aLineNumber)
   {}
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     aMarker.mCauseName.Construct(mCause);
 
     if (!mFunctionName.IsEmpty() || !mFileName.IsEmpty()) {
       dom::RootedDictionary<dom::ProfileTimelineStackFrame> stackFrame(aCx);
       stackFrame.mLine.Construct(mLineNumber);
       stackFrame.mSource.Construct(mFileName);
       stackFrame.mFunctionDisplayName.Construct(mFunctionName);
 
--- a/docshell/base/timeline/RestyleTimelineMarker.h
+++ b/docshell/base/timeline/RestyleTimelineMarker.h
@@ -21,16 +21,18 @@ public:
   {
     if (aRestyleHint) {
       mRestyleHint.AssignWithConversion(RestyleManager::RestyleHintToString(aRestyleHint));
     }
   }
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     if (GetTracingType() == MarkerTracingType::START) {
       aMarker.mRestyleHint.Construct(mRestyleHint);
     }
   }
 
 private:
   nsAutoString mRestyleHint;
 };
--- a/docshell/base/timeline/TimelineMarker.cpp
+++ b/docshell/base/timeline/TimelineMarker.cpp
@@ -23,17 +23,20 @@ TimelineMarker::TimelineMarker(const cha
   : AbstractTimelineMarker(aName, aTime, aTracingType)
 {
   CaptureStackIfNecessary(aTracingType, aStackRequest);
 }
 
 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*
 TimelineMarker::GetStack()
 {
   if (mStackTrace.initialized()) {
     return mStackTrace;
   }
--- a/docshell/base/timeline/TimestampTimelineMarker.h
+++ b/docshell/base/timeline/TimestampTimelineMarker.h
@@ -17,16 +17,18 @@ class TimestampTimelineMarker : public T
 public:
   explicit TimestampTimelineMarker(const nsAString& aCause)
     : TimelineMarker("TimeStamp", MarkerTracingType::TIMESTAMP)
     , mCause(aCause)
   {}
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     if (!mCause.IsEmpty()) {
       aMarker.mCauseName.Construct(mCause);
     }
   }
 
 private:
   nsString mCause;
 };
--- a/docshell/base/timeline/WorkerTimelineMarker.h
+++ b/docshell/base/timeline/WorkerTimelineMarker.h
@@ -25,16 +25,18 @@ public:
   {
     WorkerTimelineMarker* clone = new WorkerTimelineMarker(mOperationType, GetTracingType());
     clone->SetCustomTime(GetTime());
     return UniquePtr<AbstractTimelineMarker>(clone);
   }
 
   virtual void AddDetails(JSContext* aCx, dom::ProfileTimelineMarker& aMarker) override
   {
+    TimelineMarker::AddDetails(aCx, aMarker);
+
     if (GetTracingType() == MarkerTracingType::START) {
       aMarker.mWorkerOperation.Construct(mOperationType);
     }
   }
 
 private:
   ProfileTimelineWorkerOperationType mOperationType;
 };
--- a/dom/webidl/ProfileTimelineMarker.webidl
+++ b/dom/webidl/ProfileTimelineMarker.webidl
@@ -33,16 +33,19 @@ enum ProfileTimelineWorkerOperationType 
 };
 
 dictionary ProfileTimelineMarker {
   DOMString name = "";
   DOMHighResTimeStamp start = 0;
   DOMHighResTimeStamp end = 0;
   object? stack = null;
 
+  unsigned short processType;
+  boolean isOffMainThread;
+
   /* For ConsoleTime, Timestamp and Javascript markers.  */
   DOMString causeName;
 
   /* For ConsoleTime markers.  */
   object? endStack = null;
 
   /* For DOMEvent markers.  */
   DOMString type;