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

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: julienw, Assigned: florian)

Tracking

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [fxperf])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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

3 years ago
The session restore issue is tracked separately, so I'm updating the summary to more precisely reflect what this bug was filed about.
Summary: Tabs are slow with a lot of tabs → Moving a tab with the keyboard is slow when there are a lot of tabs
(Reporter)

Comment 2

2 years ago
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

2 years ago
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.
See Also: → 1344302
(Reporter)

Comment 4

2 years ago
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.
(Reporter)

Comment 5

9 months ago
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
(Reporter)

Updated

9 months ago
Whiteboard: [fxperf]
(Reporter)

Comment 6

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

Comment 7

9 months ago
Posted patch Tentative patch (obsolete) — Splinter Review
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?
Attachment #8995256 - Flags: feedback?(felash)
(Assignee)

Comment 8

9 months ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(emilio)
(Reporter)

Comment 10

9 months ago
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
Attachment #8995256 - Flags: feedback?(felash)
(Assignee)

Comment 11

9 months ago
(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.
Flags: needinfo?(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.
(Assignee)

Comment 13

9 months ago
(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
(Assignee)

Comment 14

9 months ago
Posted patch Tentative patch v2 (obsolete) — Splinter Review
I think this fixes the scrolling issue mentioned in comment 10.
Attachment #8995287 - Flags: feedback?(felash)
(Assignee)

Updated

9 months ago
Attachment #8995256 - Attachment is obsolete: true
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.
Flags: needinfo?(emilio)
Depends on: 1478985
See Also: → 1479012
(Reporter)

Comment 16

9 months ago
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.
Attachment #8995287 - Flags: feedback?(felash) → feedback+
(Assignee)

Comment 17

9 months ago
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?
Attachment #8995287 - Flags: review?(dao+bmo)
(Assignee)

Comment 18

9 months ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(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.
(Assignee)

Updated

9 months ago
Depends on: 1479426
(Assignee)

Updated

9 months ago
Depends on: 1479433
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.
Attachment #8995287 - Flags: review?(dao+bmo) → review-
(Assignee)

Comment 22

9 months ago
Posted patch Patch v3 (obsolete) — Splinter Review
Indeed, if we have several calls the selected tab has likely changed, good catch!
Attachment #8996252 - Flags: review?(dao+bmo)
(Assignee)

Updated

9 months ago
Attachment #8995287 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 23

9 months ago
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?
(Assignee)

Comment 25

9 months ago
(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.
(Assignee)

Comment 27

9 months ago
(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.
(Assignee)

Comment 29

9 months ago
Posted patch Patch v4 (obsolete) — Splinter Review
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.
Attachment #8997093 - Flags: review?(dao+bmo)
(Assignee)

Updated

9 months ago
Attachment #8996252 - Attachment is obsolete: true
Attachment #8996252 - Flags: review?(dao+bmo)
Attachment #8997093 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 30

9 months ago
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.
(Assignee)

Comment 31

9 months ago
Posted patch Patch v5Splinter Review
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.
Attachment #8997502 - Flags: review?(dao+bmo)
(Assignee)

Updated

9 months ago
Attachment #8997093 - Attachment is obsolete: true
(Assignee)

Comment 33

9 months ago
(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.
Attachment #8997502 - Flags: review?(dao+bmo) → review+

Comment 34

9 months ago
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.

Comment 35

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c5feed1e4a18
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.