Closed
Bug 1373897
Opened 7 years ago
Closed 7 years ago
Miscounting "successfully uploaded GUIDs" by a huge margin
Categories
(Firefox for Android Graveyard :: Android Sync, enhancement, P2)
Firefox for Android Graveyard
Android Sync
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: Grisha, Assigned: eoger, Mentored)
Details
Attachments
(1 file)
Seeing this in the logcat, after sync was interrupted by OS for not making progress: 06-17 00:57:13.769 5065 5603 I FxAccounts: fennec_grisha :: ServerSyncStage :: Stage history received 3100; stored 3100, reconciling 3100 and failed to store 0. Sent 36263; server accepted 6606563 and rejected 0. Duration: 419.52 seconds. Note the "server accepted 6606563", which is crazy. Our internal counting must be really wrong. Here's where this counter is actually incremented: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/synchronizer/RecordsChannel.java#264 Here's where we iterate through a set of "successfully uploaded GUIDs": https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadDispatcher.java#89 And here's where we pass in the "succeeded GUIDs" list to the above iterator: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java#175 Note that this happens every time a POST succeeds. Also note that it's a list of guids on the shared "whiteboard" object. And finally note that we add guids to this list here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java#145 ... but we only clear it after committing a batch, whenever we create a new whiteboard: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java#181 And so we end up over counting by huge amounts, and doing a TON of unnecessary work on large syncs. And the more records we need to upload, the more wasted effort there will be. The main impact here seems to be on performance and skewing the telemetry metrics.
Reporter | ||
Comment 1•7 years ago
|
||
Well, this doesn't happen in a general case - that is, I wasn't able to reproduce this again, which is somewhat worrying.
Updated•7 years ago
|
Priority: -- → P2
Reporter | ||
Comment 2•7 years ago
|
||
Ugh. Saw this again while working on Bug 1364644. 07-24 22:07:02.946 29259 29558 I FxAccounts: fennec_grisha :: ServerSyncStage :: Stage bookmarks received 577; stored 546, reconciling 546 and failed to store 0. Sent 496; server accepted 1496 and rejected 0. Duration: 3.71 seconds.
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to :Grisha Kruglov from comment #1) > Well, this doesn't happen in a general case - that is, I wasn't able to > reproduce this again, which is somewhat worrying. Quick test with counter replaced by a set of stored guids of which we then get a size shows that Comment 1 is correct in its assessment. So this is stupid and wasteful, but generally harmless.
Updated•7 years ago
|
Assignee: nobody → eoger
Reporter | ||
Comment 4•7 years ago
|
||
The problem is that at https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java#175 we're calling our success delegate method with a list of all guids that succeeded in a batch, across multiple requests. With every 201/200 post, this list keeps growing, and we end up calling onRecordStoreSucceeded multiple times for many of the records. Instead, we should just call payloadSucceeded with a list of guids that succeeded in the current POST.
Mentor: gkruglov
Comment hidden (mozreview-request) |
Reporter | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8902904 [details] Bug 1373897 - Clear the succeeded records GUIDs list after each POST in non-batched mode. https://reviewboard.mozilla.org/r/174624/#review180186 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java:180 (Diff revision 1) > dispatcher.batchWhiteboard.getSuccessRecordGuids(), > isCommit, > isLastPayload > ); > > + if (!dispatcher.batchWhiteboard.getInBatchingMode()) { I think it's better to move this logic to PayloadDispatcher's payloadSucceeded, and not pass in 'guids' into it at all - it already has access to the whiteboard, and it can clear the list after the 'onRecordStoreSucceeded' loop.
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8902904 [details] Bug 1373897 - Clear the succeeded records GUIDs list after each POST in non-batched mode. https://reviewboard.mozilla.org/r/174624/#review180190
Attachment #8902904 -
Flags: review?(gkruglov)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Thank you Grisha, launching a try build.
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8902904 [details] Bug 1373897 - Clear the succeeded records GUIDs list after each POST in non-batched mode. https://reviewboard.mozilla.org/r/174624/#review181458 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadDispatcher.java:100 (Diff revision 2) > bumpTimestampTo(uploadTimestamp, response.normalizedTimestampForHeader(SyncResponse.X_LAST_MODIFIED)); > uploader.setLastStoreTimestamp(uploadTimestamp); > } > > + if (!batchWhiteboard.getInBatchingMode()) { > + batchWhiteboard.clearSuccessRecordGuids(); I _think_ you also want to clear guids for `isCommit` payloads as well. Otherwise, next commit payload (in case upload is split up into multiple batches) will suffer from the same problem. ::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java:73 (Diff revision 2) > } > if (isLastPayload) { > ++lastPayloadsSucceeded; > } > + > + if (!batchWhiteboard.getInBatchingMode()) { It doesn't seem right to have to have to do this in tests. It would be nice if this test actually tested that we're clearing guids at the right times. Can you just call super.payloadSucceeded instead?
Attachment #8902904 -
Flags: review?(gkruglov)
Assignee | ||
Comment 11•7 years ago
|
||
> I _think_ you also want to clear guids for `isCommit` payloads as well. Otherwise, next commit payload (in case upload is split up into multiple batches) will suffer from the same problem.
I think it doesn't matter because |dispatcher.prepareForNextBatch();| (executed if isCommit) replaces the whole batchWhiteboard, but it looks cleaner anyway so I have made the change.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8902904 [details] Bug 1373897 - Clear the succeeded records GUIDs list after each POST in non-batched mode. https://reviewboard.mozilla.org/r/174624/#review181482 This looks good, thanks! FYI, your try push didn't actually run our junit4 tests :-(
Attachment #8902904 -
Flags: review?(gkruglov) → review+
Reporter | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 14•7 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8d4b1890f7cf Clear the succeeded records GUIDs list after each POST in non-batched mode. r=Grisha
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8d4b1890f7cf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•7 years ago
|
Product: Android Background Services → Firefox for Android
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•