1176537 - JS/Event markers not rendered in waterfall

Status: RESOLVED FIXED

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

Description

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

Attachment: 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

Comment 1

[Paul Rouget [:paul]]

Attachment: profile.json

Comment 2

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

Attachment: 1176537-marker-fix.patch

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

Comment 3

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

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 4

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

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 5

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

Comment on attachment 8632622 [details] [diff] [review]
1176537-marker-fix.patch

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

k

Comment 6

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

Attachment: 1176537-marker-fix.patch

https://hg.mozilla.org/integration/fx-team/rev/1beb2b478d08

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1beb2b478d08