1169439 - Markers should be collapsed under their parent when fully encompassed

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1169439-move-marker-global.patch

Part 1, move markers into its own "global"

Comment 2

[Victor Porof [:vporof][:vp]]

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 3

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5e803cc779dc

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8613280 [details] [diff] [review]
1169439-move-marker-global.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=562bce00ed3c

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e803cc779dc

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1169439-2.patch

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

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1169439-2.patch

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

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

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 10

[Victor Porof [:vporof][:vp]]

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".

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Pulsebot]

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

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1169439-2.patch

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

Comment 15

[Victor Porof [:vporof][:vp]]

yup

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1169439-2.patch

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

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

https://hg.mozilla.org/integration/fx-team/rev/34b8dc69708a

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34b8dc69708a

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34b8dc69708a