Closed Bug 1176537 Opened 5 years ago Closed 4 years ago

JS/Event markers not rendered in waterfall

Categories

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

41 Branch
defect
Not set

Tracking

(firefox41 affected, firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

()

Details

Attachments

(3 files, 1 obsolete file)

Attached file skipped_js_marker.json
Not sure what's going on here, but there's a large yellow strip of JS occurring in the overview, but in the waterfall, no JS markers (event handlers) appear, just DOM events (and it doesn't seem to be the parent of the expected JS markers). It's like im filtering both JS markers, and DOM events that contain JS markers.

Check out the attached profile and image[0]. This seems like a bad bug.

[0] http://i.imgur.com/cVYJ5Sq.png
Attached file profile.json
Assignee: nobody → jsantell
Depends on: 1182910
Attached patch 1176537-marker-fix.patch (obsolete) — Splinter Review
So all of our marker tests weren't actually running (via bug 1182910), and when fixed, they weren't even passing, and require changes here anyway. This simplifies a lot of the collapsing/nesting logic. What was happening was the "child" marker was responsible for closing the parent marker, and when you have more than one layer deep, a marker can't close multiple parent markers -- this fixes that.

Adds a test for this failure, and fixes the other tests so they actually run and their assertions pass.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdcfb26617eb
Attachment #8632622 - Flags: review?(vporof)
Comment on attachment 8632622 [details] [diff] [review]
1176537-marker-fix.patch

Review of attachment 8632622 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/logic/marker-utils.js
@@ -309,5 @@
> - *             the current parent.
> - * - finalize: Whether or not the current parent should be finalized and popped
> - *        off the stack.
> - */
> -const CollapseFunctions = {

:(

I'm sad to see these blueprints go away. Is there a way to maintain these rules outside of waterfall-utils? Otherwise whenever new markers are added that need special casing for collapse we'd have to modify a for loop instead of these pure predicates.

::: browser/devtools/performance/modules/logic/waterfall-utils.js
@@ +22,5 @@
> +  let node = Object.create(null);
> +  for (let prop in marker) {
> +    node[prop] = marker[prop];
> +  }
> +  node.submarkers = [];

Can't just use object extend here from heritage? Instead of defining a whole new method.
Comment on attachment 8632622 [details] [diff] [review]
1176537-marker-fix.patch

Review of attachment 8632622 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/logic/marker-utils.js
@@ -309,5 @@
> - *             the current parent.
> - * - finalize: Whether or not the current parent should be finalized and popped
> - *        off the stack.
> - */
> -const CollapseFunctions = {

Honestly, we are worried about special nesting rules when they are still very much broken for the primary nesting use case -- they were standalone pure functions previously, but many nesting rules require the whole context of the markers to determine, that's why we were having these issues before of having a subsequent child marker determine when to close its parent, as it lacked the context.

I just can't imagine how many more special collapsing rules we'll need other than merging similar ones together, which is straight forward

::: browser/devtools/performance/modules/logic/waterfall-utils.js
@@ +22,5 @@
> +  let node = Object.create(null);
> +  for (let prop in marker) {
> +    node[prop] = marker[prop];
> +  }
> +  node.submarkers = [];

Probably make more sense to keep the method but it just uses the extend here anyway. We use this often in tests, and also in views, so should stick with an easily accessible function.
Comment on attachment 8632622 [details] [diff] [review]
1176537-marker-fix.patch

Review of attachment 8632622 [details] [diff] [review]:
-----------------------------------------------------------------

k
Attachment #8632622 - Flags: review?(vporof) → review+
https://hg.mozilla.org/mozilla-central/rev/1beb2b478d08
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.