895778 - Optimize code path of CustomizableUIInternal.registerToolbar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Attachment: registerToolbar.patch

My homegrown profiler has shown that registerToolbar is the worst offender of CustomizableUIInternal when opening windows in tpaint.

The attached patch folds some calls to hasAttribute/getAttribute. It also removes isBuildAreaRegistered because the registerBuildArea already protects against a pre-registered build area.

While here I noticed that the aContainer argument of setLocationAttributes is unused so I killed it with fire.

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

With patch:
https://tbpl.mozilla.org/?tree=Try&rev=e527965b8891

Comment 1

[:Gijs (he/him)]

Comment on attachment 778329 [details] [diff] [review]
registerToolbar.patch

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

Driveby feedback - sorry. Looks good, but I have some suggestions/comments...

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ -474,5 @@
>    registerMenuPanel: function(aPanel) {
> -    if (this.isBuildAreaRegistered(CustomizableUI.AREA_PANEL, aPanel)) {
> -      return;
> -    }
> -

Actually, this function gets called whenever the menupanel is opened (ensureRegistered in panelUI is called). Executing all the rest of this function every time seems senseless. If this case of running it twice is not an issue for toolbars, excellent, but then let's move the code here, or to panelUI, so we don't do this more than once...

@@ +1737,3 @@
>        let parent = aElement.parentNode;
> +      let parentFlex = parseInt(parent.getAttribute("flex") || 0, 10);
> +      elementFlex = parseInt(elementFlex, 10);

This will be NaN if add-ons put bogus values in the attribute. Should we use || 0 to care about that?

Also, if we're going for speed, you can just coerce the string value rather than using parseInt (which is slower because it can also deal with "123junk" rather than just "123").

Comment 2

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

Attachment: 895778.patch

Gijs: Thank you for the feedback!

I've updated the patch to use type coercion instead of parseInt and sent it tryserver (also including mochitest-bc), https://tbpl.mozilla.org/?tree=Try&rev=e68904b42bcd

Comment 3

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

I failed on getting mochitest-bc tests to run because I screw up the syntax for the try push. Repushed to try server to just run the mochitest-browser-chrome test suite, https://tbpl.mozilla.org/?tree=Try&rev=1bcde58db628

Comment 4

[:Gijs (he/him)]

Comment on attachment 778678 [details] [diff] [review]
895778.patch

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

LGTM.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 778678 [details] [diff] [review]
895778.patch

Gijs knows this code better than me.

Comment 6

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

Comment on attachment 778678 [details] [diff] [review]
895778.patch

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

Ditto!

Comment 7

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

https://hg.mozilla.org/projects/ux/rev/69e73126983c

Comment 8

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/69e73126983c