When a marker is fully eclipsed by another marker, it should be collapsed under it. Chrome does this similarly. Whenever a marker's start and end time is after and before another marker respectively, it should be nested within it.

Part 1, move markers into its own "global"

Comment on attachment 8613280 [details] [diff] [review] 1169439-move-marker-global.patch Review of attachment 8613280 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/modules/logic/marker-utils.js @@ +278,5 @@ > }; > > +/** > + * A series of collapsers used by the blueprint. These functions are > + * consecutively invoked on a moving window of two markers. Nit: now that I read this again, I realize that the term "consecutively" is misleading. Remove it.

Comment on attachment 8613280 [details] [diff] [review] 1169439-move-marker-global.patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=562bce00ed3c

This is almost working. Wanted to run by you the folding mechanisms here, changed a bit from what they were to handle more scenarios. This makes each marker added to a parent responsible for closing, and has multiple depths capable. The DOM/JS folding was removed in place for parent folding (serves the same purpose), but we could readd the capability to merge these if we wanted. The consecutive identical folding removed for now while fixing up these tests, but can re-add, but I don't like the idea of having the same UI/viz for children markers nested in an eclipsing marker as a "meta" marker that represents several consecutive markers

Probably with identical/parent-child folding combined is the `either` function only handles the first one that returns something. Probably should change that into merging results to handle putting a marker under a parent, while folding with adjacent, if we want that. Another thing that's a bit weird here, is something like console.time/End ranges, as those are arbitrary -- should pretty much be a standalone thing, not interuppting the current marker collapsing.

Children fully eclipsed become collapsed under a parent now. The consecutive markers collapsing into a meta marker should be handled in bug 1169887 -- I think that's more of a UX issue on how to handle that. https://treeherder.mozilla.org/#/jobs?repo=try&revision=db266f814224

Bug 1170386 and bug 1169887 are about the consecutive label folding being very misleading -- 2 bugs in the handful of days it's been live, I think it's better to keep it removed until we have a solution.

Comment on attachment 8613660 [details] [diff] [review] 1169439-2.patch Review of attachment 8613660 [details] [diff] [review]: ----------------------------------------------------------------- Cool. Please make sure a followup is filed about coalescing similar markers together. I understand the criticism against it, but it's all about the UI and how this is presented, and not the actual idea of showing multiple identical consecutive markers *on a single row*. The problem is showing a full bar, suggesting it's a single marker. ::: browser/devtools/performance/modules/logic/marker-utils.js @@ +291,5 @@ > + * marker, becoming a parent itself, or make a new marker-esque > + * object. > + * - collapse: Whether or not this current marker should be nested within > + * the current parent. > + * - end: Whether or not the current parent should be finalized and popped Naming this "end" is confusing. Is that an end marker? The end time of a marker? A suggestion? A verb? How about "finalize" or "finish" instead? @@ +301,4 @@ > // If there is a parent marker currently being filled and the current marker > // should go into the parent marker, make it so. > if (parent && parent.name == curr.name) { > + let end = next && next.name !== curr.name; Rename "end". @@ +311,4 @@ > } > }, > > adjacent: function (parent, curr, peek) { Nit: make sure either all or no methods have a jsdoc. @@ +326,4 @@ > let next = peek(1); > + // If this marker is consumed by current parent, collapse > + if (parent && curr.end <= parent.end) { > + let end = next && next.end > parent.end; Rename "end". ::: browser/devtools/performance/modules/logic/waterfall-utils.js @@ +39,2 @@ > if (collapseInfo) { > + let { collapse, toParent, end } = collapseInfo; Rename "end". @@ +70,5 @@ > +function makeParentMarkerNode (marker) { > + let node = Object.create(null); > + for (let prop in marker) { > + node[prop] = marker[prop]; > + } Why not use Cu.cloneInto instead? @@ +80,3 @@ > } > > +function createParentNodeFactory (root) { Nit: jsdoc. @@ +101,5 @@ > + (factory.getCurrentParentNode() || root).submarkers.push(parentMarker); > + console.log("Pushing Parent ", parentMarker, "to", factory.getCurrentParentNode()||root); > + parentMarkers.push(parentMarker); > + }, > + collapseMarker: (marker) => { Nit: add documentation for all those methods, and possibly a short explanation about how all of this is supposed to work. ::: browser/devtools/performance/modules/markers.js @@ +34,5 @@ > + * object. > + * - collapse: Whether or not this current marker should be nested within > + * the current parent. > + * - end: Whether or not the current parent should be finalized and popped > + * off the stack. Rename "end".

Comments addressed; landing: https://hg.mozilla.org/integration/fx-team/rev/8e4fdb45e6a8 can address consecutive markers in bug 1169887

sorry had to back this out since this caused a lot of test failures in dt tests like https://treeherder.mozilla.org/logviewer.html#?job_id=3309085&repo=fx-team

Backout: https://hg.mozilla.org/integration/fx-team/rev/788e3cfa632b

Due to `uid`s in markers writing to the source data (which we shouldn't do), and this patch turning that into a global incrementor, so the uid's don't match up between sources. I guess we dont need it? https://treeherder.mozilla.org/#/jobs?repo=try&revision=c84ec90e48fc

yup

Relanding after successful try https://hg.mozilla.org/integration/fx-team/rev/b617a57d6bf1

Backed out for OSX debug browser_projecteditor_contextmenu_02.js and browser_projecteditor_menubar_02.js permafail starting with this push. Happened on both 10.6 and 10.10. https://treeherder.mozilla.org/logviewer.html#?job_id=3314997&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/dc4023d54436

Reset projecteditor pasting in those tests. Suspect it's due to reordering of tests, because there's nothing in the perftools that should affect those specifically. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b70288fe58e