1408092 - Don't merge or start a transaction when applying the buffer if nothing changed

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Lina Butler (ex-Mozilla)]

Currently, `BookmarkBuffer#apply` always builds and merges local and remote trees, even if nothing changed in the buffer! We expect no-op syncs to be fairly common, so there's a lot of busy work we can avoid by skipping the transaction if we don't need to merge anything.

We should also record telemetry events for these no-op syncs. Recording the number of records changed (i.e., `SELECT COUNT(*) FROM items WHERE needsMerge`) could be interesting as well.

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1408092 - Do not run the bookmarks merger if there ain't any remote and local changes.

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

Comment 2

[Lina Butler (ex-Mozilla)]

Comment on attachment 8952283 [details]
Bug 1408092 - Do not run the bookmarks merger if there ain't any remote and local changes.

https://reviewboard.mozilla.org/r/221534/#review228248

Looking good, Ed! Just a few nits, and would you also mind adding a new test file that exercises `hasChanges` for all four cases?

::: toolkit/components/places/SyncedBookmarksMirror.jsm:813
(Diff revision 1)
>  
>      return infos;
>    }
>  
> +  /*
> +   * Checks if local or buffer have any unsynced/unmerged changes.

Nit: local -> Places, buffer -> mirror, here and in the log message.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:820
(Diff revision 1)
> +   * @return {Boolean}
> +   *         `true` if something has changed.
> +   */
> +  async hasChanges() {
> +    // In the first subquery, we check incoming items with needsMerge = true
> +    // except the tombstones who don't correspond to any local bookmark.

Let's clarify in this comment that we do this because we don't currently store tombstones, and reference bug 1343103.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:822
(Diff revision 1)
> +   */
> +  async hasChanges() {
> +    // In the first subquery, we check incoming items with needsMerge = true
> +    // except the tombstones who don't correspond to any local bookmark.
> +    let rows = await this.db.execute(`
> +      SELECT 1

This is a style pref more than anything, but I'd write the query like this: `SELECT EXISTS(SELECT 1 FROM items ...) OR EXISTS(SELECT 1 FROM moz_bookmarks ...) OR EXISTS(SELECT 1 FROM moz_bookmarks_deleted) AS hasChanges`. That'll let you remove the `LIMIT 1` from the subqueries.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:832
(Diff revision 1)
> +             (NOT v.isDeleted OR b.guid NOT NULL)
> +             LIMIT 1
> +            ) OR (
> +             SELECT 1
> +             FROM moz_bookmarks
> +             WHERE guid NOT IN (:rootGuid, :tagsGuid) AND

Tags are stored as bookmarks and folders under the tags root, so they'll also get change counter bumps. This will also run a merge if there are any changes to the left pane queries, which isn't harmful, just busy work. :-) You can probably crib the `WITH RECURSIVE syncedItems(id)` definition, and use it here.

Comment 3

[Edouard Oger [:eoger]]

Comment on attachment 8952283 [details]
Bug 1408092 - Do not run the bookmarks merger if there ain't any remote and local changes.

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

Comment 4

[Edouard Oger [:eoger]]

Thanks Kit! I've made the changes and added tests :)

Comment 5

[Lina Butler (ex-Mozilla)]

Comment on attachment 8952283 [details]
Bug 1408092 - Do not run the bookmarks merger if there ain't any remote and local changes.

https://reviewboard.mozilla.org/r/221534/#review228512

Awesome, thanks, Ed! \o/

::: toolkit/components/places/SyncedBookmarksMirror.jsm:315
(Diff revision 2)
>     */
>    async apply({ localTimeSeconds = Date.now() / 1000,
>                  remoteTimeSeconds = 0 } = {}) {
> +    let hasChanges = await this.hasChanges();
> +    if (!hasChanges) {
> +      MirrorLog.debug("No changes detected in both buffer and local");

Nit: buffer -> mirror, local -> Places here, too, please. :-)

::: toolkit/components/places/SyncedBookmarksMirror.jsm:842
(Diff revision 2)
> +         SELECT b.id FROM moz_bookmarks b
> +         JOIN syncedItems s ON b.parent = s.id
> +       )
> +       SELECT 1
> +       FROM moz_bookmarks b
> +       JOIN syncedItems s ON s.id = b.id

You can simplify this to just `SELECT 1 FROM syncedItems WHERE syncChangeCounter > 0`. The join isn't needed if you change the definition to `syncedItems(id, syncChangeCounter)` and select `b.syncChangeCounter`. I think you can also skip checking for `:rootGuid`, since `syncedItems` won't include it.

Comment 6

[Edouard Oger [:eoger]]

Comment on attachment 8952283 [details]
Bug 1408092 - Do not run the bookmarks merger if there ain't any remote and local changes.

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

Comment 7

[Edouard Oger [:eoger]]

Thanks for mentoring me Kit!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf6ce57855d5
Do not run the bookmarks merger if there ain't any remote and local changes. r=kitcambridge

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/bf6ce57855d5