1818461 - UpdateEmptyToolbarMessage is doing expensive work during bookmark operations

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P3)

Description

[Marco Bonardo [:mak] (Away 10-21 August)]

I took a profile from the suggestion in bug 1533061
https://share.firefox.dev/3XWswU0

on notifications we're spending 19% of the time in updateEmptyToolbarMessage
https://searchfox.org/mozilla-central/rev/75da1dd5d4b9b991f919a41594194eab93cdef62/browser/base/content/browser-places.js#1630,2212

in particular we seem to execute it regardless of the notification, this final catch-all call is bad, it should only activate when changes are made to bookmarks on the toolbar. For example "bookmark-moved" is doing that (that's worse cause in that case we do it twice).

It also sounds like something we could cache, and then just update using the notifications.

This expensive path is actually taken every time we can't find DOM children on the toolbar, but the querySelector is wrong
https://searchfox.org/mozilla-central/rev/75da1dd5d4b9b991f919a41594194eab93cdef62/browser/base/content/browser-places.js#1612-1616
:scope > toolbarbutton:not([hidden]), doesn't match bookmarks on the toolbar here.

Finally, PlacesUtils.getChildCountForFolder is doing synchronous IO, but that is likely something we knew already, we just failed avoiding excessive calls. Of course if necessary we can expose an async API from Bookmarks, but the code must be refactored to async.

Comment 1

[Bas Schouten (:bas.schouten)]

The Performance Impact Calculator has determined this bug's performance impact to be medium. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: [x] Windows [x] macOS [x] Linux
Impact on browser: Causes noticeable jank

Comment 2

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #0)

This expensive path is actually taken every time we can't find DOM children on the toolbar, but the querySelector is wrong
https://searchfox.org/mozilla-central/rev/75da1dd5d4b9b991f919a41594194eab93cdef62/browser/base/content/browser-places.js#1612-1616
:scope > toolbarbutton:not([hidden]), doesn't match bookmarks on the toolbar here.

FWIW, I think it's not supposed to - it's supposed to match other buttons that get customized onto the toolbar. We also explicitly exclude #personal-bookmarks from the toolbaritem selector, so I'm fairly sure this was intentional at the time.

(Of course, it seems likely there is some better way of doing what the code is trying to do, but that also feels like a separate question...)

Comment 3

[Marco Bonardo [:mak] (Away 10-21 August)]

(In reply to :Gijs (he/him) from comment #2)

FWIW, I think it's not supposed to - it's supposed to match other buttons that get customized onto the toolbar. We also explicitly exclude #personal-bookmarks from the toolbaritem selector, so I'm fairly sure this was intentional at the time.

Ah I see.
That's still a bit surprising, it would be much cheaper to check if bookmark nodes exist on the toolbar through the DOM, than by querying Places.

Comment 5

[Marco Bonardo [:mak] (Away 10-21 August)]

Attachment: Bug 1818461 - Remove the synchronous PlacesUtils.getChildCountForFolder().r=daisuke!,gijs!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8581ee7357f8
Remove the synchronous PlacesUtils.getChildCountForFolder().r=daisuke,Gijs

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/8581ee7357f8