5.05ms uninterruptible reflow at adjustTabstrip@chrome://browser/content/tabbrowser.xml:5949:24

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: geeknik, Assigned: dao)

Tracking

(Blocks 1 bug, {nightly-community})

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance])

Attachments

(1 attachment)

Here's the stack:

adjustTabstrip@chrome://browser/content/tabbrowser.xml:5949:24
_handleNewTab@chrome://browser/content/tabbrowser.xml:6420:11
addTab/<@chrome://browser/content/tabbrowser.xml:2301:17
setTimeout handler*addTab@chrome://browser/content/tabbrowser.xml:2300:15
loadOneTab@chrome://browser/content/tabbrowser.xml:1522:23
enter@resource:///modules/CustomizeMode.jsm:219:19
toggle@resource:///modules/CustomizeMode.jsm:159:7
oncommand@chrome://browser/content/browser.xul:1:1
We improved adjustTabstrip to not cause a sync flush when there are overflowing tabs in bug 1356655, but I guess this is a stack for a case without overflow.
Component: Untriaged → Tabbed Browser
See Also: → 1356655
Depends on: 1356655
See Also: 1356655
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Duplicate of this bug: 1356707
Flags: qe-verify? → qe-verify-
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:p2][reserve-photon-performance]
Flags: needinfo?(dao+bmo)
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
Priority: P3 → P1
Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

https://reviewboard.mozilla.org/r/143612/#review147550

::: browser/base/content/tabbrowser.xml:6045
(Diff revision 1)
>  
> -          let numTabs = this.childNodes.length -
> -                        this.tabbrowser._removingTabs.length;
> -          if (numTabs > 2) {
> -            // This is an optimization to avoid layout flushes by calling
> -            // getBoundingClientRect() when we just opened a second tab. In
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout. Use nested requestAnimationFrame rather than
> +          // MozAfterPaint to invalidate layout as late as possible.
> +          window.requestAnimationFrame(() => {
> +            window.requestAnimationFrame(() => {

Same question as I had in bug 1368208: why is this 2 nested requestAnimationFrame rather than a requestAnimationFrame in a MozAfterPaint listener?
(In reply to Florian Quèze [:florian] [:flo] from comment #4)

> > +          // Wait until after the next paint to get current layout data without
> > +          // flushing layout. Use nested requestAnimationFrame rather than
> > +          // MozAfterPaint to invalidate layout as late as possible.
> > +          window.requestAnimationFrame(() => {
> > +            window.requestAnimationFrame(() => {
> 
> Same question as I had in bug 1368208: why is this 2 nested
> requestAnimationFrame rather than a requestAnimationFrame in a MozAfterPaint
> listener?

Although the patch here does no flushing inside the requestAnimationFrame callback, so I'm fine with the code; just confused by the comment.
Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

Forwarding the review request to Mike who has more expertise here than I do.
Attachment #8872119 - Flags: review?(florian) → review?(mconley)
Comment on attachment 8872119 [details]
Bug 1358721 - Avoid flushing layout in adjustTabstrip.

https://reviewboard.mozilla.org/r/143612/#review147654

::: browser/base/content/tabbrowser.xml:6047
(Diff revision 4)
> -            let tab = this.tabbrowser.visibleTabs[this.tabbrowser._numPinnedTabs];
> +              if (this.getAttribute("overflow") == "true") {
> -            if (tab && tab.getBoundingClientRect().width <= this.mTabClipWidth) {
> -              this.setAttribute("closebuttons", "activetab");
> +                this.setAttribute("closebuttons", "activetab");
> -              return;
> +                return;
> -            }
> +              }

Hm. This is the second time I've seen this pattern where we repeat ourselves after a double-nested rAF for something like this. I understand the need - I wonder if we could find some kind of general solution. Like, it's a shame we've had to put the same "deal with overflow" code in two places.

Perhaps it's worth having a comment near lines 6034-6037 referring to this block - "if you change this, change that", etc. At your discretion.
Attachment #8872119 - Flags: review?(mconley) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84a99d06f8db
Avoid flushing layout in adjustTabstrip. r=mconley
https://hg.mozilla.org/mozilla-central/rev/84a99d06f8db
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.7 - Jun 12
Depends on: 1368940
You need to log in before you can comment on or make changes to this bug.