Closed Bug 609398 Opened 9 years ago Closed 9 years ago

Get rid of partial sync

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+
fennec 2.0b3+ ---

People

(Reporter: philikon, Assigned: philikon)

References

Details

Attachments

(1 file)

On mobile we do partial syncs on large datasets (only fetch 50 items at a time each sync) mostly because of memory restrictions. Because we do partial syncs we have to remember which items still need to be synced, which touches disk (=bad).

Let's instead sync everything each time we sync (but on mobile we do it in batches of N items where N >= 50, but to be determined). That way mobile devices are in a fully synced state sooner and we never have to touch disk.

If a sync happens to fail, we don't update the last sync pref so the engine will just redo everything from the last sync. That's ok, conflict/merge resolution should do just fine here.
Duplicate of this bug: 607934
tracking-fennec: --- → ?
Attached patch v1Splinter Review
No longer spread batches over multiple syncs but do it within _processIncoming().

This does not provide backward compatibility for any lingering items in toFetch. I think we can get away with that because these would only occur a) on mobile and b) when you haven't synced enough times yet and c) if this patch makes it into a release that doesn't have the storage bump (the storage bump would mean everything gets synced again anyway).
Assignee: nobody → philipp
Attachment #488279 - Flags: review?(mconnor)
Comment on attachment 488279 [details] [diff] [review]
v1

>+/// 50 is hardcoded here because of URL length restrictions.
>+// (GUIDs can be up to 64 chars long)
>+MOBILE_BATCH_SIZE:                     50,

We should have a followup to be smarter about sizes of GUIDs here, using similar heuristics to what we use to get as many tabs as we can into a record.  We can easily go to 100-150 for history, I would think.

>diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js

>   // Generate outgoing records

Er, is this comment right?  I don't think so...
Attachment #488279 - Flags: review?(mconnor) → review+
(In reply to comment #3)
> Comment on attachment 488279 [details] [diff] [review]
> v1
> 
> >+/// 50 is hardcoded here because of URL length restrictions.
> >+// (GUIDs can be up to 64 chars long)
> >+MOBILE_BATCH_SIZE:                     50,
> 
> We should have a followup to be smarter about sizes of GUIDs here, using
> similar heuristics to what we use to get as many tabs as we can into a record. 
> We can easily go to 100-150 for history, I would think.

Probably, so long as we don't consume too much memory. After all that's why do the batches in the first place. Filed bug 610770.

> >   // Generate outgoing records
> 
> Er, is this comment right?  I don't think so...

lol, it's not.
Pushed with comment fixes:
http://hg.mozilla.org/services/fx-sync/rev/0622743f00e0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 612381
blocking2.0: --- → beta8+
tracking-fennec: ? → 2.0b3+
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.