1260036 - Moving a tab with the keyboard is slow when there are a lot of tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[:Gijs (he/him)]

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

Comment 2

[Julien Wajsberg [:julienw]]

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)

Comment 3

[(no longer active)]

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.

Comment 4

[Julien Wajsberg [:julienw]]

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.

Comment 5

[Julien Wajsberg [:julienw]]

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

Comment 6

[Julien Wajsberg [:julienw]]

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

Comment 7

[Florian Quèze [:florian]]

Attachment: Tentative patch

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?

Comment 8

[Florian Quèze [:florian]]

(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?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(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 10

[Julien Wajsberg [:julienw]]

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

Comment 11

[Florian Quèze [:florian]]

(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.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 13

[Florian Quèze [:florian]]

(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

Comment 14

[Florian Quèze [:florian]]

Attachment: Tentative patch v2

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

Comment 15

[Emilio Cobos Álvarez (:emilio)]

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 16

[Julien Wajsberg [:julienw]]

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 17

[Florian Quèze [:florian]]

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?

Comment 18

[Florian Quèze [:florian]]

(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?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(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 21

[Dão Gottwald [:dao]]

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.

Comment 22

[Florian Quèze [:florian]]

Attachment: Patch v3

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

Comment 23

[Florian Quèze [:florian]]

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 24

[Dão Gottwald [:dao]]

Comment on attachment 8996252 [details] [diff] [review]
Patch v3

This seems okay, but perhaps we should fix the ensureElementIsVisible implementation instead?

Comment 25

[Florian Quèze [:florian]]

(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.

Comment 26

[Dão Gottwald [:dao]]

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.

Comment 27

[Florian Quèze [:florian]]

(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).

Comment 28

[Dão Gottwald [:dao]]

(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.

Comment 29

[Florian Quèze [:florian]]

Attachment: Patch v4

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.

Comment 30

[Florian Quèze [:florian]]

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.

Comment 31

[Florian Quèze [:florian]]

Attachment: Patch v5

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.

Comment 32

[Florian Quèze [:florian]]

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

Comment 33

[Florian Quèze [:florian]]

(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.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 35

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c5feed1e4a18