Closed Bug 890105 Opened 7 years ago Closed 6 years ago

TabsInTitleBar._update should group measurements and style changes to avoid unnecessary reflows

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Keywords: perf, Whiteboard: [Australis:M8][qa-])

Attachments

(3 files, 2 obsolete files)

I think there may be opportunities to avoid reflows and short-circuit some calculations.
Try push to measure the total cost of TabsInTitlebar.init:
https://tbpl.mozilla.org/?tree=Try&rev=7500d4239790
(In reply to Matthew N. [:MattN] from comment #1)
> Try push to measure the total cost of TabsInTitlebar.init:

That push was just to measure the cost of TabsInTitlebar. This is on average 0.25% better than UX rev. ca7577238ef4 across platforms. 

Med. is 2.3% (23ms) faster and max is 1.4% (13ms) faster on 10.7 compared to UX.
Med. and max. are 1.6% (8.6ms) faster on XP Non-PGO compared to UX.

Here is an approach which removes reflows and is actually something that could land.
Avoid reflows: https://tbpl.mozilla.org/?tree=Try&rev=3e01f799c904
(In reply to Matthew N. [:MattN] from comment #2)
> Here is an approach which removes reflows and is actually something that
> could land.

Results are in and it's a net win for dirty places and tpaint. It also significantly  improved tresize on most platforms. I'll cleanup the patch and attach for review when I get a chance. I need to double-check that the default attribute doesn't break things.

I need to test this more, make sure I'm not doing something stupid by changing the order, and possible write a test to ensure we don't introduce reflows in the future (similar to http://hg.mozilla.org/mozilla-central/annotate/bc99f68f8946/browser/base/content/test/browser_tabopen_reflows.js ).

See the try changeset if you want to test with a reflow observer outputting to the console.
Attachment #771520 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 771520 [details] [diff] [review]
WIP.1. Re-order changes and set initial value of attribute

Review of attachment 771520 [details] [diff] [review]:
-----------------------------------------------------------------

Generally this looks good already, but I added some more suggestions for possible optimizations.

::: browser/base/content/browser.js
@@ +4454,5 @@
>  
>        let captionButtonsBox = $("titlebar-buttonbox");
> +      let captionButtonsBoxWidth = rect(captionButtonsBox).width;
> +      let titlebarContentHeight = rect(titlebarContent).height;
> +      let menuHeight = this._outerHeight(menubar);

This can be in #ifndef XP_MACOSX, and otherwise just set to 0, as we don't have a menubar on OS X.

@@ +4465,5 @@
>  #endif
> +
> +      this._sizePlaceholder("caption-buttons", captionButtonsBoxWidth);
> +#ifdef XP_MACOSX
> +      this._sizePlaceholder("fullscreen-button", fullscreenButtonWidth);

Can we inline these calls and rather than calling document.querySelectorAll, use:

tabsToolbar.getElementsByAttribute("type", "caption-buttons")[0].width = captionButtonsBoxWidth;
#ifdef XP_MACOSX
tabsToolbar.getElementsByAttribute("type", "fullscreen-button")[0].width = fullscreenButtonWidth;
#else
menubar.getElementsByAttribute("type", "caption-buttons")[0].width = captionButtonsBoxWidth;
#endif

That or use menubar/tabsToolbar.querySelector. I would be surprised if there was a significant difference. In any case, Array.forEach over document.querySelectorAll with an attribute selector seems less than ideal, especially on OS X where we'll be sizing menubar elements that will never see the light of day. Might even want to #ifdef those in browser.xul.

::: browser/base/content/browser.xul
@@ +41,5 @@
>          titlemodifier="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@"
>          titlemodifier_normal="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@"
>          titlemodifier_privatebrowsing="&mainWindow.titlemodifier;@PRE_RELEASE_SUFFIX@ &mainWindow.titlePrivateBrowsingSuffix;"
>  #endif
> +        tabsintitlebar="true"

Do we do this on Linux already? Also, what about popup windows? I guess those will now readjust to not have it when our code runs - but please check that that is the case.
Attachment #771520 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #4)
> >  #endif
> > +
> > +      this._sizePlaceholder("caption-buttons", captionButtonsBoxWidth);
> > +#ifdef XP_MACOSX
> > +      this._sizePlaceholder("fullscreen-button", fullscreenButtonWidth);
> 
> Can we inline these calls and rather than calling document.querySelectorAll,
> use:
> 
> tabsToolbar.getElementsByAttribute("type", "caption-buttons")[0].width =
> captionButtonsBoxWidth;
> #ifdef XP_MACOSX
> tabsToolbar.getElementsByAttribute("type", "fullscreen-button")[0].width =
> fullscreenButtonWidth;
> #else
> menubar.getElementsByAttribute("type", "caption-buttons")[0].width =
> captionButtonsBoxWidth;
> #endif
> 
> That or use menubar/tabsToolbar.querySelector. I would be surprised if there
> was a significant difference. In any case, Array.forEach over
> document.querySelectorAll with an attribute selector seems less than ideal,

It's a class selector with an extra attribute check. (Which, by the way, means you should also check the class name in addition to the type attribute, because the latter isn't reserved in any way.) Please only make that change if you actually find it to be faster, which I doubt.
Comment on attachment 771520 [details] [diff] [review]
WIP.1. Re-order changes and set initial value of attribute

>+      let menuHeight = this._outerHeight(menubar);
>+      let tabsToolbar = $("TabsToolbar");
>+      let tabAndMenuHeight = this._outerHeight(tabsToolbar) + menuHeight;
...
>       // If the titlebar is taller than the menubar, add more padding to the
>       // bottom of the menubar so that it matches.
>       if (menuHeight && titlebarContentHeight > menuHeight) {
>         let menuTitlebarDelta = titlebarContentHeight - menuHeight;
>         menubar.style.paddingBottom = menuTitlebarDelta + "px";
>         menuHeight += menuTitlebarDelta;
>       }
> 
>       // Next, we calculate how much we need to stretch the titlebar down to
>       // go all the way to the bottom of the tab strip.
>-      let tabsToolbar = $("TabsToolbar");
>-      let tabAndMenuHeight = this._outerHeight(tabsToolbar) + menuHeight;
>       titlebarContent.style.marginBottom = tabAndMenuHeight + "px";

This looks like a behavior change. tabAndMenuHeight included menuTitlebarDelta, now it wouldn't.
Component: Theme → General
Summary: Try to optimize TabsInTitlebar calculations → TabsInTitleBar._update should group measurements and style changes to avoid unnecessary reflows
(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > >  #endif
> > > +
> > > +      this._sizePlaceholder("caption-buttons", captionButtonsBoxWidth);
> > > +#ifdef XP_MACOSX
> > > +      this._sizePlaceholder("fullscreen-button", fullscreenButtonWidth);
> > 
> > Can we inline these calls and rather than calling document.querySelectorAll,
> > use:
> > 
> > tabsToolbar.getElementsByAttribute("type", "caption-buttons")[0].width =
> > captionButtonsBoxWidth;
> > #ifdef XP_MACOSX
> > tabsToolbar.getElementsByAttribute("type", "fullscreen-button")[0].width =
> > fullscreenButtonWidth;
> > #else
> > menubar.getElementsByAttribute("type", "caption-buttons")[0].width =
> > captionButtonsBoxWidth;
> > #endif
> > 
> > That or use menubar/tabsToolbar.querySelector. I would be surprised if there
> > was a significant difference. In any case, Array.forEach over
> > document.querySelectorAll with an attribute selector seems less than ideal,
> 
> It's a class selector with an extra attribute check. (Which, by the way,
> means you should also check the class name in addition to the type
> attribute, because the latter isn't reserved in any way.) Please only make
> that change if you actually find it to be faster, which I doubt.

From a quick check in the browser console, getElementsByAttribute isn't any faster than node.querySelector. However, either that or the limited node.querySelector is significantly better than document.querySelectorAll, even if we have to do it twice, once for each container. Anyway, the primary point was, let's not do the unnecessary work of changing the items in the menubar when we're on OS X.
Component: General → Theme
Summary: TabsInTitleBar._update should group measurements and style changes to avoid unnecessary reflows → Try to optimize TabsInTitlebar calculations
Summary: Try to optimize TabsInTitlebar calculations → TabsInTitleBar._update should group measurements and style changes to avoid unnecessary reflows
Component: Theme → General
Keywords: perf
(In reply to :Gijs Kruitbosch from comment #4)
> Generally this looks good already, but I added some more suggestions for
> possible optimizations. 

We can handle those optimizations in a new bug if necessary. Let's keep this bug focused on the reflows in TabsInTitlebar (of which there are none after my patch).

> Might even want to #ifdef those in browser.xul.

We're already talking about this code doing stuff to the menubar on OS X in bug 851652. 

> ::: browser/base/content/browser.xul
> > +        tabsintitlebar="true"
> 
> Do we do this on Linux already? 

I wrapped this in ifdef CAN_DRAW_IN_TITLEBAR so it doesn't cause a reflow for Linux.

I tested on XP with popups and different font/caption button sizes and everything seemed the same compared to UX Nightly.

(In reply to Dão Gottwald [:dao] from comment #6)
> This looks like a behavior change. tabAndMenuHeight included
> menuTitlebarDelta, now it wouldn't.

Fixed.
Attachment #771520 - Attachment is obsolete: true
Attachment #773156 - Flags: review?(gijskruitbosch+bugs)
Based on browser_tabopen_reflows.js.

I was wrong in my last comment about there being no uninterruptible reflows from TabsInTitlebar._update after my patch. There is still one on OS X but I don't know what is making the layout info dirty but I think it's some other code in an unrelated function. As a result I don't think that needs to be solved in this bug.
Attachment #773171 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 773156 [details] [diff] [review]
v.1 Fix correctness issues

Review of attachment 773156 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Please do include the #ifndef XP_MACOSX for the menubar in the work for bug 851652.
Attachment #773156 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 773171 [details] [diff] [review]
Test v.1 - Add a test to check for new uninterruptible reflows during window creation

Review of attachment 773171 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/browser_windowopen_reflows.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +"use strict";

Out of curiosity, why this?

@@ +19,5 @@
> +  // Overflowable toolbar calculations causes reflows
> +  "OverflowableToolbar.prototype._onOverflow@resource:///modules/CustomizableUI.jsm|" +
> +    "OverflowableToolbar.prototype.init@resource:///modules/CustomizableUI.jsm|" +
> +    "OverflowableToolbar.prototype.observe@resource:///modules/CustomizableUI.jsm|" +
> +    "gBrowserInit._delayedStartup@chrome://browser/content/browser.js|",

I'm not convinced this reliably fires before MozAfterPaint. In principle, it shouldn't anymore, AFAIK. But I guess you tested it? If so, please file a followup bug.

@@ +27,5 @@
> +  // TabsInTitlebar._update causes a reflow on OS X trying to do calculations since layout info is already dirty.
> +  EXPECTED_REFLOWS.push("rect@chrome://browser/content/browser.js|" +
> +                          "TabsInTitlebar._update@chrome://browser/content/browser.js|" +
> +                          "updateAppearance@chrome://browser/content/browser.js|" +
> +                          "handleEvent@chrome://browser/content/tabbrowser.xml|");

Fascinating. Please file a followup bug for this, we can determine prio depending on how close this bug gets us to m-c parity on OS X and Windows for ts_dirtypaint.
Attachment #773171 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 773156 [details] [diff] [review]
v.1 Fix correctness issues

>+#ifdef XP_MACOSX
>+      let fullscreenButtonWidth = rect($("titlebar-fullscreen-button")).width;
>+
>+      // Begin setting CSS properties which will cause a reflow
>+      this._sizePlaceholder("fullscreen-button", fullscreenButtonWidth);
>+#endif

#ifdef XP_MACOSX
      let fullscreenButtonWidth = rect($("titlebar-fullscreen-button")).width;
#endif

      // Begin setting CSS properties which will cause a reflow
#ifdef XP_MACOSX
      this._sizePlaceholder("fullscreen-button", fullscreenButtonWidth);
#endif
Comment on attachment 773156 [details] [diff] [review]
v.1 Fix correctness issues

Landed patch with comment 13 addressed: https://hg.mozilla.org/projects/ux/rev/727132bea25c

I need to investigate the unexpected reflows on Try.
Attachment #773156 - Flags: checkin+
There were improvements on most platforms :)
Note that I added the reflow with a JSM in the stack twice because the resource URI differs between local builds and on Try. I suspect this is due to omni.ja.

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 773171 [details] [diff] [review]
> > +"use strict";
> 
> Out of curiosity, why this?

It's preferred to add this to all new JS files to get better warnings.

> @@ +19,5 @@
> > +  // Overflowable toolbar calculations causes reflows
> > +  "OverflowableToolbar.prototype._onOverflow@resource:///modules/CustomizableUI.jsm|" +
> 
> I'm not convinced this reliably fires before MozAfterPaint. In principle, it
> shouldn't anymore, AFAIK. But I guess you tested it? If so, please file a
> followup bug.

It still does on UX tip for OS X only. Filed as bug 901415.

> @@ +27,5 @@
> > +  // TabsInTitlebar._update causes a reflow on OS X trying to do calculations since layout info is already dirty.
> > +  EXPECTED_REFLOWS.push("rect@chrome://browser/content/browser.js|" +
> > +                          "TabsInTitlebar._update@chrome://browser/content/browser.js|" +
> > +                          "updateAppearance@chrome://browser/content/browser.js|" +
> > +                          "handleEvent@chrome://browser/content/tabbrowser.xml|");
> 
> Fascinating. Please file a followup bug for this, we can determine prio
> depending on how close this bug gets us to m-c parity on OS X and Windows
> for ts_dirtypaint.

I think this may just be a timing difference i.e. it's in the tested time period on OS X but not other platforms? Also, since you mentioned Windows, I want to clarify that this is only on OS X which is why it's in the OS X block. I'm not sure why we want a bug on this since a reflow make sense in rect?
Attachment #773171 - Attachment is obsolete: true
Attachment #785622 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 785622 [details] [diff] [review]
Test v.1.1 - Updated expected reflows on r+

Review of attachment 785622 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/browser_windowopen_reflows.js
@@ +23,5 @@
>  
> +if (Services.appinfo.OS == "Darwin") {
> +  // TabsInTitlebar._update causes a reflow on OS X trying to do calculations
> +  // since layout info is already dirty. This doesn't seem to happen before
> +  // MozAfterPaint on other platforms.

Wut. Surely that code reflows on Windows as well? Maybe we call resize() earlier on OS X and that causes this to fire within the bounds on OS X and outside of them on Windows... That'd also explain the overflowable toolbar hit. We can probably see if we can delay this still further in OS X if we need to.
Attachment #785622 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed the test: https://hg.mozilla.org/projects/ux/rev/b058f9b111b4
Whiteboard: [Australis:M8][fixed-in-ux]
(In reply to Matthew N. [:MattN] from comment #19)
> Follow-ups to workaround bug 906578:
2nd one should have been https://hg.mozilla.org/projects/ux/rev/0e1a2fa8d2dd
https://hg.mozilla.org/mozilla-central/rev/727132bea25c
https://hg.mozilla.org/mozilla-central/rev/b058f9b111b4
https://hg.mozilla.org/mozilla-central/rev/031395261220
https://hg.mozilla.org/mozilla-central/rev/0e1a2fa8d2dd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Target Milestone: --- → Firefox 28
Whiteboard: [Australis:M8] → [Australis:M8][qa-]
You need to log in before you can comment on or make changes to this bug.