Closed
Bug 1276090
Opened 8 years ago
Closed 8 years ago
Don't increment the bookmarks score during a batch operation
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: markh, Assigned: markh)
References
Details
(Whiteboard: [sync-data-integrity])
Attachments
(1 file, 1 obsolete file)
The bookmarks engine treats every bookmark change as "significant" - even ones that are happening as part of a batch operation. This means our tracker is almost guaranteed to miss changes - a batch operation is almost certainly making multiple changes but we will stop listening after the first. This probably becomes moot with Kit's patch, but I had this patch in my tree and figured I see what people thought about it in the meantime. Richard/Kit, can you see any reason this isn't the right thing to do for our current tracker?
Attachment #8757076 -
Flags: feedback?(rnewman)
Comment 1•8 years ago
|
||
Comment on attachment 8757076 [details] [diff] [review] 0001-Don-t-increment-the-sync-score-during-a-batch-bookma.patch Review of attachment 8757076 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/modules/engines/bookmarks.js @@ +1375,5 @@ > + // If we are in a batch update just record what we should increment the > + // score by; we avoid starting a Sync during a batch upload as this will > + // cause us to miss future notifications from the batch. > + if (this._inBatchUpdate) { > + this._batchScore += SCORE_INCREMENT_XLARGE; Every single bump will exceed the sync threshold. Might as well just do nothing and just bump score in `onEndUpdateBatch`, or flip a flag here, instead. @@ +1532,5 @@ > PlacesUtils.annotations.removeItemAnnotation(itemId, BookmarkAnnos.PARENT_ANNO); > }, > > + onBeginUpdateBatch: function () { > + this._inBatchUpdate = true; Can batches ever nest? Can we make this a `batchDepth` counter instead, and have the active edge be 1->0?
Attachment #8757076 -
Flags: feedback?(rnewman) → feedback+
Updated•8 years ago
|
Assignee: nobody → markh
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Don't increment the bookmarks "score" during a batch operation. → Don't increment the bookmarks score during a batch operation
Updated•8 years ago
|
Whiteboard: [sync-data-integrity]
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #1) > > + if (this._inBatchUpdate) { > > + this._batchScore += SCORE_INCREMENT_XLARGE; > > Every single bump will exceed the sync threshold. Might as well just do > nothing and just bump score in `onEndUpdateBatch`, or flip a flag here, > instead. Yeah, I did the former - unconditionally increment the score at the end of the batch - it doesn't seem worthwhile optimizing for the case of a batch that does nothing. > @@ +1532,5 @@ > > PlacesUtils.annotations.removeItemAnnotation(itemId, BookmarkAnnos.PARENT_ANNO); > > }, > > > > + onBeginUpdateBatch: function () { > > + this._inBatchUpdate = true; > > Can batches ever nest? The runInBatch implementation tracks the depth and only notifies at the end, so in practice, this does the right thing without taking nested batches into account... > Can we make this a `batchDepth` counter instead, and have the active edge be > 1->0? But yeah, I don't see a good reason to rely on this implementation detail, so I ended up using a batchDepth. Note a side-effect of this patch is that one existing test that uses PlacesSortFolderByNameTransaction now only increments the score by SCORE_INCREMENT_XLARGE instead of SCORE_INCREMENT_XLARGE*2 due to it using a batch under the covers, but this shouldn't cause any problems. Patch forthcoming...
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61492/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61492/
Attachment #8766684 -
Flags: review?(rnewman)
Assignee | ||
Updated•8 years ago
|
Attachment #8757076 -
Attachment is obsolete: true
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8766684 [details] Bug 1276090 - increment the sync score after a batch bookmark operation. This patch causes continuous syncing :( The engine does its own batching so every time we finish syncing we bump the score and start again.
Attachment #8766684 -
Flags: review?(rnewman)
Comment 5•8 years ago
|
||
Oh, that's exciting.
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8766684 [details] Bug 1276090 - increment the sync score after a batch bookmark operation. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61492/diff/1-2/
Attachment #8766684 -
Flags: review?(rnewman)
Comment 7•8 years ago
|
||
Comment on attachment 8766684 [details] Bug 1276090 - increment the sync score after a batch bookmark operation. https://reviewboard.mozilla.org/r/61492/#review58592 ::: services/sync/modules/engines/bookmarks.js:1537 (Diff revision 2) > > - onBeginUpdateBatch: function () {}, > - onEndUpdateBatch: function () {}, > + onBeginUpdateBatch: function () { > + ++this._batchDepth; > + }, > + onEndUpdateBatch: function () { > + if(--this._batchDepth == 0 && this._batchSawScoreIncrement) { Nit: space after `if`, use `===`. So your change is: don't increment as a result of a batch alone, because a batch will occur as an ordinary part of a sync. The score won't be incremented when the tracker is disabled during a sync, so we'll keep re-syncing until an uninterrupted sync occurs. Makes sense.
Attachment #8766684 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Thanks! (In reply to Richard Newman [:rnewman] from comment #7) > use `===`. While I made that change, FTR, https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style still says "In JavaScript, == is preferred to ==="
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8766684 [details] Bug 1276090 - increment the sync score after a batch bookmark operation. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61492/diff/2-3/
Comment 10•8 years ago
|
||
Pushed by mhammond@skippinet.com.au: https://hg.mozilla.org/integration/autoland/rev/511ae2ae8db6 increment the sync score after a batch bookmark operation. r=rnewman
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/511ae2ae8db6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in
before you can comment on or make changes to this bug.
Description
•