1372496 - PlacesUtils.bookmarks.eraseEverything is very slow

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Mark Banner (:standard8)]

In working on bug 1095426, I have discovered that the async eraseEverything appears to be quite slow.

When erasing ~24k bookmarks, it takes approximately 4 to 5 minutes on my Macbook Pro (after extending the sqlite timeout).

I'm currently investigating the causes of this, though it mainly seems to be related to the updateFrecency calculations.

Comment 1

[Mark Banner (:standard8)]

I've cut down the bookmarks size to about 6.6k, my analysis shows that the erasing takes about 7s, but the updateFrecency takes about 6.5s of that.

From the code it looks like the updateFrecency is intended to run in the background, there's no await for it. However, the withConnectionWrapper/db.executeTransaction wrapper in eraseEverything seems to be ensuring that the frecency update completes.

I'm not sure if that is intended or not.

Attempting to batch the update frecency algorithm actually slows it down, so that isn't an option ;-)

Comment 2

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #1)
> From the code it looks like the updateFrecency is intended to run in the
> background, there's no await for it. However, the
> withConnectionWrapper/db.executeTransaction wrapper in eraseEverything seems
> to be ensuring that the frecency update completes.

Regardless, all the queries are serialized on a thread, it's possible the updateFrecency call still happens before our COMMIT and so we end up waiting for it. We could try delaying it, maybe we could even do it in the transaction .then handler? Unfortunately requestIdleCallback is not yet usable from non DOM contexts.
 
> I'm not sure if that is intended or not.

No, the frecency updates should be outside the transaction :(

Another thing you should try, is to wrap the frecency updates in a transaction but batch them. In a previous patch I noticed that doing thousands of updates in a single query was 0(n^2), when I chunked the changes into smaller pieces it took one tenth of the time.

To sum up:
1. we can enqueue the updateFrecency calls better so they won't run inside the main transaction (API will return faster)
2. we should make updateFrecency chunk (we could try with 1k at a time) and try with and without a transaction.

Comment 3

[Marco Bonardo [:mak]]

Also, for reference and working with real-world numbers, telemetry tells us the 95th percentile of users have less than 1.4k bookmarks. Testing with double that number should be safe enough for the foreseeable future.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

P1 as it is blocking another P1 with patches.

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1372496 - Speed up bookmarks.eraseEverything and bookmarks.remove APIs by handling updating of frencencies outside of the main transaction.

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

Comment 6

[Mark Banner (:standard8)]

(In reply to Marco Bonardo [::mak] from comment #2)
> 2. we should make updateFrecency chunk (we could try with 1k at a time) and
> try with and without a transaction.

I've just been trying this and comparing it. With 2.8k bookmarks on my machine with just step 1, we're seeing the updateFrecency take ~ 1035ms. With various sized chunk and with/without transactions I'm seeing a range of about 1020 - 1080. So I don't think there's any real benefit there.

Comment 7

[Mark Banner (:standard8)]

Attachment: Example test for speed of removing 2.8k bookmarks.

This is what I've been using to test the speed of the changes. It is a simple xpcshell-test, that inserts and then removes 2.8k bookmarks.

Without the patch I've put up for review, this would show that eraseEverything would take about 1152 milliseconds.

With the patch for review, eraseEverything takes 274 milliseconds. updateFrecencies happening in the background and takes about 1034 milliseconds.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8877963 [details]
Bug 1372496 - Speed up bookmarks.eraseEverything and bookmarks.remove APIs by handling updating of frencencies outside of the main transaction.

https://reviewboard.mozilla.org/r/149362/#review154026

::: toolkit/components/places/Bookmarks.jsm:1609
(Diff revision 1)
>      if (item.type == Bookmarks.TYPE_BOOKMARK && !isUntagging) {
>        // ...though we don't wait for the calculation.
>        updateFrecency(db, [item.url]).catch(Cu.reportError);
>      }
>  
> +    if (urls && urls.length) {

just for added safety, let's also check item.type == Bookmarks.TYPE_FOLDER

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8877963 [details]
Bug 1372496 - Speed up bookmarks.eraseEverything and bookmarks.remove APIs by handling updating of frencencies outside of the main transaction.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a4350c371bb
Speed up bookmarks.eraseEverything and bookmarks.remove APIs by handling updating of frencencies outside of the main transaction. r=mak

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/8a4350c371bb