Closed Bug 1408092 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: lina, Assigned: eoger)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Priority: -- → P2
Blocks: 1433177
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: P2 → P1
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.
Attachment #8952283 - Flags: review?(kit)
Thanks Kit! I've made the changes and added tests :)
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.
Attachment #8952283 - Flags: review?(kit) → review+
Thanks for mentoring me Kit!
Pushed by eoger@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/bf6ce57855d5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.