Closed Bug 1358173 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.4 - May 1
Tracking Status
firefox55 --- fixed

People

(Reporter: florian, Assigned: florian)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-performance])

Attachments

(1 file, 1 obsolete file)

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
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Flags: qe-verify?
Priority: -- → P1
Attached patch Patch (obsolete) — Splinter Review
This just returns early when removeNotification didn't find any notification to remove.
Attachment #8860085 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8860085 - Flags: review?(gijskruitbosch+bugs) → review-
(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.
Attached patch Patch v2Splinter Review
Added a comment explaining why it makes sense to return early there.
Attachment #8860451 - Flags: review?(gijskruitbosch+bugs)
Attachment #8860085 - Attachment is obsolete: true
Attachment #8860451 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bb5fe78d7b
Removing a non-existent panelUI notification shouldn't be expensive, r=Gijs.
https://hg.mozilla.org/mozilla-central/rev/e8bb5fe78d7b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify? → qe-verify-
No longer blocks: photon-performance-triage
You need to log in before you can comment on or make changes to this bug.