Avoid layout flush and invalidation loop in _positionPinnedTabs

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 2 bugs, {perf})

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
(In reply to Markus Stange [:mstange] from bug 1339859 comment #12)
> Thanks for the profile!
> 
> We have a layout thrashing loop at
> http://searchfox.org/mozilla-central/rev/
> fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/browser/base/content/tabbrowser.
> xml#6078-6082 :
> 
> for (let i = numPinned - 1; i >= 0; i--) {
>   let tab = this.childNodes[i];
>   width += tab.getBoundingClientRect().width;
>   tab.style.marginInlineStart = -(width + scrollButtonWidth + paddingStart)
> + "px";
> }
> 
> We query layout information and then set a CSS property that affects layout,
> in a loop.

It's reasonable to assume that all pinned tabs have the same width. This way we can avoid the above problem.
Comment hidden (mozreview-request)

Comment 2

4 months ago
mozreview-review
Comment on attachment 8856088 [details]
Bug 1354782 - Re-use pinned tab width to avoid layout flush and invalidation loop in _positionPinnedTabs.

https://reviewboard.mozilla.org/r/128034/#review130576

::: browser/base/content/tabbrowser.xml:6082
(Diff revision 1)
>              let width = 0;
>  
>              for (let i = numPinned - 1; i >= 0; i--) {
>                let tab = this.childNodes[i];
> -              width += tab.getBoundingClientRect().width;
> +              if (!pinnedTabWidth) {
> +                pinnedTabWidth = tab.getBoundingClientRect().width;

(for a follow-up) Could we save the pinned tab's width once at the time the tab is pinned (or soon after), and avoid this getBoundingClientRect call completely?
Attachment #8856088 - Flags: review?(florian) → review+
(Assignee)

Comment 3

4 months ago
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> Comment on attachment 8856088 [details]
> Bug 1354782 - Re-use pinned tab width to avoid layout flush and invalidation
> loop in _positionPinnedTabs.
> 
> https://reviewboard.mozilla.org/r/128034/#review130576
> 
> ::: browser/base/content/tabbrowser.xml:6082
> (Diff revision 1)
> >              let width = 0;
> >  
> >              for (let i = numPinned - 1; i >= 0; i--) {
> >                let tab = this.childNodes[i];
> > -              width += tab.getBoundingClientRect().width;
> > +              if (!pinnedTabWidth) {
> > +                pinnedTabWidth = tab.getBoundingClientRect().width;
> 
> (for a follow-up) Could we save the pinned tab's width once at the time the
> tab is pinned (or soon after), and avoid this getBoundingClientRect call
> completely?

Yes, but with two caveats:

- We already flush layout before the loop, so getBoundingClientRect is cheap at this point. We'd have to cache scrollButtonWidth and paddingStart too.

- We'd have to clear the cache when the theme layout changes (e.g. when enabling a compact theme, but we already call _positionPinnedTabs there for the same reason, so it's probably not a big deal)

Do you think the added complexity would be worth it?
Flags: needinfo?(florian)
(In reply to Dão Gottwald [::dao] from comment #3)
> (In reply to Florian Quèze [:florian] [:flo] from comment #2)

> > (for a follow-up) Could we save the pinned tab's width once at the time the
> > tab is pinned (or soon after), and avoid this getBoundingClientRect call
> > completely?
> 
> Yes, but with two caveats:
> 
> - We already flush layout before the loop, so getBoundingClientRect is cheap
> at this point. We'd have to cache scrollButtonWidth and paddingStart too.
> 
> - We'd have to clear the cache when the theme layout changes (e.g. when
> enabling a compact theme, but we already call _positionPinnedTabs there for
> the same reason, so it's probably not a big deal)
> 
> Do you think the added complexity would be worth it?

Yes. Any layout flush we can remove (especially when they are during user interactions where smooth visual feedback is expected like resizing) is worth it. But it's not worth blocking landing of removing the invalidation loop for several days, this is why I said "for a follow-up". Up to you to decide if you want to try here or file a lower priority follow-up :-).
Flags: needinfo?(florian)
(Assignee)

Updated

4 months ago
Blocks: 1354789

Comment 5

4 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/630ecf1e615d
Re-use pinned tab width to avoid layout flush and invalidation loop in _positionPinnedTabs. r=florian
https://hg.mozilla.org/mozilla-central/rev/630ecf1e615d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
No longer blocks: 1348289
Blocks: 1348289
You need to log in before you can comment on or make changes to this bug.