STR: 1. from a about:home page, open the devtools, and run the following snippet: for (var i = 0; i < 300; i++) { window.open(i%2 ? 'http://www.google.fr' : 'http://linuxfr.org') } The goal is to open enough tabs, that are different so that we can notice the issue later on. 2. Open a last tab to another website (eg: http://everlong.org). 3. Press and keep pressed CTRL + SHIFT + LEFT-ARROW. This moves the tab to the left. => You'll notice it's quite smooth at the beginning, and then it's more and more laggy and slow. I don't know if this the same issue, but I notice that restarting Firefox with these tabs is also very slow. Note that this is not only a virtual issue, I get this issue every day in my normal browsing behavior. I tried to profile but I can't really make sense of what I get.

The session restore issue is tracked separately, so I'm updating the summary to more precisely reflect what this bug was filed about.

After profiling again, I've come to the conclusion that this is the slow operation: https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/toolkit/content/widgets/scrollbox.xml#138 It's slow when the tab I'm trying to move is at the left of the scrollbox. When it's at the right it's super quick. It looks like the triggered restyle is costly when it's at this position. The call tree looks like: _handleKeyDownEvent -> moveTabForward (tabbrowser.xml) -> moveTabTo (tabbrowser.xml) -> _handleTabSelect (tabbrowser.xml) -> ensureElementIsVisible (scrollbox.xml) -> get_scrollClientRect (scrollbox.xml)

That smells like a sync reflow to me. There is some discussion around the slowness caused by animating the scrolling of the tab bar in JS in bug 1344302.

Yeah, but the fact is that it's fast when the tab is at the right, but slow when it's at the left. So even if it's very likely there is a sync reflow, the reflow itself is a lot slower when the moved tab is at the left.

I did some new profiles: * when a tab is to the left: https://perfht.ml/2v2vfQy * when a tab is to the right: https://perfht.ml/2v7bBmt We can see that it's 3 times slower when it's to the left. Also, this is when keeping the "ctrl+shift+page down/up" pressed: * when a tab starts from the left: https://perfht.ml/2vadLBK * when a tab starts from the right: https://perfht.ml/2v9DnPl

We clearly see that the restyle is painfully slow when we restyle the tab on the left :( We could use some virtualization here...

This is painfully slow because for each keypress event we change the tab selection and try to scroll the newly selected tab into view immediately. I think we should limit that behavior to at most once per refresh driver tick. This is what the attached WIP attempts to do, and it seems to work fine locally. It's just a WIP because I haven't searched hard for what other cases this change might break (ie. is there other code doing computations that would be broken by the fact that the scroll is now async). Julien, can you confirm that it fixes the bug for you, or at least dramatically reduces the pain?

(In reply to Julien Wajsberg [:julienw] from comment #5) > * when a tab starts from the left: https://perfht.ml/2vadLBK Emilio, is there anything actionable from a layout perspective in this profile?

(In reply to Florian Quèze [:florian] from comment #8) > (In reply to Julien Wajsberg [:julienw] from comment #5) > > > * when a tab starts from the left: https://perfht.ml/2vadLBK > > Emilio, is there anything actionable from a layout perspective in this > profile? The stacks in that profile look busted (there's random garbage in there), so no :) But if you get a profile which isn't feel free to ni?, happy to take a look... I suspect we're restyling a lot because of `:-moz-window-inactive` or such... But again can't tell without a proper profile.

Comment on attachment 8995256 [details] [diff] [review] Tentative patch The tab bar scroll is a bit off but otherwise this looks better. Here is a profile with that patch: https://perfht.ml/2v3kb5y

(In reply to Julien Wajsberg [:julienw] from comment #10) > Here is a profile with that patch: https://perfht.ml/2v3kb5y The stacks in this new profile seem better.

Any chances of seeing one with the 'Sequential styling' feature of the profiler toggled? Otherwise I can just see that we're styling a bunch of stuff OMT. Or, if the STR at comment 0 are still valid, I can take a look tomorrow.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12) > Any chances of seeing one with the 'Sequential styling' feature of the > profiler toggled? Otherwise I can just see that we're styling a bunch of > stuff OMT. https://perfht.ml/2JY6lqF

I think this fixes the scrolling issue mentioned in comment 10.

So, what's going on is that we're shuffling the <tab>'s attributes, which means that we need to start looking into all the attribute selectors that may change the matching of the <tab>. That includes a bunch of :root selectors, because we don't special-case :root, so we match those against every element. I think adding a bucket for :root selectors should improve the situation here and in general... Will see. Then there are a bunch of selectors that we can't really do better than that like: https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/browser/themes/shared/urlbar-searchbar.inc.css#216 Or all the [data-identity-color] attributes: https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/browser/components/contextualidentity/content/usercontext.css We may want to be more granular at that, but that may very well end up being a performance regression for elements with a lot of attributes, so it's more unclear it's worth doing. I'll file a bug with the :root bucket and we can measure stuff there.

Comment on attachment 8995287 [details] [diff] [review] Tentative patch v2 Review of attachment 8995287 [details] [diff] [review]: ----------------------------------------------------------------- yep, this is better ! Still not smooth of course, you expected this, but much better already.

Comment on attachment 8995287 [details] [diff] [review] Tentative patch v2 This optimization is simple and effective enough that I think we should attempt to land something along these lines. The part that I'm unsure about is if I need code to pay special attention to the aInstant parameter. More specifically, if during the same frame _handleTabSelect is called both with aInstant = false and aInstant = true. The current (naive) patch will just take into account the value of the first call and drop future calls on the floor. But maybe I'm overthinking this and the chances of this edge case occurring are tiny enough (and the consequence small enough) that it doesn't matter. Dão, any opinion on this?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #15) Thanks again for investigating! And I see your patch for the :root bucket got r+'ed :-). Not sure how big an improvement I should expect from that patch, but I guess we'll see soon. > Then there are a bunch of selectors that we can't really do better than that > like: > > > https://searchfox.org/mozilla-central/rev/ > d47c829065767b6f36d29303d650bffb7c4f94a3/browser/themes/shared/urlbar- > searchbar.inc.css#216 Shouldn't this match only the #pageActionSeparator element, and then look only at its sibling? Or am I missing something? Is this bad enough that we should consider writing the selector differently? > Or all the [data-identity-color] attributes: > > > https://searchfox.org/mozilla-central/rev/ > d47c829065767b6f36d29303d650bffb7c4f94a3/browser/components/ > contextualidentity/content/usercontext.css > > We may want to be more granular at that, but that may very well end up being > a performance regression for elements with a lot of attributes, so it's more > unclear it's worth doing. Is it bad enough that it would make sense to add a class to all the attributes that can ever have this data-identity-color attribute? Would this help?

(In reply to Florian Quèze [:florian] from comment #18) > (In reply to Emilio Cobos Álvarez (:emilio) from comment #15) > > Thanks again for investigating! And I see your patch for the :root bucket > got r+'ed :-). Not sure how big an improvement I should expect from that > patch, but I guess we'll see soon. > > > Then there are a bunch of selectors that we can't really do better than that > > like: > > > > > > https://searchfox.org/mozilla-central/rev/ > > d47c829065767b6f36d29303d650bffb7c4f94a3/browser/themes/shared/urlbar- > > searchbar.inc.css#216 > > Shouldn't this match only the #pageActionSeparator element, and then look > only at its sibling? Or am I missing something? Is this bad enough that we > should consider writing the selector differently? No, because it's in a :not(..), so it matches any element which doesn't have the pageActionSeparator id. If it's possible to write it differently it'd be nice, but probably not terrible? > > Or all the [data-identity-color] attributes: > > > > > > https://searchfox.org/mozilla-central/rev/ > > d47c829065767b6f36d29303d650bffb7c4f94a3/browser/components/ > > contextualidentity/content/usercontext.css > > > > We may want to be more granular at that, but that may very well end up being > > a performance regression for elements with a lot of attributes, so it's more > > unclear it's worth doing. > > Is it bad enough that it would make sense to add a class to all the > attributes that can ever have this data-identity-color attribute? Would this > help? Whenever an attribute changes anywhere in the page we go through these, yeah. Probably not terrible, since we avoid looking for attribute changes whose attribute doesn't appear in any selector, but it may add up. Classes are definitely faster.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #19) > > Shouldn't this match only the #pageActionSeparator element, and then look > > only at its sibling? Or am I missing something? Is this bad enough that we > > should consider writing the selector differently? > > No, because it's in a :not(..), so it matches any element which doesn't have > the pageActionSeparator id. If it's possible to write it differently it'd be > nice, but probably not terrible? Oh, I think I misread your question, let me try to clarify. For normal styling of an element yes, it's pretty fast, we only look at #pageActionSeparator, then to the previous siblings. But when an id or attribute changes, the style of #pageActionSeparator may have changed (we either got / removed the pageActionSeparator id which is inside the :not(..), or the hidden attribute), so we need to check that selector unconditionally to see if it changed matching or not. We'd only look at following siblings if it actually changed, though.

Comment on attachment 8995287 [details] [diff] [review] Tentative patch v2 (In reply to Florian Quèze [:florian] from comment #17) > Comment on attachment 8995287 [details] [diff] [review] > Tentative patch v2 > > This optimization is simple and effective enough that I think we should > attempt to land something along these lines. > > The part that I'm unsure about is if I need code to pay special attention to > the aInstant parameter. More specifically, if during the same frame > _handleTabSelect is called both with aInstant = false and aInstant = true. > The current (naive) patch will just take into account the value of the first > call and drop future calls on the floor. But maybe I'm overthinking this and > the chances of this edge case occurring are tiny enough (and the consequence > small enough) that it doesn't matter. Dão, any opinion on this? The later call should always win regardless of aInstant because this.selectedItem could have changed, as I understand it.

Indeed, if we have several calls the selected tab has likely changed, good catch!

Here is a new profile from a build that includes the fixes from bug 1478985, bug 1479426, and the patch I attached here (attachment 8996252 [details] [diff] [review]) : https://perfht.ml/2NYZYpC

Comment on attachment 8996252 [details] [diff] [review] Patch v3 This seems okay, but perhaps we should fix the ensureElementIsVisible implementation instead?

(In reply to Dão Gottwald [::dao] from comment #24) > This seems okay, but perhaps we should fix the ensureElementIsVisible > implementation instead? I'm afraid other callers may assume that ensureElementIsVisible works synchronously, and read the scroll position right after calling ensureElementIsVisible.

I don't think we need to be too concerned these days with legacy add-ons gone. I believe the only significant API users are tabbed browser and some menu binding. Plus, the default non-aInstant case is already async.

(In reply to Dão Gottwald [::dao] from comment #26) > I don't think we need to be too concerned these days with legacy add-ons > gone. I believe the only significant API users are tabbed browser and some > menu binding. Plus, the default non-aInstant case is already async. Where would you do it? Only in the scrollbox.xml implementation? I'm asking because it looks like we currently have 7 "ensureElementIsVisible" methods in the tree: https://searchfox.org/mozilla-central/search?q=ensureElementIsVisible While I like the idea of fixing the whole category of bugs rather than just this specific instance, I don't think doing it everywhere is an option just to fix this bug, and introducing more inconsistencies may not be an improvement. Also, the other tabbrowser.xml caller doesn't look like it would benefit from this new behavior: https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/browser/base/content/tabbrowser.xml#726 (there's a forced sync layout flush at line 709, so ensureElementIsVisible can work synchronously cheaply here).

(In reply to Florian Quèze [:florian] from comment #27) > (In reply to Dão Gottwald [::dao] from comment #26) > > I don't think we need to be too concerned these days with legacy add-ons > > gone. I believe the only significant API users are tabbed browser and some > > menu binding. Plus, the default non-aInstant case is already async. > > Where would you do it? Only in the scrollbox.xml implementation? Yes. > I'm asking > because it looks like we currently have 7 "ensureElementIsVisible" methods > in the tree: > https://searchfox.org/mozilla-central/search?q=ensureElementIsVisible While > I like the idea of fixing the whole category of bugs rather than just this > specific instance, I don't think doing it everywhere is an option just to > fix this bug, and introducing more inconsistencies may not be an improvement. The default behavior is already different enough that it doesn't matter. Also, the devtools thing is just a reimplementation of XUL widgets and we should probably unfork (i.e. get rid of) this stuff when we've moved to HTML.

Ok, moved this requestAnimationFrame call to scrollbox.xml. I was concerned existing code may be relying on the sync behavior, but the only case I could find where code obviously relies on this turned out to be dead code (I couldn't find anything ever setting this._scrollTarget) so I just removed it.

Unfortunately try isn't happy about this patch (https://treeherder.mozilla.org/#/jobs?repo=try&revision=623bb98d8afc3d66ef3079a0a1db070fe956a819), the browser/base/content/test/tabs/browser_overflowScroll.js test relies on the sync behavior. New patch fixing the test coming.

This changes the test to wait until we have applied the changes. For the specific case of double clicking the arrow to scroll by a 'page', I had to change things a bit more because the behavior before the patch is broken, and the test enforces that behavior. Before the patch, the mousedown event first scrolls by one tab, and then the click event (for click count = 2) scrolls by a page. With the patch applied, the second scroll discards the first one so we end up scrolling by one less tab than before.

Try seems happier now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45fe6a53894af749cd5076c9f123b45a4ca8f3a2

(In reply to Florian Quèze [:florian] from comment #31) > For the specific case of double clicking the arrow to scroll by a 'page', I > had to change things a bit more because the behavior before the patch is > broken, and the test enforces that behavior. Before the patch, the mousedown > event first scrolls by one tab, and then the click event (for click count = > 2) scrolls by a page. With the patch applied, the second scroll discards the > first one so we end up scrolling by one less tab than before. I think the behavior change isn't actually perceptible by users, as I don't know any user who can make a double click in less than 16ms, so it's unlikely that the mousedown and double click events will ever happen in the same frame for real users. Unless they use some tool to simulate a double click when they do a single click on a different button I guess.

Pushed by florian@queze.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5feed1e4a18 changing the tab selection several times in a row should flush layout at most once per refresh driver tick, rather than once per tab, r=Dao.