Closed Bug 1411891 Opened 7 years ago Closed 7 years ago

Improve the performance of deleting bookmarks with async transactions

Categories

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

Product:
Firefox
Component:
Bookmarks & History
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1411891 - Improve the performance of deleting bookmarks with async transactions.
59 bytes, text/x-review-board-request
mak
: review+
Details 
I'm looking into improving the performance of deleting a set of bookmarks in one operation.

I think we can get various gains by batching the write to database operations.
Comment on attachment 8922243 [details]
Bug 1411891 - Improve the performance of deleting bookmarks with async transactions.

Marco, this is a very WIP patch that's still incomplete and not fully tested.

However, it does work for the simple case of deleting 300 bookmarks from a folder.

In my testing it is reducing the time taken from around 4-5 seconds to about 1 second.

It changes the PlacesUtils.bookmarks.remove() API slightly to accept arrays. So I'm definitely interested in feedback on the general approach and api changes.
Attachment #8922243 - Flags: feedback?(mak77)
Comment on attachment 8922243 [details]
Bug 1411891 - Improve the performance of deleting bookmarks with async transactions.

https://reviewboard.mozilla.org/r/193274/#review198574

::: browser/components/places/content/controller.js:975
(Diff revision 2)
>        await this._removeRange(range, transactions, removedFolders);
>      }
>  
>      if (transactions.length > 0) {
>        if (PlacesUIUtils.useAsyncTransactions) {
> -        await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactions.length, async () => {
> +        // XXX 5000 should be the number of bookmarks we're affecting?

5000 sounds by far too much?

::: toolkit/components/places/Bookmarks.jsm:788
(Diff revision 2)
> -      }
> +        }
> -
> +      }
> +      Cu.reportError(`notifying done ${performance.now()}`);
>        // Remove non-enumerable properties.
> -      return Object.assign({}, item);
> +      // XXX Return all objects removed.
> +      // XXX Do we even need to return anything from here?

Very valid questions. The reason to return the bookmarks, is pretty much the same about notifying, a consumer may only have the guid at hand, and may want to know the bookmark info just before it was removed, he can't fetch those info anymore after the removal.

What to return? fetch() returns only one entry, and provides an onResult handler. Though, that's because:
* it's common to use fetch() just to check if an url is bookmarked or get the last bookmark
* I think APIs should avoid returning multiple types, "object or null" is acceptable, "object, or array, or null" is not.

In this case, the most common use doesn't care about the return value at all. And since we throw if a bookmark is not found, there is no null result. I think both options are valid:
1. just resolve or reject to nothing and provide an onResult handler
2. reject or resolve to an array of the removed bookmarks

the former could be a bit more memory efficient, the latter could be a bit easier for the consumer.
The only things currently using the result are WebExtensions, NewTabUtils and tests afaict. They do single removals, so they won't see benefit from returning an array. Maybe we should go the callback path for now? What do you think?

::: toolkit/components/places/Bookmarks.jsm:1779
(Diff revision 2)
> +    let performance = Services.appShell.hiddenDOMWindow.performance;
> +    Cu.reportError(`starting transaction ${performance.now()}`);
> +
>      await db.executeTransaction(async function transaction() {
> +      for (let item of items) {
> +        // XXX can we simplify isUntagging and just set it for the whole function?

not worth the complications, tags should just be moved apart in the future.

::: toolkit/components/places/Bookmarks.jsm:1818
(Diff revision 2)
> +      let syncChangeDelta =
> +        PlacesSyncUtils.bookmarks.determineSyncChangeDelta(options.source);
> +
> +      let parentGuids = [];
> +
> +      for (let item of items) {

why do we need to loop twice? afaict syncChangeDelta can be determined earlier, as well as the parentGuids declaration.
Was it just to measure?

::: toolkit/components/places/Bookmarks.jsm:1822
(Diff revision 2)
> +
> +      for (let item of items) {
> +        let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId;
> +
> +        if (!parentGuids.includes(item.parentGuid)) {
> +          parentGuids.push(item.parentGuid);

Use a Set?

::: toolkit/components/places/PlacesTransactions.jsm:1493
(Diff revision 2)
> +    let removedItems = [];
> +
> +    for (let guid of guids) {
>        try {
> -        tree = await PlacesUtils.promiseBookmarksTree(guid);
> +        // Although we don't strictly need to get this information for the remove,
> +        // we do need it for the possiblity of undo().

this could be one of those cases where we could use the remove call return value! maybe it's THE best case to evaluate, it would save a read.

There is only one thing to take into account, that we may get in the same remove call a parent and a child, and we should not try to create a child before the parent on undo. but maybe this is already an existing problem with the current code...
Attachment #8922243 - Flags: feedback?(mak77) → feedback+
Comment on attachment 8922243 [details]
Bug 1411891 - Improve the performance of deleting bookmarks with async transactions.

https://reviewboard.mozilla.org/r/193274/#review198574

> 5000 sounds by far too much?

Oh, I meant I should replace that number with the bookmark count (rather than transaction count).

> why do we need to loop twice? afaict syncChangeDelta can be determined earlier, as well as the parentGuids declaration.
> Was it just to measure?

Unfortunately, we still need to handle addSyncChangesForBookmarksWithURL and adjustSeparatorsSyncCounter which I think both need to be run after the main delete has happened. Likewise, there some things that seem to need to happen before that (removing the folder contents).
Comment on attachment 8922243 [details]
Bug 1411891 - Improve the performance of deleting bookmarks with async transactions.

https://reviewboard.mozilla.org/r/193274/#review198574

> this could be one of those cases where we could use the remove call return value! maybe it's THE best case to evaluate, it would save a read.
> 
> There is only one thing to take into account, that we may get in the same remove call a parent and a child, and we should not try to create a child before the parent on undo. but maybe this is already an existing problem with the current code...

I discussed this with Marco on irc. The summary is that it would be hard to define a consistent return value for remove() which takes account of the various different options that are available during the remove (e.g. folders -> tree, multiple removes -> array etc).

Additionally we don't have any major uses for the return value. There's a potential for a slight optimisation here, but that would require more of a rewrite, or using fetchTree that we haven't got yet.

Therefore we've decided to drop the return value from PlacesUtils.bookmarks.remove() at the moment.
Comment on attachment 8922243 [details]
Bug 1411891 - Improve the performance of deleting bookmarks with async transactions.

https://reviewboard.mozilla.org/r/193274/#review199566

::: toolkit/components/places/Bookmarks.jsm:790
(Diff revision 3)
> +        }
> +
> -    // Even if we ignore any other unneeded property, we still validate any
> +        // Even if we ignore any other unneeded property, we still validate any
> -    // known property to reduce likelihood of hidden bugs.
> +        // known property to reduce likelihood of hidden bugs.
> -    let removeInfo = validateBookmarkObject("Bookmarks.jsm: remove", info);
> +        let removeInfo = validateBookmarkObject("Bookmarks.jsm: remove", info);
>  

I wonder if we should split the for loop here.
The reason is that this is changing synchronous exceptions into async rejections. The risk is that if consumers forgets to await or catch like in a simple
PlacesUtils.bookmarks.remove(some_broken_input);
then we'd have an obscure bug.
On the other side, splitting the for means we have to walk the array twice.
I suppose we're still far from a "missing-await-or-catch" eslint rule?
If the added cost is not extreme, for now I'd prefer to keep the previous synchronous exceptions.

::: toolkit/components/places/Bookmarks.jsm:2189
(Diff revision 3)
> -var removeAnnotationsForItem = async function(db, itemId) {
> +var removeAnnotationsForItems = async function(db, items) {
> +  // Remove the annotations.
>    await db.executeCached(
> -    `DELETE FROM moz_items_annos
> -     WHERE item_id = :id
> -    `, { id: itemId });
> +    `DELETE FROM moz_items_annos WHERE item_id IN (${
> +      new Array(items.length).fill("?").join(",")})`,
> +    items.map(item => item._id)

copy sqlList from History.jsm and use it, we don't need to bind when we completely control the data, in this case it's just ids. The advantage is that it's more performant.

::: toolkit/components/places/PlacesTransactions.jsm:1497
(Diff revision 3)
> -    let promiseBookmarksTree = async function(guid) {
> -      let tree;
> +    let removedItems = [];
> +
> +    for (let guid of guids) {
>        try {
> -        tree = await PlacesUtils.promiseBookmarksTree(guid);
> +        // Although we don't strictly need to get this information for the remove,
> +        // we do need it for the possiblity of undo().

typo: possiblity

::: toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js:249
(Diff revision 3)
> +  let bm1 = await PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
> +                                                 parentGuid: PlacesUtils.bookmarks.unfiledGuid,
> +                                                 url: new URL("http://remove.example.com/") });
> +  let bm2 = await PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
> +                                                 parentGuid: PlacesUtils.bookmarks.unfiledGuid,
> +                                                 url: new URL("http://remove1.example.com/") });

no need to new URL
Attachment #8922243 - Flags: review?(mak77) → review+
Comment on attachment 8922243 [details]
Bug 1411891 - Improve the performance of deleting bookmarks with async transactions.

https://reviewboard.mozilla.org/r/193274/#review199566

> I wonder if we should split the for loop here.
> The reason is that this is changing synchronous exceptions into async rejections. The risk is that if consumers forgets to await or catch like in a simple
> PlacesUtils.bookmarks.remove(some_broken_input);
> then we'd have an obscure bug.
> On the other side, splitting the for means we have to walk the array twice.
> I suppose we're still far from a "missing-await-or-catch" eslint rule?
> If the added cost is not extreme, for now I'd prefer to keep the previous synchronous exceptions.

I debated about which to go for re the loop or putting it in to the asnyc parts, but I'm quite happy to go with moving it to the sync side. The cost isn't that much (especially in comparison to the db access).

For the ESLint rule, we'd actually need something like flow or other code analysis - we'd have to know the function being called was an async / promise returning function.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e17a2bca5391
Improve the performance of deleting bookmarks with async transactions. r=mak
I'd missed there was a test outside the directories that was relying on the return value. I've changed the test and also updated the jsdoc for the function it was testing.
Flags: needinfo?(standard8)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e33f91893d88
Improve the performance of deleting bookmarks with async transactions. r=mak
https://hg.mozilla.org/mozilla-central/rev/e33f91893d88
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Blocks: 1405242
Regressions: 1540669
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: