1358173 - Removing an "addon-alert" notification that doesn't exist is 8% of _delayedStartup

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[Florian Quèze [:florian]]

See this profile: https://perfht.ml/2pGOprO

The browser-addon.js initialization running during _delayedStartup does a PanelUI.removeNotification("addon-alert") call at http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/browser/base/content/browser-addons.js#507

This notification most likely doesn't exist (as we are just initializing the new window), but the removeNotification call triggers the expensive _updateNotifications method unconditionally at http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/browser/components/customizableui/content/panelUI.js#231

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

This just returns early when removeNotification didn't find any notification to remove.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8860085 [details] [diff] [review]
Patch

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

Several points to unpack here:

- This should come with a comment about why this is necessary;
- why is _updateNotifications() so expensive, esp. if it should be a no-op?
- can we (additionally) avoid the call to removeNotification() if we haven't added any? Seems like the code in browser-addons.js should be aware of this.

Comment 3

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #2)

> - why is _updateNotifications() so expensive, esp. if it should be a no-op?

From looking at the profile, the expensive parts are _clearNotificationPanel and _clearMenuItems, which both seem to trigger synchronous XBL binding attachment. I think that's when accessing the .children property.

> - can we (additionally) avoid the call to removeNotification() if we haven't
> added any? Seems like the code in browser-addons.js should be aware of this.

I think the browser-addons.js initialization code could be written in a way that avoids it, eg. by adding:
  if (!this.initialized)
    return;
just before the PanelUI.removeNotification call.
But I think whoever wrote that code assumed removing a non-existent notification would be a no-op and cheap enough to not need an additional check... and I think that expectation was reasonable.

Comment 4

[Florian Quèze [:florian]]

Attachment: Patch v2

Added a comment explaining why it makes sense to return early there.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bb5fe78d7b
Removing a non-existent panelUI notification shouldn't be expensive, r=Gijs.

Comment 6

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/e8bb5fe78d7b