1392533 - Make the view directly communicate batches to the result

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P1)

Description

[Marco Bonardo [:mak]]

Currently the view (through the controller) executes a bunch of operations using the bookmarking API. The old API in certain cases was starting a "batch" that basically consisted in sending 2 notification onBegin/EndBatch and grouping writes into a transaction.

For the transaction, we'll try to provider more batchins APIs (like removeMany, insertMany...).
For the batch notifications, it sounds pointless for the view to use an API that sends a notification to the result, when it can directly communicate to the result.  More specifically, the view knows when it's about to do a large change, while the API doesn't know.

Additionally, the new bookmarking API doesn't have the concept of batching, since in the async world it is a bit more challenging.

The only things using batch notifications atm are:
* nsNavHistoryResult: the view can directly talk with the result, no need to go through 2 layers of abstraction.
* NewTabUtils.jsm: seems to do this for perf reasons, but it just doesn't work with the new history APIs, we'd better fix the APIs if there's a perf issue
* nsPlacesExpiration.js: tries to avoid expiring during a batch... off-hand we should rather move expiration to idle (bug 1376533 intended to do that but it failed to, will file a new bug). Not a big deal in general, was just an overzealous thing with main-thread IO.
* sync/modules/engines/bookmarks.js: Sync tries to avoid bumping score during batches. Similar to NewTabUtils this is unlikely to work with the new APIs, and probably require a different approach.

Considered all of the above, we should probably evaluate completely removing runInBatchMode and onBegin/EndUpdateBatch and make the view and result communicate batches directly between them.

If we'd ever need a general notification to tell consumers some large IO is happening, we could probably use an "heavy-io-task" nsIObserver topic.

I don't know if we'll do all of this here, potentially we could even just make the view invoke onBegin/EndUpdateBatch on the result and keep the rest for when async transactions ride the train, depending on the risk assessment of the above changes.

Kit, do you oversee any possible issues with dropping batches handling in Sync?

Comment 1

[Lina Butler [:lina]]

(In reply to Marco Bonardo [::mak] from comment #0)
> Kit, do you oversee any possible issues with dropping batches handling in
> Sync?

Probably not.

If there's a batch in progress, we defer syncing until it's complete. I think there's some value in doing this for changes like deleting a folder containing many bookmarks, or moving several bookmarks from one folder to another. Firing separate notifications definitely causes Sync to do more busy work: the first one starts a sync; subsequent ones will try and fail because we don't allow multiple in-flight syncs. Sync might also upload a temporarily incorrect tree if it ran before the changes committed, or a spurious follow-up sync if the changes committed and the observers fired after. But, since the async APIs don't batch, that's already the case today.

If we want to make this more efficient in the future, we could always add notifications for `onManyItemsMoved`, `onManyItemsAdded`, `onManyItemsRemoved`, etc., or even your `heavy-io-task` suggestion. We could also go further and add a generic `onSyncChanged` notification to indicate "bookmarks changed in some way, now is a good time to sync."

Comment 2

[Marco Bonardo [:mak]]

My idea in bug 1340498 would be to send an array of related notifications, so basically if a folder is removed, you get all the notifications related to that operation in an array, and can decide how to handle those.
It would still be at an API level, so 2 calls to remove() would generate 2 notification each with its own array, but at least if a remove causes many other notifications (children removal, annotations removals, tags removals) all of them would be presented as a single operation. Things like insertTree or eraseEverything would notify just once.
It's all stuff up in the air, but I think it's one of the next targets for optimizations.

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1392533 - Make the places tree view directly communicate batch notifications to the results.

Review commit: https://reviewboard.mozilla.org/r/177590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177590/

Comment 4

[Mark Banner (:standard8)]

I've attached my WIP so far. This gets the UI notifying the results of the batching. I've see quite big improvements in delete times with this.

Pasting isn't currently quite so good - the tagging service is kicking and attempts to do its own batching, which ends up turning all the batching off. Should be reasonable easy to solve.

Comment 5

[Mark Banner (:standard8)]

Note to self: Think about if we can do batching of importing bookmarks.

Comment 6

[Lina Butler [:lina]]

Drive-by note: this would be super handy for synced bookmarks, too, especially from large first syncs.

Comment 7

[Marco Bonardo [:mak]]

one thing at a time, we should first fix the most common cases here, so we can enabled APT. Then remove the old APIs so we can start from a fresher POV, at that point we can start rethinking the notifications system and send more useful notifications.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8905782 [details]
Bug 1392533 - Make the places tree view directly communicate batch notifications to the results.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177590/diff/1-2/

Comment 9

[Mark Banner (:standard8)]

When combined with bug 1403494, the patch here helps reduce the time for various actions:

For pasting 300 bookmarks into an empty folder, it reduces the time from about 2.6 to 2.3 seconds.
For deleting 300 bookmarks (select-all, right-click, delete), it reduces the time from about 11.5 to 2.5 seconds.
For moving 100 bookmarks up 10 places in a folder of 300, it reduces the time from about 2.8 to 2.1 seconds.

These are all measured on my Mac. Different set-ups may vary.

These times still aren't ideal, but they are heading in the right direction, and future work should improve things even more.

There's probably more activities we can do yet for batching, e.g. importing of bookmarks, maybe some other actions, but we'll separate those off to other bugs.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8905782 [details]
Bug 1392533 - Make the places tree view directly communicate batch notifications to the results.

https://reviewboard.mozilla.org/r/177590/#review189896

::: toolkit/components/places/PlacesTransactions.jsm:359
(Diff revision 2)
> +   * @param {Integer} itemsBeingChanged The count of items being changed. If the
> +   *                                    count is lower than a threshold, then
> +   *                                    batching won't be set.
> +   * @param {Function} functionToWrap The function to
> +   */
> +  async batchUpdatesForNode(resultNode, itemsBeingChanged, functionToWrap) {

I'd suggest to move this util to PlacesUIUtils. it's not strictly related to transactions, and usually utils acting on nodes are there.

The javadoc should have a @note about the fact this is pass-through if resultNode is not defined.

::: toolkit/components/places/PlacesTransactions.jsm:360
(Diff revision 2)
> +   *                                    count is lower than a threshold, then
> +   *                                    batching won't be set.
> +   * @param {Function} functionToWrap The function to
> +   */
> +  async batchUpdatesForNode(resultNode, itemsBeingChanged, functionToWrap) {
> +    if (resultNode) {

I think you should either have an explicit if (!resultNode) with an early return at the beginning, or move the check to the calling point and strictly require resultNode here.
Either of the two would be more readable imo.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8905782 [details]
Bug 1392533 - Make the places tree view directly communicate batch notifications to the results.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177590/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a6261ed5c41
Make the places tree view directly communicate batch notifications to the results. r=mak

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a6261ed5c41