896157 - Optimize the CustomizableUIInternal.setLocationAttributes function

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: setLocationAttributes.patch

CustomizableUIInternal.setLocationAttributes is called 300 during the tpaint run, with an average function call cost of 0.075ms and average per-window cost of 1.125ms. During the full run of tpaint, setLocationAttributes is directly responsible for 22.498ms.

This function *should* be really fast, all it does at a high-level is set two attributes on the node that is passed to it. However, this attribute value will trigger some styles to be applied.

The attached patch changes setLocationAttributes so that the costly attribute, customizableui-areatype, is hard-coded on the parent element of the node, and thus doesn't need to be applied to each node. That leaves this function to only set the customizableui-anchorid attribute, which is only useful for items in the menu-panel. This allows the function to have an early return if the area isn't the menu-panel.

Pushed to try server, https://tbpl.mozilla.org/?tree=Try&rev=ffb188dc4bc9

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

s/is called 300 during/is called 300 times during/

Also, a baseline try run to compare the above try results:
https://tbpl.mozilla.org/?tree=Try&rev=67b189bc81b4

Comment 2

[:Gijs (he/him)]

Comment on attachment 778810 [details] [diff] [review]
setLocationAttributes.patch

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

Generally, this makes sense to me, but it does make me sad. I'm going to clear review and wait for mconley's feedback here; Personally I'd prefer not to have to do this because our selectors become uglier again. You might also want to check with Dao to see if he has ideas...

(some questions about OS X styles which I don't quite understand follow)

::: browser/themes/osx/browser.css
@@ +8,5 @@
>  %filter substitution
>  %define forwardTransitionLength 150ms
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
>  %define conditionalForwardWithUrlbarWidth 27
> +%define toolbarButtonPressed > :hover:active:not([disabled="true"])

Why the switch to a child selector?

@@ +416,5 @@
>    #forward-button:hover:active:not([disabled="true"]) {
>      -moz-image-region: rect(18px, 72px, 36px, 54px);
>    }
>  
> +  :not([customizableui-areatype="menu-panel"]) > #home-button @toolbarButtonPressed@ {

Why not just [customizableui-areatype="toolbar"] ?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 778810 [details] [diff] [review]
> setLocationAttributes.patch
> 
> Review of attachment 778810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Generally, this makes sense to me, but it does make me sad. I'm going to
> clear review and wait for mconley's feedback here; Personally I'd prefer not
> to have to do this because our selectors become uglier again. You might also
> want to check with Dao to see if he has ideas...
> 
> (some questions about OS X styles which I don't quite understand follow)
> 
> ::: browser/themes/osx/browser.css
> @@ +8,5 @@
> >  %filter substitution
> >  %define forwardTransitionLength 150ms
> >  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
> >  %define conditionalForwardWithUrlbarWidth 27
> > +%define toolbarButtonPressed > :hover:active:not([disabled="true"])
> 
> Why the switch to a child selector?
> 
> @@ +416,5 @@
> >    #forward-button:hover:active:not([disabled="true"]) {
> >      -moz-image-region: rect(18px, 72px, 36px, 54px);
> >    }
> >  
> > +  :not([customizableui-areatype="menu-panel"]) > #home-button @toolbarButtonPressed@ {
> 
> Why not just [customizableui-areatype="toolbar"] ?

Both of these are just me breaking apart @toolbarButtonPressed@ because the attribute moved to the parent node of the targeted element. The child selector was already part of @toolbarButtonPressed@.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #3)
> The child selector was already part of @toolbarButtonPressed@.

Umm, no, it wasn't.

Do I interpret http://perf.snarkfest.net/compare-talos/index.html?oldRevs=67b189bc81b4&newRev=ffb188dc4bc9&submit=true right such that this didn't improve tpaint numbers?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to Jared Wein [:jaws] from comment #3)
> > The child selector was already part of @toolbarButtonPressed@.
> 
> Umm, no, it wasn't.

Sorry, I should have looked before commenting. This was a mistake and I can remove that child selector.
 
> Do I interpret
> http://perf.snarkfest.net/compare-talos/index.
> html?oldRevs=67b189bc81b4&newRev=ffb188dc4bc9&submit=true right such that
> this didn't improve tpaint numbers?

I triggered more runs and the numbers are slightly better. My earlier run on tryserver (https://tbpl.mozilla.org/?tree=Try&rev=a423328c6c9a) showed better results so I'll have to investigate the differences here.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 778810 [details] [diff] [review]
setLocationAttributes.patch

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

So are we absolutely sure this improves performance?

::: browser/themes/osx/browser.css
@@ +416,5 @@
>    #forward-button:hover:active:not([disabled="true"]) {
>      -moz-image-region: rect(18px, 72px, 36px, 54px);
>    }
>  
> +  :not([customizableui-areatype="menu-panel"]) > #home-button @toolbarButtonPressed@ {

I agree - that seems simpler.

@@ +425,5 @@
>      -moz-image-region: rect(18px, 144px, 36px, 126px);
>    }
>  
>    #bookmarks-menu-button[open] {
>      -moz-image-region: rect(36px, 144px, 54px, 126px);

Why aren't you checking the parent areatype here?

@@ +437,5 @@
>      -moz-image-region: rect(18px, 180px, 36px, 162px);
>    }
>  
>    #history-panelmenu[open] {
>      -moz-image-region: rect(36px, 180px, 54px, 162px);

Why aren't you checking the parent areatype here?

@@ +502,5 @@
>      -moz-image-region: rect(18px, 450px, 36px, 432px);
>    }
>  
>    #developer-button[open] {
>      -moz-image-region: rect(36px, 450px, 54px, 432px);

Why aren't you checking the parent areatype here?

@@ +558,4 @@
>      -moz-image-region: rect(36px, 432px, 54px, 414px);
>    }
>  
>    #fullscreen-button[checked="true"]@toolbarButtonPressed@ {

Why aren't you checking the parent areatype here?

@@ +917,5 @@
>    }
>  
> +  [customizableui-areatype="menu-panel"] > #bookmarks-menu-button[starred],
> +  toolbarpaletteitem[place="palette"] > #bookmarks-menu-button[starred],
> +  toolbarpaletteitem[place="panel"] > #bookmarks-menu-button[starred] {

The starred state shouldn't be represented in customization mode.

@@ +930,5 @@
>  
> +  [customizableui-areatype="menu-panel"] > #downloads-button,
> +  [customizableui-areatype="menu-panel"] > #downloads-indicator,
> +  toolbarpaletteitem[place="palette"] > #downloads-button,
> +  toolbarpaletteitem[place="palette"] > #downloads-indicator,

The downloads indicator is never visible during customization, so rules for it within a toolbarpaletteitem can be removed.

::: browser/themes/shared/menupanel.inc.css
@@ +18,5 @@
>    -moz-image-region: rect(0px, 160px, 32px, 128px);
>  }
>  
> +[customizableui-areatype="menu-panel"] > #bookmarks-menu-button[starred],
> +toolbarpaletteitem[place="palette"] > #bookmarks-menu-button,

These last two selectors override the above group. We shouldn't represent the starred state in customization mode anyways.

@@ +31,5 @@
>  }
>  
> +[customizableui-areatype="menu-panel"] > #downloads-button,
> +[customizableui-areatype="menu-panel"] > #downloads-indicator,
> +toolbarpaletteitem[place="palette"] > #downloads-button,

As above, no need to have rules for the downloads indicator within toolbarpaletteitems.

::: browser/themes/shared/toolbarbuttons.inc.css
@@ +29,5 @@
>    -moz-image-region: rect(0, 144px, 18px, 126px);
>  }
>  
> +[customizableui-areatype="toolbar"] > #bookmarks-menu-button[starred],
> +toolbarpaletteitem[place="toolbar"] > #bookmarks-menu-button[starred] {

Remove customization mode starred state rule.

@@ +40,5 @@
>  }
>  
> +[customizableui-areatype="toolbar"] > #downloads-indicator,
> +[customizableui-areatype="toolbar"] > #downloads-button,
> +toolbarpaletteitem[place="toolbar"] > #downloads-indicator,

Remove downloads-indicator customization rules.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 896157.patch

Addressed review comments and repushed to try on top of the latest m-c to ux merge.

https://tbpl.mozilla.org/?tree=Try&rev=d518dc75700f

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Baseline try push, https://tbpl.mozilla.org/?tree=Try&rev=fdf4d28c6198

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 780535 [details] [diff] [review]
896157.patch

I'll hold off on the review request until the try numbers come back.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

The numbers that came back weren't conclusive enough to make this change worthwhile. Averaging the tpaint numbers they went from 147.8 to 148.5 (actually a decrease in performance, surprisingly, but that could also just be within the range of noise).