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

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Bogdan Maris, Desktop QA]

[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.

Comment 1

[Bogdan Maris, Desktop QA]

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

Comment 2

[Bogdan Maris, Desktop QA]

This is not reproducible with other lightweight themes or Default theme.

Comment 3

[Dão Gottwald [:dao]]

Could you reproduce this on Windows or Linux?

Comment 4

[Bogdan Maris, Desktop QA]

(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.

Comment 5

[:Gijs (he/him)]

(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.

Comment 6

[Bogdan Maris, Desktop QA]

(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

Comment 7

[Bogdan Maris, Desktop QA]

The issue is reproducible on all three machines from above.

Comment 8

[:Gijs (he/him)]

(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?

Comment 9

[Markus Stange [:mstange]]

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?

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Setting priority to P3 because this seems to affect only specific versions of OS X. This does not block.

Comment 11

[Bogdan Maris, Desktop QA]

(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

Comment 12

[Markus Stange [: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.

Comment 13

[Dão Gottwald [:dao]]

(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?

Comment 14

[Dão Gottwald [:dao]]

(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?

Comment 15

[Markus Stange [:mstange]]

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).

Comment 16

[Dão Gottwald [:dao]]

(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.

Comment 17

[Dão Gottwald [:dao]]

(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.

Comment 18

[:Gijs (he/him)]

Dão, was there more to do here besides the deps that already got fixed?

Comment 19

[Dão Gottwald [:dao]]

(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.

Comment 20

[Bogdan Maris, Desktop QA]

(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

Comment 21

[Marco Mucci [:MarcoM]]

Discussed with Florian - setting as a meta bug.