Closed Bug 675397 Opened 10 years ago Closed 7 years ago

Reshuffle default syncing order

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: philikon, Assigned: mathias.demare, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 3 obsolete files)

Syncing history takes ages because it's usually the biggest lump. Let's make it sync last and other engines that have a stronger visual effect (prefs and bookmarks) sync first.

We could even have history not sync at all for the first sync, or just a smaller amount of the most (f)recent history, to make the first sync appear much quicker.
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug][mentor=gps][lang=js]
Gregory, want to give a link to the relevant file?
Hi, I would like to work on this bug. Can you help me get started thanks :)
Sync's code lives in the services-central repository at https://hg.mozilla.org/services/services-central/ under the services/sync directory.

Currently, the Sync code base is going through a lot of refactoring in preparation for supporting the next version of the Sync protocol. This means that this change may not ship for a few releases. So, if you want your contribution to ship soon, you may want to find another bug.

Anyway, the code for starting the syncing of engines lives in services/sync/modules/stages/enginesync.js. See around line 144 where it calls getEnabled() to obtain the set of engines to sync and then syncs them. This list is obtained from EngineManager, which lives in services/sync/modules/engines.js. (about line 370).

EngineManager stores engines as a map, which has no order. However, getEnabled() returns an array. We can either sort the array before or after return. I think I favor sorting after as part of engine syncing.

What I would do is add a "syncPriority" integer property to the SyncEngine type (also defined in engines.js). Then, use Array.sort in enginesync.js to sort engines at sync time.
Attached file Patch (obsolete) —
Attachment #664050 - Flags: review?(margaret.leibovic)
Attachment #664050 - Flags: review?(margaret.leibovic)
Comment on attachment 664050 [details]
Patch

Wrong patch. Sorry.
Attachment #664050 - Attachment is obsolete: true
Could you have a look at this and tell me what needs to be changed??
Thanks!
Attachment #678111 - Flags: feedback?(gps)
Comment on attachment 678111 [details] [diff] [review]
Proposed patch - added Priority variable and invoked sort method

Review of attachment 678111 [details] [diff] [review]:
-----------------------------------------------------------------

Richard: Can you take this for me? I'm busy for the next week or so, as you know.
Attachment #678111 - Flags: feedback?(gps) → feedback?(rnewman)
Yup, will attend to in the first half of this week.
Whiteboard: [good first bug][mentor=gps][lang=js] → [good first bug][mentor=rnewman][lang=js]
Comment on attachment 678111 [details] [diff] [review]
Proposed patch - added Priority variable and invoked sort method

Review of attachment 678111 [details] [diff] [review]:
-----------------------------------------------------------------

Please prepare your patch like this:

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

I'd also like to see a test here; add a brief test to test_enginemanager.js to verify sortedness.

Thanks!

::: services/sync/modules/engines.js
@@ +547,5 @@
>    _recordObj: CryptoWrapper,
>    version: 1,
>    
> +  //This gives the order in which the engines have to be synced
> +  SyncPriority: 0,

Nits:

* Space between comment marker // and test.
* Period at end of sentence.
* Use "camelCase" convention for member names.

And the comment should probably read:

  // A relative priority to use when computing an order
  // for engines to be synced. Higher-priority engines are
  // synced first.

::: services/sync/modules/engines/apps.js
@@ +132,5 @@
>    _storeObj: AppStore,
>    _trackerObj: AppTracker,
>    _recordObj: AppRec,
> +  applyIncomingBatchSize: APPS_STORE_BATCH_SIZE,
> +  SyncPriority:6

Space before value.

Current style is to include trailing commas to prevent exactly this kind of two-line change, so please add a trailing comma to the line you add.

::: services/sync/modules/engines/bookmarks.js
@@ +200,5 @@
>    _storeObj: BookmarksStore,
>    _trackerObj: BookmarksTracker,
>    version: 2,
> +  SyncPriority:3,
> +    

Space before value, and you introduced trailing whitespace. Please empty line 204.

::: services/sync/modules/engines/forms.js
@@ +160,4 @@
>    _trackerObj: FormTracker,
>    _recordObj: FormRec,
>    applyIncomingBatchSize: FORMS_STORE_BATCH_SIZE,
> +  SyncPriority:4,

Space before value.

@@ +160,5 @@
>    _trackerObj: FormTracker,
>    _recordObj: FormRec,
>    applyIncomingBatchSize: FORMS_STORE_BATCH_SIZE,
> +  SyncPriority:4,
> +    

Trailing whitespace.

::: services/sync/modules/engines/history.js
@@ +40,5 @@
>    _storeObj: HistoryStore,
>    _trackerObj: HistoryTracker,
>    downloadLimit: MAX_HISTORY_DOWNLOAD,
> +  applyIncomingBatchSize: HISTORY_STORE_BATCH_SIZE,
> +  SyncPriority:8

Trailing comma, space.

::: services/sync/modules/engines/passwords.js
@@ +35,5 @@
>    _trackerObj: PasswordTracker,
>    _recordObj: LoginRec,
>    applyIncomingBatchSize: PASSWORDS_STORE_BATCH_SIZE,
> +  SyncPriority:5,
> +    

Trailing comma, space, whitespace.

::: services/sync/modules/engines/prefs.js
@@ +40,5 @@
>    _trackerObj: PrefTracker,
>    _recordObj: PrefRec,
>    version: 2,
> +  SyncPriority:1,
> +    

Trailing comma, space, whitespace.

::: services/sync/modules/engines/tabs.js
@@ +49,5 @@
>    _storeObj: TabStore,
>    _trackerObj: TabTracker,
>    _recordObj: TabSetRecord,
> +  SyncPriority:2,
> +    

Trailing comma, space, whitespace.

::: services/sync/modules/stages/enginesync.js
@@ +141,4 @@
>        return;
>      }
>  
> +    try { 

Trailing whitespace.

@@ +141,5 @@
>        return;
>      }
>  
> +    try { 
> +      for each (let engine in this.service.engineManager.getEnabled().sort(function(a,b) {return a.SyncPriority-b.SyncPriority})) {

Nits:
* Include a single space on either side of an arithmetic operator.
* Always include the final semicolon in a statement.
* Always name functions.
* Include a space between function arguments.
* Prefer multi-line functions to one-liners.

But overall, why not just sort the return value inside EngineManager.getEnabled? That will affect all three call sites, and is nice and simple.
Attachment #678111 - Flags: feedback?(rnewman) → feedback+
Duplicate of this bug: 669898
Mentor: rnewman
Whiteboard: [good first bug][mentor=rnewman][lang=js] → [good first bug][lang=js]
I fixed the remarks mentioned above and added a unittest. Comments welcome :-)
Attachment #8497399 - Flags: review?(rnewman)
Comment on attachment 8497399 [details] [diff] [review]
Proposed patch (fixed comments and added unittest)

Review of attachment 8497399 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for cleaning this up! Comments inline.

::: services/sync/modules/engines.js
@@ +485,5 @@
>    getEnabled: function () {
> +    return this.getAll().filter(function(engine) engine.enabled)
> +        .sort(function(a, b) {
> +            return a.syncPriority - b.syncPriority;
> +        });

Let's change this in two ways:

* Use modern arrow syntax, 'cos the {}-less function syntax will eventually go away.
* Align on columns.

 return this.getAll()
            .filter((engine) => engine.enabled)
	    .sort((a, b) => a.syncPriority - b.syncPriority);

@@ +705,5 @@
>  
> +  // A relative priority to use when computing an order
> +  // for engines to be synced. Higher-priority engines are
> +  // synced first.
> +  syncPriority: 0,

It's worth noting that Array.prototype.sort isn't necessarily stable. (I think SpiderMonkey's is, but we can't rely on it.)

So comment here:

// It is recommended that a unique value be used for each engine,
// in order to guarantee a stable sequence.

::: services/sync/tests/unit/test_enginemanager.js
@@ +84,5 @@
> +
> +  do_check_eq(engines.length, 3);
> +  do_check_eq(engines[0], petrol);
> +  do_check_eq(engines[1], dummy);
> +  do_check_eq(engines[2], diesel);

Please lift do_check_array_eq out of test_keys.js into head_helpers.js, and use it here:

  do_check_array_eq(engines, [petrol, dummy, diesel]);

@@ +93,5 @@
> +
> +  engines = manager.getEnabled();
> +
> +  do_check_eq(engines.length, 3);
> +  do_check_eq(engines[0], petrol);

Same.
Attachment #8497399 - Flags: review?(rnewman) → feedback+
Comment on attachment 8497399 [details] [diff] [review]
Proposed patch (fixed comments and added unittest)

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

>+  // A relative priority to use when computing an order
>+  // for engines to be synced. Higher-priority engines are
>+  // synced first.
>+  syncPriority: 0,

It's worth noting in this comment which direction is "higher priority" (i.e. for priorities [0,1,2], does 0 sync first, or 2?).

The order implemented in this patch is:
prefs,tabs,bookmarks,forms,passwords,clients,addons,history

How does that differ from the current order? Is the current order deterministic? Was there any thinking behind this order beyond what's in comment 0? I might want to quibble with this ordering a little, just wondering where it comes from.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #14)

> It's worth noting in this comment which direction is "higher priority" (i.e.
> for priorities [0,1,2], does 0 sync first, or 2?).

Agreed.


> The order implemented in this patch is:
> prefs,tabs,bookmarks,forms,passwords,clients,addons,history
> 
> How does that differ from the current order?

The current order is deterministic, but only by accident. It's determined by a pref, with values dumped into an object, so the order is only maintained because JS objects maintain the order of their keys (at least at the sizes we hit).

> Services.prefs.getCharPref("services.sync.registerEngines")
"Bookmarks,Form,History,Password,Prefs,Tab,Addons"

> Weave.Service.engineManager._engines
Object { bookmarks: Object, forms: Object, history: Object, passwords: Object, prefs: Object, tabs: Object, addons: Object, adblockplus: Object }

> Weave.Service.engineManager.getEnabled().map((e) => e.name)
Array [ "bookmarks", "forms", "history", "passwords", "prefs", "tabs" ]


If I had to pick the order -- and unless you disagree, this is the order I'll request in my final review! -- it'd be:

1. Prefs. High-impact, small size, rarely fails.
2. Passwords. Immediate value, typically small, rarely fails.
3. Tabs. User-visible, typically small, never fails, but not immediately visible -- the user has to visit about:sync-tabs. But we want to *upload* sooner rather than later, and we don't want to stall that by syncing bookmarks first.
4. Bookmarks. Often immediately visible to the user, bookmarks are how most users seem to decide whether Sync is working or not.
5. Add-ons. Most users don't have many, and the data is small, but side-effects of install can take a while, and this isn't critical data.
6. Form history.
7. History.

Clients is special, and doesn't need a priority -- it's not even a registered engine.
I modified the patch according to the comments given. For now, I've taken the order as suggested by Richard.
Attachment #8497399 - Attachment is obsolete: true
Attachment #8497982 - Flags: review?(rnewman)
Comment on attachment 8497982 [details] [diff] [review]
Proposed patch: incorporated additional review comments (+ order change)

Review of attachment 8497982 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Please make sure the xpcshell tests pass.

Setting f? on Gavin to see if he has a different order in mind. If that clears, we're good to land.
Attachment #8497982 - Flags: review?(rnewman)
Attachment #8497982 - Flags: review+
Attachment #8497982 - Flags: feedback?(gavin.sharp)
Assignee: nobody → mathias.demare
Status: NEW → ASSIGNED
The xpcshell tests give the following result on my machine:
With patch:
103:44.16 INFO | Result summary:
103:44.16 INFO | Passed: 1884
103:44.16 INFO | Failed: 15
103:44.16 INFO | Todo: 5
103:44.16 INFO | Retried: 27

Without patch:
194:43.17 INFO | Result summary:
194:43.17 INFO | Passed: 1884
194:43.17 INFO | Failed: 15
194:43.18 INFO | Todo: 5
194:43.18 INFO | Retried: 64

---

I have the impression the failed tests are because of services running on my machine (ports being in use). I'm afraid I cannot try the xpcshell tests on the try server, as I don't have permission for that.
Mathias: feel free to request L1 access; see here:

<https://www.mozilla.org/hacking/committer/>

CC me on the bug and I'll vouch for you.

Docs for Try are here:

<https://wiki.mozilla.org/ReleaseEngineering/TryServer>

I'll push this one for you in the mean time.
Try build looks pretty clean to me.

Gavin, I'm going to go ahead and flag this checkin-needed; if you have strong opinions about the order please either clear the flag, or let us know so we can land a follow-up.
Keywords: checkin-needed
Attachment #678111 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/4339077e7826
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Comment on attachment 8497982 [details] [diff] [review]
Proposed patch: incorporated additional review comments (+ order change)

Sounds fine.
Attachment #8497982 - Flags: feedback?(gavin.sharp) → feedback+
https://hg.mozilla.org/mozilla-central/rev/4339077e7826
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → mozilla35
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.