609398 - Get rid of partial sync

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

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.

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: v1

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).

Comment 3

[Mike Connor [: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...

Comment 4

[Philipp von Weitershausen [:philikon]]

(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.

Comment 5

[Philipp von Weitershausen [:philikon]]

Pushed with comment fixes:
http://hg.mozilla.org/services/fx-sync/rev/0622743f00e0