1451352 - Improve performance of copying bookmarks (via copy/paste or drag/drop)

Status: RESOLVED WONTFIX

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

Description

[Mark Banner (:standard8)]

Splitting this out from bug 1404909 as recent changes makes it easier for us to deal with copy & move separately.

We should be able to improve the performance of copying multiple bookmarks.

Currently, we create a transaction for each individual bookmark in the copy, and then execute those transactions.

If we create a new PlacesTransaction, e.g. InsertTree, then we can create an object tree of the items to insert, and insert them all in one go.

This allows us to use database insert optimisations of insertTree, rather than individual insert operations.

Once we can redo notifications, we'll get further gains there.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1451352 - Rework copying bookmarks (via paste or dnd) to use insertTree to improve performance.

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

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8964935 [details]
Bug 1451352 - Rework copying bookmarks (via paste or dnd) to use insertTree to improve performance.

This largely works for copies from urlbar / tabs / external instances.

I'd like some feedback on the general architecture before I go further.

To finish it, I need a version of promiseBookmarksTree that returns items that are in a suitable format to push to insertTree, or do some kind of translation.

I'm thinking that the optimal way would be to implement PU.bookmarks.fetchTree().

Comments welcome.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8964935 [details]
Bug 1451352 - Rework copying bookmarks (via paste or dnd) to use insertTree to improve performance.

I'd have nothing against introducing an insertTree transaction....
But I have a couple concerns:
1. I wonder if it may be less efficient in the single/few insert case, that may be the most common one. Then we'd need to complicate the code to take different paths.
2. How common is copying multiple bookmarks? I suspect it's far more common to "move" them, and the copy today happens mostly because we don't allow moving from bookmark queries, that is something we want to change, to reduce the likelihood for the user to create dupes.

And based on the above questions, is it worth to add this complication for a possibly uncommon operation?

Comment 4

[Mark Banner (:standard8)]

I think you're probably right, that this isn't worth the complication at this stage. I'm still a little concerned about performance there, as we won't be able to get batch updates for the UI easily, but lets park this for now, and see what happens once we've finished the other changes we're working on.