I believe we should sometimes fold markers together, like Chrome does. Examples: - 12 paint markers, in a row, very very close to each other. We could collapse all these markers into one; - a click event marker might include JS markers and sync reflow. This could be collapsed into just "click event"

Might be similar to bug 1145645

yes, would much prefer folding to merging. I don't want to lose data, just collapse it by default to give me a better overview.

This is pretty important as well. How hard would this be to do, vp? Never really looked at the waterfall widget

This needs a bit of hacking. A blueprint of some sort specifying which markers to fold, which one is the parent to contain the expando arrow etc., or this information included in the markers themselves. From the view's POV, the DOM structure of the waterfall needs to be changed a bit, either by adding a SuperMarker (expando, container etc.), or somehow just linking multiple existing markers together and showing an expando on the parent. The latter approach is probably faster. All in all not a huge amount of work, but it's going to be a little bit all over the place.

Also, seeing fitzgen dogfooding on treeherder last few days, the less markers we render in the waterfall the better, so maybe it'd help with bug 1151406. Also with timing issues, I wonder if we'd get some weirdness in syncing -- like a marker that should be strictly inside of a JS call falls slightly outside of it. May have to do some adjusting. Paul, how important is this? Also, what would some "rules" be for collapsing? I have a pretty good idea (pretty much any of our purple/green markers should collapse under a JS one), but what about others?

> Paul, how important is this? We can ship without it. I would love to have this, but it should not be more important that correctness bugs and UI annoyances. > Also, what would some "rules" be for collapsing 1) DOM Events markers should fuse into the direct child marker (JS). In the UI, they appear as one marker, and it's impossible to "unfold" them. 2) JS markers can include other markers (sync reflows). Markers inside a JS marker should be collapsed in the JS marker. 3) Successive repeating markers should be fused into on marker of the same nature (if close enough). The "meta marker" can be uncollapsed. This new marker should be displayed a bit differently because it's not a blocking operation. Think of 100 restyle markers spread over 2 seconds (with no markers between). They should be seen as one purple marker, with white stripes maybe. I'm not sure about 3). We should look at what Chrome & Safari do.

(In reply to Paul Rouget [:paul] from comment #7) > 1) DOM Events markers should fuse into the direct child marker (JS). In the > UI, they appear as one marker, and it's impossible to "unfold" them. Disagree. Would prefer to have them separate (after expanding of course). I prefer the focus on what is happening at the top level: an XHR load or a DOM event, or a requestAnimationFrame, etc. This was useful when debugging treeherder performance because I was able to see at a glance that even though things were in an rAF loop, XHR load events were choking the event loop in between the rAF calls and a frame wasn't actually getting painted between rAF calls. If all of that was just "Function Call" it would be less clear (although the JS entry reasons does help, it isn't as nice IMO). If I had to compromise, I would say they should fuse the other way, the JS into the DOM event. Seeing `DOM event (load)` or `Timeout Fired` is way more useful than `Function Call (DOMEventListenerNotNull)` or `Function Call (TimeoutHandlerNotNull)` like we have on Function Call markers now. > 2) JS markers can include other markers (sync reflows). Markers inside a JS > marker should be collapsed in the JS marker. I think this should just be the general rule: if a marker is wholly contained within another marker, then it should be collapsed within the containing marker. The only exception is console.time() and console.timeEnd() which aren't LIFO like the other markers and can likely span the huge swaths of the profile. > 3) Successive repeating markers should be fused into on marker of the same > nature (if close enough). The "meta marker" can be uncollapsed. This new > marker should be displayed a bit differently because it's not a blocking > operation. Think of 100 restyle markers spread over 2 seconds (with no > markers between). They should be seen as one purple marker, with white > stripes maybe. > > I'm not sure about 3). We should look at what Chrome & Safari do. I'm a tentative -1 for (3). I really want to know what Gecko is really doing, even if it's a Gecko bug; I don't want to be lied to. Additionally, when we get better metadata for why a marker is happening (badly needed for recalc style and reflow) then each of these markers might have different metadata causes.

(In reply to On vacation until May 17th from comment #8) > (In reply to Paul Rouget [:paul] from comment #7) > > 1) DOM Events markers should fuse into the direct child marker (JS). In the > > UI, they appear as one marker, and it's impossible to "unfold" them. > > Disagree. Would prefer to have them separate (after expanding of course). I > prefer the focus on what is happening at the top level: an XHR load or a DOM > event, or a requestAnimationFrame, etc. This was useful when debugging > treeherder performance because I was able to see at a glance that even > though things were in an rAF loop, XHR load events were choking the event > loop in between the rAF calls and a frame wasn't actually getting painted > between rAF calls. If all of that was just "Function Call" it would be less > clear (although the JS entry reasons does help, it isn't as nice IMO). > > If I had to compromise, I would say they should fuse the other way, the JS > into the DOM event. Seeing `DOM event (load)` or `Timeout Fired` is way more > useful than `Function Call (DOMEventListenerNotNull)` or `Function Call > (TimeoutHandlerNotNull)` like we have on Function Call markers now. Agreed. What I meant is just to avoid having 2 markers that reflect the exact same thing.

3) I think having a meta marker for similar nodes is a good one. Once we have correct (or atleast similar to Chrome) restyle markers, (bug 1164338) we'll have a lot of cascading sequences of markers, like this, which cover the full duration of the JS marker encapsulating it: http://i.imgur.com/3GkTx7L.png In Chrome, they also have this as a long list of cascading markers. If we were to combine these in a long (purple) marker, with "lighter" purple inbetween like: [XXxxXxXxXxxXxxxxXXXXxX] That was able to be expanded, I think that gets the job done -- we can still individually inspect markers for their metadata (especially once we add more, interesting data points per marker), and this lets you expand without needing a giant amount of vertical space.

This replaces the current waterfall view with a tree. It's one level deep, so everything should look and function exactly the same, but the underlying logic supports folding markers for free. I'm still tweaking the reduce methods for collapsing markers, but the next patch should be fairly small.

Worth mentioning is that the waterfall is now a subclass of the AbstractTreeItem. So if we optimize that to only display visible nodes at once, then all the views in the perf tool will get a free perf boost. Working on that now.

Last comment here: performance is a bit dodgy right now, but fixable. I got some ideas.

Comment on attachment 8610304 [details] [diff] [review] Part 1: Replace the waterfall view with a tree Review of attachment 8610304 [details] [diff] [review]: ----------------------------------------------------------------- verrrry nice. ::: browser/devtools/performance/views/details-waterfall.js @@ -79,5 @@ > */ > _onMarkerSelected: function (event, marker) { > let recording = PerformanceController.getCurrentRecording(); > - // Race condition in tests due to lazy rendering of markers in the > - // waterfall? intermittent bug 1157523 I think this may have reduced the chance of that intermittent occurring atleast (and all perf intermittents kinda bleed into all the other bugs so it's hard to tell)

Two simple things implemented so far (we can further extend the logic later): 1. Identical markers are collapsed together 2. DOM events + following JS marker are combined into a single meta marker

Comment on attachment 8610732 [details] [diff] [review] Part 2, v2: Add marker folding logic Review of attachment 8610732 [details] [diff] [review]: ----------------------------------------------------------------- Awesome. I don't think the console markers should be collapsable, and with merging identical nodes in general, afraid they'll make some markers look larger than they are (if there's a lot of space between them in the current selection, but have yet to try it out (will in a bit). If tests are easy, great, if not let's do a follow up

Fixed typo

Comment on attachment 8610742 [details] [diff] [review] Part 2, v2: Add marker folding logic k

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #19) > Comment on attachment 8610732 [details] [diff] [review] > Part 2, v2: Add marker folding logic > > Review of attachment 8610732 [details] [diff] [review]: > ----------------------------------------------------------------- > > Awesome. I don't think the console markers should be collapsable, and with > merging identical nodes in general, afraid they'll make some markers look > larger than they are (if there's a lot of space between them in the current > selection True. Easily fixable by adding another conditional in the folding predicate. We can do that later.

A GIF https://dl.dropboxusercontent.com/u/2388316/markers.gif

(In reply to Victor Porof [:vporof][:vp] from comment #23) > A GIF https://dl.dropboxusercontent.com/u/2388316/markers.gif It looks like this isn't just folding markers that are within another marker into the containing marker, but also aggregating a series of the same marker type into a single merged fake marker. I think this should only be done if and only if the start and end times are back-to-back. For example (especially!), in the gif I saw one of these merged fake markers for GC where there was actually quite a bit of time between individual GC slices: [//////////////////////////////] <--- merged fake marker [] [] <--- actual markers [] [] Showing that whole region as a GC pause is _very_ misleading: it makes it seem like there is a long GC pause (what we want to avoid and alert users of), when in fact there were a few short pauses (this is our ideal state with incremental GC) and the mutator (the non-GC JS code) is still running most of the time. I'm not saying we shouldn't collapse these markers, just that we shouldn't merge them into a fake marker when they aren't back-to-back because it is misleading. If they fall within another marker that wholly contains them, then yeah they should be collapsed as per comment the second part of comment 8.

I agree with everything in comment 24, but we really should either land or not land this, so we have time to uplift and test the all-new waterfall view.

I'll make it so that only back to back markers are collapsed.

Perfect; gives us time to nail down that logic and get perf improvements in 40.1

Making a note here after experimenting a bit: adhering to a *strict* back-to-back rule doesn't work at all. No markers ever merge together if we rely on the consecutive end & start being identical, or very very close. Allowing for a relatively small delta in between is probably a good idea.

It seems that docShells.now(), which is what we're using for everything now, is accurate to the microsecond (and I've seen some things that look like they have 2 more decimal points of precision in other instances??), so strict back-to-back is probably impossible. Can we land atleast the waterfall changes, and the framework for the collapsing atleast, and figure out rules later?

Even with a 10ms window for example, consecutive "Paint" markers never collapse on any page that I've tested. I'll attach a new patch with some numbers that make sense to me. They seem to behave nicely in the real world.

Now fixing and adding a test.

(In reply to Victor Porof [:vporof][:vp] from comment #30) > Even with a 10ms window for example, consecutive "Paint" markers never > collapse on any page that I've tested. I'll attach a new patch with some > numbers that make sense to me. They seem to behave nicely in the real world. I don't know much about painting, but it seems quite possible that this is similar to the GC markers I mentioned in comment 24, and that they *shouldn't* be merged. Would be good to find someone on the platform team who can better answer the question. My gut reaction is that 10ms is way too long, and our fuzz factor should be more like 1ms.

Comment on attachment 8610935 [details] [diff] [review] Part 2, v3: Add marker folding logic Review of attachment 8610935 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/modules/global.js @@ +108,5 @@ > + * Invoked with 3 arguments: the current parent marker, the > + * current marker and the next marker. If nothing is returned, > + * the marker is added as a standalone entry in the waterfall. > + * Otherwise, an object needs to be returned with the properties: > + * - toParent: The parent parent marker name (needs to be an parent parent @@ +109,5 @@ > + * current marker and the next marker. If nothing is returned, > + * the marker is added as a standalone entry in the waterfall. > + * Otherwise, an object needs to be returned with the properties: > + * - toParent: The parent parent marker name (needs to be an > + * entry in the `TIMELINE_BLUEPRINT` itself). It took me a bit to get this -- this is the name of a new meta marker? @@ +170,5 @@ > label: getJSLabel, > fields: getJSFields, > }, > + "meta::DOMEvent+JS": { > + group: 1, Do these metamarkers affect anything in the overview graphs? Does it need a group tag?

10ms is indeed wayyyy too long. Here is my issue: I'll select a chunk in the overview and in the waterfall, I see a bunch of markers cascading down. Since I can only select such a small time range, for example if I'm hovering in on a red (GC) marker in the overview), this means that all my markers in the waterfall are still super tiny, but use the minimum width provided, so it's hard to tell which markers are encapsulating what -- this was especially painful when trying to find markers that had stack traces so I can debug that. My view was only maybe 20ms or so across, but had many markers inbetween that could not be rendered at a proportional width (because then it'd be too small for me to select). If we want to do something like this, it should be based on the relative view width. If I have 10s selected, and some markers are 10ms next to each other, or 0.1%, then sure, collapse, I'm at an eagle eye's view anyway, but when I'm zoomed in quite a bit, like 20ms, then I'd expect only to collapse if the markers are again, like 0.1% close, or .02 ms.

It sounds like our minimum width isn't minimal enough?

It is, this is just a lot of markers at a very small range. Collapsing under parent would've helped in this scenario, because I just wanted to know what markers were nested -- the timing is indicative via ordering, except in this case.

1ms for paint markers will end up with them never collapsing. Even 10ms will have almost never collapse. It might indeed be the case that we should never collapse Paint markers. But the same thing happens with all the other marker types as well; I used "Paint" before just as an example: the reality is, most of the time, all markers are relatively far away from each other. However, after playing with this a bit, I strongly feel like collapsing markers of the same type gives me a much better feel for what's going on. Having the entire view eclipsed by a waterfall of a single marker makes it harder to focus on what happens before & after. The patches are up here, you can experiment with the reduce methods. That being said, let's just land this and we can iterate on the collapse logic later, since it's fairly easy to change.

+1 to landing and fighting in whistler about this

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #36) > Comment on attachment 8610935 [details] [diff] [review] > Part 2, v3: Add marker folding logic > > Review of attachment 8610935 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +109,5 @@ > > + * current marker and the next marker. If nothing is returned, > > + * the marker is added as a standalone entry in the waterfall. > > + * Otherwise, an object needs to be returned with the properties: > > + * - toParent: The parent parent marker name (needs to be an > > + * entry in the `TIMELINE_BLUEPRINT` itself). > > It took me a bit to get this -- this is the name of a new meta marker? > Yes, although a better definition is simply a "parent" marker. For example * consecutive "Parse XML" markers get collapsed, but not into a meta marker, just a single enclosing "Parse XML" marker. * the other example is consecutive "DOMEvent" and "JavaScript" markers, which *do* get collapsed into a DOMEvent+JS meta marker. > @@ +170,5 @@ > > label: getJSLabel, > > fields: getJSFields, > > }, > > + "meta::DOMEvent+JS": { > > + group: 1, > > Do these metamarkers affect anything in the overview graphs? Does it need a > group tag? Nope, I guess not.

(btw all of my observations refer to Styles markers, which can easily accumulate long, connected chains, and sometimes, can always come in pairs of two, immediately after each other, 0.01ms apart)

Maybe we need different fuzz values for different kinds of markers? Ugh.

We should understand why, in the first place, N markers don't make ONE marker. What is happening between 2 paint makers exactly? Or between 2 style markers? > +1 to landing and fighting in whistler about this +1

Rebased, cleaned up CSS a bit.

Made it so that meta DOM+JS event markers now copy the information from the child markers and show it by default in the sidebar.

Collapsed DOM+JS markers.

Added a peek function.

We fixed bug 1130589 for free. http://www.reactiongifs.com/r/dink.gif

Comment on attachment 8611313 [details] [diff] [review] Part 4, v5: Add a new test Review of attachment 8611313 [details] [diff] [review]: ----------------------------------------------------------------- We're probably going to want several tests for the many collapsing scenarios eventually, this looks good tho

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #55) > Comment on attachment 8611313 [details] [diff] [review] > Part 4, v5: Add a new test > > Review of attachment 8611313 [details] [diff] [review]: > ----------------------------------------------------------------- > > We're probably going to want several tests for the many collapsing scenarios > eventually, this looks good tho This tests the currently implemented collapsing scenarios. If we add more scenarios, yes, then we need more tests.

Landed on fx-team with wrong bug number: https://hg.mozilla.org/integration/fx-team/rev/f24a70954568 https://hg.mozilla.org/integration/fx-team/rev/37227fbe07f7 https://hg.mozilla.org/integration/fx-team/rev/c3594996ddb6 https://hg.mozilla.org/integration/fx-team/rev/37d2acb3c1f1

OOOOPS

This is in central.