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

Status

()

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

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

41 Branch
Firefox 41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(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]
1169439-move-marker-global.patch

Part 1, move markers into its own "global"
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8613280 - Flags: review?(vporof)
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.
Attachment #8613280 - Flags: review?(vporof) → review+

Comment 3

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/5e803cc779dc
Comment on attachment 8613280 [details] [diff] [review]
1169439-move-marker-global.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=562bce00ed3c
Attachment #8613280 - Flags: checkin+
(Assignee)

Updated

2 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/5e803cc779dc
Created attachment 8613563 [details] [diff] [review]
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
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]
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
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]
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".
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
Backout:
https://hg.mozilla.org/integration/fx-team/rev/788e3cfa632b
Created attachment 8614693 [details] [diff] [review]
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
Attachment #8613660 - Attachment is obsolete: true
Flags: needinfo?(jsantell)
Attachment #8614693 - Flags: review+
yup
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.
https://treeherder.mozilla.org/logviewer.html#?job_id=3314997&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/dc4023d54436
Created attachment 8615403 [details] [diff] [review]
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
Attachment #8614693 - Attachment is obsolete: true
Attachment #8615403 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/34b8dc69708a
https://hg.mozilla.org/mozilla-central/rev/34b8dc69708a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
https://hg.mozilla.org/mozilla-central/rev/34b8dc69708a
You need to log in before you can comment on or make changes to this bug.