Closed
Bug 1408092
Opened 7 years ago
Closed 6 years ago
Don't merge or start a transaction when applying the buffer if nothing changed
Categories
(Firefox :: Sync, enhancement, P1)
Firefox
Sync
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.
Updated•7 years ago
|
Priority: -- → P2
Updated•6 years ago
|
Assignee: nobody → eoger
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request) |
Reporter | ||
Comment 2•6 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
Thanks Kit! I've made the changes and added tests :)
Reporter | ||
Comment 5•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
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
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf6ce57855d5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in
before you can comment on or make changes to this bug.
Description
•