Closed Bug 1354782 Opened 3 years ago Closed 3 years ago

Avoid layout flush and invalidation loop in _positionPinnedTabs

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: dao, Assigned: dao)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(1 file)

(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 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+
(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)
Blocks: 1354789
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.