If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Markers should be collapsed under their parent when fully encompassed

RESOLVED FIXED in Firefox 41



Developer Tools: Performance Tools (Profiler/Timeline)
2 years ago
2 years ago


(Reporter: jsantell, Assigned: jsantell)


41 Branch
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)



(2 attachments, 3 obsolete attachments)

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.
Created attachment 8613280 [details] [diff] [review]

Part 1, move markers into its own "global"
Assignee: nobody → jsantell
Attachment #8613280 - Flags: review?(vporof)
Comment on attachment 8613280 [details] [diff] [review]

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.
Attachment #8613280 - Flags: review?(vporof) → review+

Comment 3

2 years ago
Comment on attachment 8613280 [details] [diff] [review]

Attachment #8613280 - Flags: checkin+


2 years ago
Keywords: leave-open
Created attachment 8613563 [details] [diff] [review]

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
Attachment #8613563 - Flags: feedback?(vporof)
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.
Created attachment 8613660 [details] [diff] [review]

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.

Attachment #8613563 - Attachment is obsolete: true
Attachment #8613563 - Flags: feedback?(vporof)
Attachment #8613660 - Flags: review?(vporof)
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]

Review of attachment 8613660 [details] [diff] [review]:


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".
Attachment #8613660 - Flags: review?(vporof) → review+
Comments addressed; landing: https://hg.mozilla.org/integration/fx-team/rev/8e4fdb45e6a8
can address consecutive markers in bug 1169887
Whiteboard: [fixed-in-fx-team]
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
Flags: needinfo?(jsantell)

Comment 13

2 years ago
Created attachment 8614693 [details] [diff] [review]

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?

Attachment #8613660 - Attachment is obsolete: true
Flags: needinfo?(jsantell)
Attachment #8614693 - Flags: review+
Relanding after successful try  https://hg.mozilla.org/integration/fx-team/rev/b617a57d6bf1
Keywords: leave-open
Blocks: 1141614
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.

Created attachment 8615403 [details] [diff] [review]

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.

Attachment #8614693 - Attachment is obsolete: true
Attachment #8615403 - Flags: review+
Last Resolved: 2 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.