[meta] Very slow width resize action when multiple tabs are pinned using compact themes

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bogdan_maris, Unassigned)

Tracking

(Blocks 1 bug, {meta})

Trunk
Firefox 55
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 unaffected, firefox52 unaffected, firefox53 wontfix, firefox54 fix-optional, firefox55 fixed)

Details

(Whiteboard: [photon-performance])

[Affected versions]:
- latest Developer Edition 53.0a2
- latest Nightly 54.0a1
- Firefox 52 beta 6 using compact theme (https://addons.mozilla.org/en-Us/firefox/addon/devedition-theme-enabler/)

[Affected platforms]:
- macOS 10.12.3

[Steps to reproduce]:
1. Start Firefox
2. Change the theme to one of the compact themes (about:addons/appearance)
3. Open multiple tabs (eg.10) and pin them
4. Grab Fx from one side and shrink the width of the browser

[Expected result]:
- The transition is smooth

[Actual result]:
- Very laggy transition when changing the width of Fx.

[Regression range]:
- This is not a recent regression in Compact Themes feature since it reproduces in Nightly from 2017-01-17 as well.

[Additional notes]:
- Screencast showing the issue is attached.
- I noted that 52 and 51 are not affected because to reproduce on those builds it is required to install the addon from amo. The tracking can be changed if considered otherwise.
And this is the screencast showing the issue (this was to large to upload it to Bugzilla).
https://dl.dropboxusercontent.com/u/109148197/New%20slow%20transition.mov
This is not reproducible with other lightweight themes or Default theme.
Could you reproduce this on Windows or Linux?
Flags: needinfo?(bogdan.maris)
(In reply to Dão Gottwald [:dao] from comment #3)
> Could you reproduce this on Windows or Linux?

No, this is only reproducible on macOS 10.12.3. I've also tested using Mac OS X 10.11.6 but was unable to see this issue.
Flags: needinfo?(bogdan.maris)

Comment 5

2 years ago
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > Could you reproduce this on Windows or Linux?
> 
> No, this is only reproducible on macOS 10.12.3. I've also tested using Mac
> OS X 10.11.6 but was unable to see this issue.

Have you tested multiple machines with 10.12.3? Seems unlikely (but possible!) this is an OS X version issue, and more likely that it's to do with the graphics card, 3rd party software, or something else specific to that machine.
Flags: needinfo?(bogdan.maris)
(In reply to :Gijs from comment #5)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #4)
> > (In reply to Dão Gottwald [:dao] from comment #3)
> > > Could you reproduce this on Windows or Linux?
> > 
> > No, this is only reproducible on macOS 10.12.3. I've also tested using Mac
> > OS X 10.11.6 but was unable to see this issue.
> 
> Have you tested multiple machines with 10.12.3? Seems unlikely (but
> possible!) this is an OS X version issue, and more likely that it's to do
> with the graphics card, 3rd party software, or something else specific to
> that machine.

I tested on two different iMacs and one MacBook Pro (10.12.1, 10.12.3 and 10.12.4):
 - Intel Core i5 2.7 GHz, graphics Intel Iris Pro
 - Intel Core i5 2.5 GHz, AMD Radeon HD 6750M
 - Intel Core i5 2.7 Ghz, Intel Iris Graphics 6100 1536MB
Flags: needinfo?(bogdan.maris) → needinfo?(gijskruitbosch+bugs)
The issue is reproducible on all three machines from above.

Comment 8

2 years ago
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #6)
> (In reply to :Gijs from comment #5)
> > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #4)
> > > (In reply to Dão Gottwald [:dao] from comment #3)
> > > > Could you reproduce this on Windows or Linux?
> > > 
> > > No, this is only reproducible on macOS 10.12.3. I've also tested using Mac
> > > OS X 10.11.6 but was unable to see this issue.
> > 
> > Have you tested multiple machines with 10.12.3? Seems unlikely (but
> > possible!) this is an OS X version issue, and more likely that it's to do
> > with the graphics card, 3rd party software, or something else specific to
> > that machine.
> 
> I tested on two different iMacs and one MacBook Pro (10.12.1, 10.12.3 and
> 10.12.4):
>  - Intel Core i5 2.7 GHz, graphics Intel Iris Pro
>  - Intel Core i5 2.5 GHz, AMD Radeon HD 6750M
>  - Intel Core i5 2.7 Ghz, Intel Iris Graphics 6100 1536MB

This sounds like a native widget issue, then. Markus, can you take a look or redirect to someone who can?
Component: Theme → Widget: Cocoa
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mstange)
Product: Firefox → Core
I'm not able to reproduce this bug on my machine, using macOS 10.12.3 Beta (16D25a).

Bogdan, can you get a profile of the problem using the addon from https://perf-html.io/ and share the link to it?
Flags: needinfo?(mstange) → needinfo?(bogdan.maris)
Setting priority to P3 because this seems to affect only specific versions of OS X. This does not block.
Priority: -- → P3
(In reply to Markus Stange [:mstange] from comment #9)
> I'm not able to reproduce this bug on my machine, using macOS 10.12.3 Beta
> (16D25a).
> 
> Bogdan, can you get a profile of the problem using the addon from
> https://perf-html.io/ and share the link to it?

Sorry for the late reply. Here is the link requested: https://perfht.ml/2moFhcP
Flags: needinfo?(bogdan.maris) → needinfo?(mstange)
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.
Component: Widget: Cocoa → Tabbed Browser
Flags: needinfo?(mstange) → needinfo?(dao+bmo)
Product: Core → Firefox
(In reply to Markus Stange [:mstange] from 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.

Resizing a window should call _positionPinnedTabs once at most if and when it causes the tab strip to overflow or underflow (and this isn't specific to compact themes). So that seems unlikely to be the biggest problem here if the whole resize operation is janky. Or are you seeing that _positionPinnedTabs is called multiple times?
Flags: needinfo?(dao+bmo) → needinfo?(mstange)
(In reply to Dão Gottwald [::dao] from comment #13)
> Resizing a window should call _positionPinnedTabs once at most if and when
> it causes the tab strip to overflow or underflow (and this isn't specific to
> compact themes).

It shouldn't be specific to Mac either. Has somebody reproduced this elsewhere?
Flags: needinfo?(bogdan.maris)
What I'm seeing in the profile is that each refresh tick during the resize takes 50ms: https://perfht.ml/2oLUkPa
And 25ms of each of those is spent within _positionPinnedTabs. It's probably just one call to _positionPinnedTabs per refresh tick, but *inside* of that function there is a loop. We flush and invalidate styles for each pinned tab, which adds up if there are lots of them.

I'm also seeing lots of time spent in frame construction. Are we toggling the "position" property on these tabs between "static" and "fixed"? Or are we removing tabs from the DOM and re-inserting them? Both of those require frame reconstruction.

And the other problem I see in the profile, which is probably not caused by front-end code, is that during the resize there are very long gaps between calls to -[NSWindow setFrame:display:]. That function is where the window size actually changes. The time between those calls is filled with refresh driver ticks. Maybe there's an animation running, and we desperately try to repaint that animation multiple times before giving the system a chance to actually resize the window? But that doesn't explain why the "overflow" event handler ("onxbloverflow") is called on each of those refresh ticks; the overflow event shouldn't need to fire when the window size hasn't changed.

So it seems there are multiple bugs here: Slow overflow event handlers (front-end specific) + starved window resizing while there are remaining slow Gecko events in the Gecko event loop (Mac-specific).
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #15)
> What I'm seeing in the profile is that each refresh tick during the resize
> takes 50ms: https://perfht.ml/2oLUkPa
> And 25ms of each of those is spent within _positionPinnedTabs. It's probably
> just one call to _positionPinnedTabs per refresh tick, but *inside* of that
> function there is a loop. We flush and invalidate styles for each pinned
> tab, which adds up if there are lots of them.

So _positionPinnedTabs is called way too often. This is definitely broken.

> I'm also seeing lots of time spent in frame construction. Are we toggling
> the "position" property on these tabs between "static" and "fixed"?

Yes, but it's (supposed to be) a one-time switch when overflowing / underflowing.

> But that doesn't explain why the "overflow"
> event handler ("onxbloverflow") is called on each of those refresh ticks;
> the overflow event shouldn't need to fire when the window size hasn't
> changed.

There's an overflow event commonly originating from tab elements (from bug 658467), but the overflow handler bails out early in that case. It sounds like in this case we're getting other unexpected overflow events.
Depends on: 1354781
Depends on: 1354782
(In reply to Dão Gottwald [::dao] from comment #14)
> (In reply to Dão Gottwald [::dao] from comment #13)
> > Resizing a window should call _positionPinnedTabs once at most if and when
> > it causes the tab strip to overflow or underflow (and this isn't specific to
> > compact themes).
> 
> It shouldn't be specific to Mac either. Has somebody reproduced this
> elsewhere?

Already answered in comment 4.
Flags: needinfo?(bogdan.maris)
OS: All → Mac OS X
Depends on: 1354789

Comment 18

2 years ago
Dão, was there more to do here besides the deps that already got fixed?
Flags: needinfo?(dao+bmo)
Flags: qe-verify?
Priority: P3 → P2
Whiteboard: [photon-performance]
(In reply to :Gijs from comment #18)
> Dão, was there more to do here besides the deps that already got fixed?

We'll need confirmation from Bogdan on whether the symptom is fixed, and another profile to see if _positionPinnedTabs is still called too often, and if so figure out why.
Flags: needinfo?(dao+bmo) → needinfo?(bogdan.maris)
(In reply to Dão Gottwald [::dao] from comment #19)
> (In reply to :Gijs from comment #18)
> > Dão, was there more to do here besides the deps that already got fixed?
> 
> We'll need confirmation from Bogdan on whether the symptom is fixed, and
> another profile to see if _positionPinnedTabs is still called too often, and
> if so figure out why.

I just tried again using latest Nightly 55.0a1 (buildID: 20170417030204) on macOS 10.12.4 and I can't reproduce this issue anymore (Firefox is no longer slow on resize).
Here is another profile I used: https://perfht.ml/2pcEtJF
Flags: needinfo?(bogdan.maris) → needinfo?(dao+bmo)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(dao+bmo)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Discussed with Florian - setting as a meta bug.
Flags: qe-verify? → qe-verify-
Keywords: meta
Priority: P2 → --
QA Contact: bogdan.maris
Summary: Very slow width resize action when multiple tabs are pinned using compact themes → [meta] Very slow width resize action when multiple tabs are pinned using compact themes
You need to log in before you can comment on or make changes to this bug.