Closed Bug 600429 Opened 9 years ago Closed 9 years ago

Tune score increments to trigger a sync more quickly

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla7
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: mconnor, Assigned: emtwo)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [verifed in services])

Attachments

(2 files, 13 obsolete files)

11.90 KB, patch
Details | Diff | Splinter Review
22.14 KB, patch
Details | Diff | Splinter Review
basically, we should tweak the thresholds to sync more aggressively when a user is actively browsing, and then reduce the background updates significantly when a device is idle.  When a user comes back from idle, we should sync at that time (plausibly on a very short idle time, perhaps 1 second).  This should dramatically lessen the need for manually syncing.
blocking2.0: --- → betaN+
Target Milestone: --- → 1.6
Note: this may include forcing a sync on shutdown/sleep in a case where the sync hasn't fired yet, but the goal is to make this an exceptional case.
We'll experiment with this, and try to get something viable into final, but we will not consider this a hard blocker.
blocking2.0: betaN+ → -
status2.0: --- → wanted
Let's call this... Instant Sync! :D
OS: Windows Vista → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: 1.6 → ---
Assignee: nobody → msamuel
Attachment #537061 - Flags: feedback?(philipp)
Comment on attachment 537061 [details] [diff] [review]
instantantly syncing bookmarks by adjusting their score value

I'm a bit confused by this patch. If all you want to do is adjust bookmarks's score value to trigger a "more instant" sync, wouldn't it be enough to do just this change:

>   /* Every add/remove/change is worth 10 points */
>   _upScore: function BMT__upScore() {
>-    this.score += 10;
>+    this.score += this._score_increment;
>   },

?

Specifically, I'm confused about what the engine.instantSync and engine.score_increment attributes are for. (Btw, you don't need to define getters and setters for them since they just seem to be dumb attributes.)
Attachment #537061 - Flags: feedback?(philipp)
(In reply to comment #5)
> Comment on attachment 537061 [details] [diff] [review] [review]
> instantantly syncing bookmarks by adjusting their score value
> 
> I'm a bit confused by this patch. If all you want to do is adjust
> bookmarks's score value to trigger a "more instant" sync, wouldn't it be
> enough to do just this change:
> 
> >   /* Every add/remove/change is worth 10 points */
> >   _upScore: function BMT__upScore() {
> >-    this.score += 10;
> >+    this.score += this._score_increment;
> >   },
> 
> ?
> 
> Specifically, I'm confused about what the engine.instantSync and
> engine.score_increment attributes are for. (Btw, you don't need to define
> getters and setters for them since they just seem to be dumb attributes.)

The goal is to adjust the bookmark's score value so that after a bookmark is added, the score is always above the threshold so that it will immediately trigger a sync.  And the threshold changes depending on whether the clients are mobile, multi-desktop, etc.

engine.score_increment is the score value that an engine increments by. Before this was 10 for bookmarks & is hardcoded but since we want a bookmark to trigger a sync immediately then its score_increment needs to be adjusted based on the changing threshold.  engine.instantSync is a boolean which registers whether an engine wants to instant sync.  If it does, then its score_increment will be updated based on the threshold. This is good for easily setting whether an engine wants to instant sync.
Another idea to avoid some extra code if it's better - we could make the bookmark score larger than the largest threshold so we don't need to worry about changing its score when the threshold changes. But I have a feeling that having an arbitrarily large score could be an issue.
(In reply to comment #6)
> The goal is to adjust the bookmark's score value so that after a bookmark is
> added, the score is always above the threshold so that it will immediately
> trigger a sync.  And the threshold changes depending on whether the clients
> are mobile, multi-desktop, etc.

Oh right, I forgot about that! So if you only have a single device connected, we don't need to be syncing as instantly, right? Because there isn't another device that would benefit from the uploaded data. And for multiple devices, I'm not so sure we should distinguish desktop and mobile any longer. In short, let's just have one multi-device threshold to which we can tune the score increments. They won't work as well for the single-device threshold but that's ok.

> engine.score_increment is the score value that an engine increments by.
> Before this was 10 for bookmarks & is hardcoded but since we want a bookmark
> to trigger a sync immediately then its score_increment needs to be adjusted
> based on the changing threshold.  engine.instantSync is a boolean which
> registers whether an engine wants to instant sync.  If it does, then its
> score_increment will be updated based on the threshold. This is good for
> easily setting whether an engine wants to instant sync.

That makes sense now, thanks! I was missing the variable score bit. But as said, instead of adding this complexity it feels like we can simplify it a great deal by having a simpler threshold scheme.
Attachment #537061 - Attachment is obsolete: true
Comment on attachment 537219 [details] [diff] [review]
WIP - instantly syncing bookmarks & passwords for multiple clients

>+// Instant sync score increment
>+const SCORE_INCREMENT = 300 / 2 + 1; // MULTI_DEVICE_THRESHOLD / 2 + 1
>+

Any reason why you're not just simply using the MULTI_DEVICE_THRESHOLD constant instead of 300 here?

Also, it took me a while to figure out why you chose only half the threshold for the score increment: because when you add a bookmark, we also add the parent to the tracker. But, when you just change the title or tags of a bookmark, only one item -- the bookmark itself -- gets tracked (and when you move a bookmark between folders, three items get tracked!). There isn't really any reason why we can't make the increment simply MULTI_DEVICE_THRESHOLD + 1. If we overshoot the threshold by twice its value, no harm done, right?

>-    // A single add, remove or change is 15 points, all items removed is 50
>+    // A single add, remove or change will trigger a sync for MULTI-DEVICE.
>+    // All items removed is 500 points, also triggering a sync for MULTI-DEVICE.
>     switch (aData) {
>     case 'modifyLogin':
>       aSubject = aSubject.QueryInterface(Ci.nsIArray).
>         queryElementAt(1, Ci.nsILoginMetaInfo);

Woah, a case statement with fallthrough. Please add a

  // fallthrough

comment here.

>     case 'addLogin':
>     case 'removeLogin':
>       // Skip over Weave password/passphrase changes
>       aSubject.QueryInterface(Ci.nsILoginMetaInfo).
>         QueryInterface(Ci.nsILoginInfo);
>       if (aSubject.hostname == PWDMGR_HOST)
>         break;
> 
>-      this.score += 15;
>+      this.score += SCORE_INCREMENT;
>       this._log.trace(aData + ": " + aSubject.guid);
>       this.addChangedID(aSubject.guid);
>       break;
>     case 'removeAllLogins':
>       this._log.trace(aData);
>       this.score += 500;
>       break;
>     }

What about the last case? Could just use SCORE_INCREMENT here as well, no? No more magic numbers!

Also, the prefs engine seems like a good candidate for this treatment.


>diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js
>--- a/services/sync/modules/service.js
>+++ b/services/sync/modules/service.js
>@@ -1869,17 +1869,17 @@ WeaveSvc.prototype = {
>     this.numClients = numClients;
> 
>     if (numClients == 1) {
>       this.syncInterval = SINGLE_USER_SYNC;
>       this.syncThreshold = SINGLE_USER_THRESHOLD;
>     }
>     else {
>       this.syncInterval = hasMobile ? MULTI_MOBILE_SYNC : MULTI_DESKTOP_SYNC;
>-      this.syncThreshold = hasMobile ? MULTI_MOBILE_THRESHOLD : MULTI_DESKTOP_THRESHOLD;
>+      this.syncThreshold = MULTI_DEVICE_THRESHOLD;
>     }
>   },

Can now get rid of the 'hasMobile' variable, too, right?


>diff --git a/services/sync/tests/unit/test_instant_sync.js b/services/sync/tests/unit/test_instant_sync.js

Yay for writing tests! I'm not sure these tests separate concerns appropriately, though. I think we should definitely test that the various trackers increment the score correctly. Feel free to amend test_bookmark_tracker.js and create test_password_tracker.js (welcome to my world 1 year ago when I was touching code that didn't have tests yet, so the first thing I got do was write tests...)

We should also test that a sufficiently large score update triggers a sync. This behaviour is there right now, but as you very well know, it's kind of idiotic with the whole idle thing. This also makes it hard to test. I see you've tried to tame it with

>+  // Service._calculateScore gets called after a 3 second delay.
>+  // A 4 second delay is used for buffer time here.
>+  Utils.delay(waitForSync, 4000);

but that's not ideal. Time delays like that are the perfect way to create intermittently failing tests or, as they're called in Mozilla lingo, random oranges. Since you'll be revamping that whole sync-trigger-on-score-update code anyway, you probably want to write the tests then. And those tests should also preferably use a stub engine implementation so they're independent of specific engine quirks. Unit tests means we're testing the smallest possible units... independently!
> >diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js
> >--- a/services/sync/modules/service.js
> >+++ b/services/sync/modules/service.js
> >@@ -1869,17 +1869,17 @@ WeaveSvc.prototype = {
> >     this.numClients = numClients;
> > 
> >     if (numClients == 1) {
> >       this.syncInterval = SINGLE_USER_SYNC;
> >       this.syncThreshold = SINGLE_USER_THRESHOLD;
> >     }
> >     else {
> >       this.syncInterval = hasMobile ? MULTI_MOBILE_SYNC : MULTI_DESKTOP_SYNC;
> >-      this.syncThreshold = hasMobile ? MULTI_MOBILE_THRESHOLD : MULTI_DESKTOP_THRESHOLD;
> >+      this.syncThreshold = MULTI_DEVICE_THRESHOLD;
> >     }
> >   },
> 
> Can now get rid of the 'hasMobile' variable, too, right?

I haven't made changes to sync intervals yet.  That's why I kept 'hasMobile' there.  Do we also want to combine MULTI_MOBILE_SYNC & MULTI_DESKTOP_SYNC into MULTI_DEVICE_SYNC? Maybe that would be more appropriate in another patch dealing with sync intervals?
(In reply to comment #11)
> > Can now get rid of the 'hasMobile' variable, too, right?
> 
> I haven't made changes to sync intervals yet.  That's why I kept 'hasMobile'
> there.  Do we also want to combine MULTI_MOBILE_SYNC & MULTI_DESKTOP_SYNC
> into MULTI_DEVICE_SYNC? Maybe that would be more appropriate in another
> patch dealing with sync intervals?

Err yeah, you're right. I totally didn't see the syncInterval line.
* prefs now uses SCORE_INCREMENT as well
* Score incrementing tests for prefs, bookmarks, passwords all in test_<engine>_tracker.js files
Attachment #537219 - Attachment is obsolete: true
Attached file WIP - removing sync delays (obsolete) —
* removed unnecessary delays with sync
* 100ms wait on score update
* passes current unit & tps tests

next: 
* add a tests for sufficiently large score update triggering a sync
* removing heartbeat (I think this should be part of a different patch though)
Attachment #537882 - Flags: feedback?(philipp)
oops, last patch was the wrong one, here's the right one.
Attachment #537882 - Attachment is obsolete: true
Attachment #537882 - Flags: feedback?(philipp)
Attachment #537884 - Attachment is patch: true
Comment on attachment 537698 [details] [diff] [review]
Part 1 (WIP): tune score increments

Just a few teeny teeny nits:

>-    // A single add, remove or change is 15 points, all items removed is 50
>+    // A single add, remove or change or removing all items will trigger a sync for MULTI_DEVICE.

nit: please wrap overlong lines to max 80 characters.

>+        // Trigger a sync for MULTI-DEVICE for a change that determines
>+        // which prefs are synced or a regular pref change.
>+        if (aData.indexOf(WEAVE_SYNC_PREFS) == 0 || this._prefs.get(WEAVE_SYNC_PREFS + aData, false)) {

same here.

>     _("Tell the tracker to start tracking changes.");
>     Svc.Obs.notify("weave:engine:start-tracking");
>     createBmk();
>     // We expect two changed items because the containing folder
>     // changed as well (new child).
>     do_check_eq([id for (id in tracker.changedIDs)].length, 2);
>+    do_check_eq(tracker.score, 602);

Would be best if you could compare the score to multiples of the SCORE_INCREMENT constant throughout these tests. That way we only need to change the value in once place.

>diff --git a/services/sync/tests/unit/test_password_tracker.js b/services/sync/tests/unit/test_password_tracker.js
>new file mode 100644

<33333

>+Engines.register(PasswordEngine);
>+let engine = Engines.get("passwords");
>+let store  = engine._store;
>+store.wipe();

The store.wipe() shouldn't be necessary.

>+  function createPassword() {
>+    _("RECORD NUM: " + recordNum);
>+    let record = {id: "GUID" + recordNum,
>+                    hostname: "http://foo.bar.com",
>+                    formSubmitURL: "http://foo.bar.com/baz",
>+                    username: "john" + recordNum,
>+                    password: "smith",
>+                    usernameField: "username",
>+                    passwordField: "password"};

nit: align indention please.

r=philikon with those addressed.
Attachment #537698 - Flags: feedback+
Attachment #537698 - Attachment description: WIP → Part 1 (WIP): tune score increments
Comment on attachment 537884 [details] [diff] [review]
Part 2 (WIP): removing sync delays

>-    Weave.Service.syncOnIdle(1);
>+    // The 'Weave' object will disappear once the window closes.
>+    let Service = Weave.Service;
>+    Weave.Utils.nextTick(function() Service.checkSyncReady(), Service);

Actually, I realized we can also collapse this to

  Weave.Utils.nextTick(Weave.Service.checkSyncReady, Weave.Service);

That way we don't need to create a local reference to Weave.Service because there's no closure that will live on longer.

That said, we should call Weave.Service.sync directly here. All that checkSyncReady does is abort if the master password is locked, but the wizard ensures it's *always* unlocked for the duration of the wizard, so no need for that check. (More on checkSyncReady below.)

>       case "weave:engine:score:updated":
>-        this._handleScoreUpdate();
>+        let now = Date.now();
>+        if (this._lastScoreUpdate && (now - this._lastScoreUpdate) >= 100) {
>+          this._calculateScore();
>+        }
>+        this._lastScoreUpdate = now;

This won't implement the desired behaviour. This will immediately trigger a sync on the first bookmark of a batch operation, but we always want to delay for 100ms since there might be another pending score update. You really want to do what _handleScoreUpdate did:

    Utils.namedTimer(this._calculateScore, SCORE_UPDATE_DELAY, this,
                     "_scoreTimer");

where you define const SCORE_UPDATE_DELAY = 100 as a global const. This will be enough to get the desired behaviour: we'll calculate the score in at least 100ms, but if we get more updates in the mean time, we'll delay further. (This magic is brought to us by Utils.namedTimer.)


>-  /**
>-   * Call sync() on an idle timer
>-   *
>-   * delay is optional
>-   */
>-  syncOnIdle: function WeaveSvc_syncOnIdle(delay) {
>-    // No need to add a duplicate idle observer, and no point if we got kicked
>-    // out by the master password dialog.
>+  checkSyncReady: function WeaveSvc_checkSyncReady(delay) {
>+    // No point if we got kicked out by the master password dialog.
>     if (Status.login == MASTER_PASSWORD_LOCKED &&
>         Utils.mpLocked()) {
>-      this._log.debug("Not syncing on idle: Login status is " + Status.login);
>+      this._log.debug("Not initiating sync: Login status is " + Status.login);
> 
>       // If we're not syncing now, we need to schedule the next one.
>       this._scheduleAtInterval(MASTER_PASSWORD_LOCKED_RETRY_INTERVAL);
>       return false;
>     }
> 
>-    if (this._idleTime)
>-      return false;
>-
>-    this._idleTime = delay || IDLE_TIME;
>-    this._log.debug("Idle timer created for sync, will sync after " +
>-                    this._idleTime + " seconds of inactivity.");
>-    Svc.Idle.addIdleObserver(this, this._idleTime);
>+    this.sync();
>   },

Couple of things here:

* "checkSyncReady" basically exists so that we can check whether the Master Password is locked or not. The reason for that is that we don't want to kick off a background sync if the MP is locked since that will trigger the MP dialog out of nowhere which would be pretty scary. On that basis, "checkSyncReady" isn't an ideal name since it really kicks off a sync (conditionally). So I suggest calling it something like "syncIfMPUnlocked".

* It seems like you want it to have a boolean return value depending on whether the MP was locked or not, except you never actually return true. The previous function that was there, syncOnIdle, had the same problem btw. I don't think it's really necessary to have that return value anyway, so let's just get rid of it. The one place where it's used it's just for logging. Also, you're calling this.sync() synchronously in the end, which right now at least still blocks execution, so your return logic wouldn't work so well anyway.

* Speaking of calling this.sync() synchronously... You should either (a) not do that and use Utils.nextTick(this.sync, this) OR (b) make sure you never invoke checkSyncReady that way. It's probably easier to just do (a), so that's what I'd recommend.

* You removed the comment explaining what the function does. Please add one back.

* For new functions we define we stick with the "foo: function foo()" pattern.

* Tests?!?

>     // Start the sync right away if we're already late
>     if (interval <= 0) {
>-      if (this.syncOnIdle())
>-        this._log.debug("Syncing as soon as we're idle.");
>+      if (this.checkSyncReady())
>+        this._log.debug("Syncing now.");

As mentioned above, get rid of the if() + logging call here. Just invoke checkSyncReady().

>     this._log.trace("Next sync in " + Math.ceil(interval / 1000) + " sec.");
>-    Utils.namedTimer(this.syncOnIdle, interval, this, "_syncTimer");
>+    Utils.namedTimer(function() this.checkSyncReady(), interval, this, "_syncTimer");

can just write as

  Utils.namedTimer(this.checkSyncReady, interval, this, "_syncTimer");

>@@ -1602,17 +1572,17 @@ WeaveSvc.prototype = {
>       info = new Resource(this.infoURL).get();
>       if (info && info.success) {
>         // if clients.lastModified doesn't match what the server has,
>         // we have another client in play
>         this._log.trace("Remote timestamp:" + info.obj["clients"] +
>                         " Local timestamp: " + Clients.lastSync);
>         if (info.obj["clients"] > Clients.lastSync) {
>           this._log.debug("New clients detected, triggering a full sync");
>-          this.syncOnIdle();
>+          this.checkSyncReady();
>           return;
>         }
>       }
>       else {
>         this._checkServerError(info);
>         this._log.debug("Heartbeat failed. HTTP Error: " + info.status);
>       }
>     } catch(ex) {

I thought we were just going to rip out the whole heartbeat thing? I guess separate patch is better...
Attachment #537884 - Attachment description: WIP - removing sync delays → Part 2 (WIP): removing sync delays
* nit fixes
Attachment #537698 - Attachment is obsolete: true
Attachment #538087 - Attachment is patch: true
* changes recommended in comment 17 made
* added tests

philikon: 
in the second test I'm overriding _checkSync which was a quick and dirty way to ignore not being logged in and moving on. Is there a less hacky method that can mimic being logged in? Also, not sure if this is what you generally had in mind for testing without delays.
Attachment #537884 - Attachment is obsolete: true
Attached patch Part 3 (WIP): remove heartbeat (obsolete) — Splinter Review
This seemed simpler than I expected. Hopefully I didn't miss anything.
Attachment #538178 - Attachment description: Patch 3 (WIP): remove heartbeat → Part 3 (WIP): remove heartbeat
Comment on attachment 538176 [details] [diff] [review]
Part 2 (WIP): removing sync delays

Nice. Just some comments on the test.

> new file mode 100644
> --- /dev/null
> +++ b/services/sync/tests/unit/test_instantsync_scoreTriggeredSync.js

Nit: that filename is quite a mouthful. test_score_triggers.js seems descriptive enough to me...

Also, test files should have the Public Domain licence header (https://www.mozilla.org/MPL/boilerplate-1.1/pd-c). We haven't been very good about this in the past, but we should do it for new files or files we touch.

>+/*
>+ * Tests start here.
>+ */

O RLY?!? :p

>+let svc = Weave.Service;

'svc' is a bit confusing as util.js also exposes an object called Svc. If you don't feel like typing Weave.Service everywhere, then you could at least import service.js and use Service (without the Weave. prefix).

>+function test_score_updated() {
>+  let scoreUpdated = 0;
>+
>+  Svc.Obs.add("weave:engine:score:updated", function() {
>+    scoreUpdated++;
>+  });
>+
>+  try {
>+    do_check_eq(engine.score, 0);
>+
>+    tracker.score += SCORE_INCREMENT;
>+    do_check_eq(engine.score, SCORE_INCREMENT);

With this test you're likely to trigger a sync, which isn't what we want to test here. So perhaps use a smaller increment?

>+    do_check_eq(scoreUpdated, 1);
>+  } finally {
>+    tracker.resetScore();
>+  }
>+}

Nit: Since this tests Tracker functionality, I suggest calling this function test_tracker_score_updated(). Also, please wrap this test function in add_test() and call run_next_test() in the end (see below for explanation... just mentioning this up here so the comment is next to the code.)

>+function test_sync_triggered() {
>+  tracker.score += SCORE_INCREMENT;
>+  svc.syncThreshold = MULTI_DEVICE_THRESHOLD;
>+  let syncTriggered = 0;
>+  let checkedSync = 0;
>+
>+  // Ignore user is not logged in.
>+  svc._checkSync = function() {
>+    _("Ignore user not logged in.");
>+    checkedSync++;
>+    return "";
>+  }

If the user isn't logged in, better fake the login rather than monkey patching this method. You could create a minimal server setup and call Service.login() to do that. Other tests have the necessary bits you could steal.

>+  svc.syncIfMPUnlocked = function() {
>+    _("Sync will be triggered!");
>+    syncTriggered++;
>+  }

Why are you monkey patching this? We want to check that a sufficiently high score update actually triggers a sync no? We're now overwriting the method that actually calls Service.sync!

>+  // This would be called after a delay in weave:engine:score:update.
>+  svc._calculateScore();

*chuckle* you big fat cheater! We want to test that _calculateScore is called, no? Calling it ourselves is... well... cheating!

So what do we want to test? We want to basically trigger a score update and then ensure Service.sync() is called eventually. We kinda know how long it will take (SCORE_UPDATE_DELAY), but hard coding timings into tests is brittle and invites random oranges.

But, fortunately, the Service notifies us when it starts and finishes a Sync! So, we could simply add an observer for this and then trigger a score update. If everything works as intended, the Service will trigger a sync and eventually call our observer. Something like this perhaps:

add_test(function test_score_increment_triggers_sync() {
  let server = sync_httpd_setup({...});
  Service.login();

  Service.syncThreshold = MULTI_DEVICE_THRESHOLD;
  Svc.Obs.add("weave:service:sync:finish", function onSyncFinish() {
    // We synced, yay!
    server.stop(run_next_test);
  });

  let tracker = new SteamEngine()._tracker;
  tracker.score += SCORE_INCREMENT;
});

As you can probably tell, this requires an async testing pattern, because we push something off and then have to wait until we get called back (via an observer notification in this case). So we need to indicate this to the test runner because now the test isn't over yet when the test function completes (because we're still waiting for Service to do stuff and call us back). We do this by wrapping your test function in add_test() and then explicitly calling run_next_test() when the test is finished.

You'll need quite a bit of boilerplate to satisfy Service.sync's needs, but since you'll already have to set up a fake server in order to do Service.login(). Again, have a look at other tests that call Service.sync() with a fake engine.

>+function run_test() {
>+  initTestLogging("Trace");
>+
>+  Log4Moz.repository.getLogger("Service.Main").level = Log4Moz.Level.Trace;
>+
>+  test_score_updated();
>+  test_sync_triggered();
>+}

With both test functions wrapped in add_test() and using run_next_test(), you no longer have to invoke them here. Instead, just call run_next_test(). (I also prefer defining run_test() at the top of the file so the file reads in execution order.)
Attachment #538176 - Flags: feedback+
Comment on attachment 538087 [details] [diff] [review]
Part 1 (WIP): tune score increments

I forgot one minor thing: The history, forms and tabs engine also increment the score, of course. Most of the time their increments should be much lower than the default SCORE_INCREMENT, except when we actually really really want to sync.

This, for instance, is the case when the user wipes their history. See HistoryTracker.prototype.onClearHistory(). Can you replace the hardcoded 500 value with SCORE_INCREMENT, too, please?

For every other change, history and tabs increment the score by 1 (tabs only 10% of the time for most tab events, which is a bit of a WTF) and forms by 10. Those values seem ok to me, but it would be good if we could const these values while we're at it.
* added const for score increments in history, tabs, & forms
Attachment #538087 - Attachment is obsolete: true
Attachment #538339 - Attachment is patch: true
> > new file mode 100644
> > --- /dev/null
> > +++ b/services/sync/tests/unit/test_instantsync_scoreTriggeredSync.js
> 
> Nit: that filename is quite a mouthful. test_score_triggers.js seems
> descriptive enough to me...

I'm happy with any name but I was just going by the test_<component>_<behaviour>.js naming convention you mentioned and was thinking the component that will drive instant sync will be called instantsync.
* addressing points in comment 21
Attachment #538176 - Attachment is obsolete: true
* noticed extra/unnecessary line of code in the last attachment
Attachment #538596 - Attachment is obsolete: true
* pulling out score & interval related code from service & putting it in a separate component
* pulled out more stuff
Attachment #539086 - Attachment is obsolete: true
Attachment #539372 - Flags: feedback?(philipp)
Comment on attachment 538339 [details] [diff] [review]
Part 1 (WIP): tune score increments

SCORE_INCREMENT_XLARGE... nice :)
Attachment #538339 - Flags: review+
Comment on attachment 538598 [details] [diff] [review]
Part 2 (WIP): removing sync delays

>+/*
>+ * A stub engine implementation.
>+ */
...

rnewman just uplifted this to the head files (see bug 656513 part 0), so you won't have to define your own here. He called it RotaryEngine, though.

>+function run_test() {
>+  initTestLogging("Trace");
>+
>+  Log4Moz.repository.getLogger("Service.Main").level = Log4Moz.Level.Trace;

The Service.Main logger has been renamed to Sync.Service (see bug 661587).

r+ with those minor things addressed. Nice work!
Attachment #538598 - Flags: review+
Comment on attachment 539372 [details] [diff] [review]
Part 4 (WIP): creating instantsync component

This is a great start!

>diff --git a/services/sync/modules/instantsync.js b/services/sync/modules/instantsync.js
>new file mode 100644
>--- /dev/null
>+++ b/services/sync/modules/instantsync.js

Since we're planning to rip a couple more things out of Service (error reporting policy, logging policy, etc.), how about we call this file policies.js?

Also, please don't forget the licence header. Please copy it from https://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c. The original code is "Firefox Sync", the initial developer is "the Mozilla Foundation", the copyright is (C) 2011 and you should add yourself to the contributors section.

Actually, you should always add yourself to the contributors section when you make significant changes to any file (forgot to mention this earlier, sorry).


Ok, first things first: bikeshedding!

>+let InstantSync = {

I wonder if we want to actually call this InstantSync (which describes a property) or maybe something more generic, like SyncScheduler. Also, it will do a little bit more than just trigger based on (non-)idle and score updates (stuff like backoff and error handling, see below).

>+  _log: Log4Moz.repository.getLogger("InstantSync"),

Please prefix the logger name with "Sync." (see bug 661587).

>+  addObservers: function addObservers() {

I suggest calling this init() since at some point it might do more than just add observers.

>+  observe: function Error_observe(subject, topic, data) {

Error_observe?

>+  _calculateScore: function _calculateScore() {
...
>+  _checkSyncStatus: function _checkSyncStatus() {
...
>+  _syncIfMPUnlocked: function _syncIfMPUnlocked() {
etc.

Can we get rid of the ugly underscore on methods we move over?


Now a bit more serious matters ;)

+  /**
+   * Check if we should be syncing and schedule the next sync, if it's not scheduled
+   */
+  _checkSyncStatus: function _checkSyncStatus() {
...
+    Svc.Obs.notify("weave:instantsync:schedule-next-sync", wait);
+  },

Why doesn't it just call this.scheduleNextSync(wait)?

It looks like you're using observer notifications as alternate method calls. That's not really what they're there for. Their purpose is to allow pluggability, so that one component can notify others about stuff that it's doing, without knowing who or what those components are and what they're going to do. You're kind of using it in the opposite way.

>-  /**
>    * Incorporates the backoff/retry logic used in error handling and elective
>    * non-syncing.
>    */
>-  _scheduleAtInterval: function _scheduleAtInterval(minimumInterval) {
>+  scheduleAtInterval: function scheduleAtInterval(minimumInterval) {
>     const MINIMUM_BACKOFF_INTERVAL = 15 * 60 * 1000;     // 15 minutes
>     let interval = this._calculateBackoff(this._syncErrors, MINIMUM_BACKOFF_INTERVAL);
>     if (minimumInterval)
>       interval = Math.max(minimumInterval, interval);
> 
>     let d = new Date(Date.now() + interval);
>     this._log.config("Starting backoff, next sync at:" + d.toString());
> 
>-    this._scheduleNextSync(interval);
>+    Svc.Obs.notify("weave:instantsync:schedule-next-sync", interval);
>   },

I think this method should move to the SyncScheduler, too, including all the backoff handling stuff (e.g. the "weave:service:backoff:interval" observer). Service._calculateBackoff might have to move to Utils for that so it can be used from both Service and SyncScheduler.

> 
>   _syncErrors: 0,
>   /**
>    * Deal with sync errors appropriately
>    */
>   _handleSyncError: function WeaveSvc__handleSyncError() {
>     this._syncErrors++;
> 
>     // Do nothing on the first couple of failures, if we're not in
>     // backoff due to 5xx errors.
>     if (!Status.enforceBackoff) {
>       if (this._syncErrors < MAX_ERROR_COUNT_BEFORE_BACKOFF) {
>-        this._scheduleNextSync();
>+        Svc.Obs.notify("weave:instantsync:schedule-next-sync");

This branch of _handleSyncError and the whole _syncError counting would have to move to SyncScheduler, too. (_handleSyncError is called on the "weave:service:sync:error" observer notification.)


>@@ -1600,22 +1469,22 @@ WeaveSvc.prototype = {
>     let dateStr = new Date().toLocaleFormat(LOG_DATE_FORMAT);
>     this._log.info("Starting sync at " + dateStr);
>     this._catch(function () {
>       // Make sure we're logged in.
>       if (this._shouldLogin()) {
>         this._log.debug("In sync: should login.");
>         if (!this.login()) {
>           this._log.debug("Not syncing: login returned false.");
>-          this._clearSyncTriggers();    // No more pending syncs, please.
>+          Svc.Obs.notify("weave:clear-sync-triggers");    // No more pending syncs, please.

Login failed, right? So why can't the SyncScheduler do this in the "weave:service:login:failed" observer?

> 
>           // Try again later, just as if we threw an error... only without the
>           // error count.
>           if (!this._skipScheduledRetry())
>-            this._scheduleAtInterval(MASTER_PASSWORD_LOCKED_RETRY_INTERVAL);
>+            this.scheduleAtInterval(MASTER_PASSWORD_LOCKED_RETRY_INTERVAL);

Same here.

>     // if we don't have a node, get one.  if that fails, retry in 10 minutes
>     if (this.clusterURL == "" && !this._setCluster()) {
>       Status.sync = NO_SYNC_NODE_FOUND;
>-      this._scheduleNextSync(10 * 60 * 1000);
>+      Svc.Obs.notify("weave:instantsync:schedule-next-sync", 10 * 60 * 1000);
>       return;
>     }

WOAH. Can you please const those numbers, e.g. as NO_SYNC_NODE_INTERVAL? Thanks!

So, this is in Service._lockedSync, which is essentially the inner sync function. Here were return silently if we have no Sync node, but we do set Status.sync! So the SyncScheduler can listen to "weave:service:sync:finish" and check if NO_SYNC_NODE_FOUND, right? In that case, schedule the next sync for NO_SYNC_NODE_INTERVAL.


>     // Clear out any potentially pending syncs now that we're syncing
>-    this._clearSyncTriggers();
>-    this.nextSync = 0;
>+    Svc.Obs.notify("weave:clear-sync-triggers"); 
>+    InstantSync.nextSync = 0;

You can do that in SyncScheduler on weave:service:sync:start.

>     // reset backoff info, if the server tells us to continue backing off,
>     // we'll handle that later
>     Status.resetBackoff();

and that.

>     // Figure out what the last modified time is for each collection
>     let info = this._fetchInfo(infoURL);
>-    this.globalScore = 0;
>+    InstantSync.globalScore = 0;

That too.

>       }
>       finally {
>         // Always immediately push back the local client (now without commands)
>         this._syncEngine(Clients);
>       }
>     }
> 
>     // Update the client mode and engines because it might change what we sync.
>-    this._updateClientMode();
>+    InstantSync.updateClientMode();

So this happens when the Clients engine has just synced, right? So you could do this in on the "weave:engine:sync:finished" observer in the SyncScheduler. The engine sends its name along as the subject, so just check for subject == "clients".


I wouldn't be surprised if only half of the code we're touching here actually has tests (e.g. the backoff stuff). Bonus points for writing tests for code that doesn't :)
Attachment #539372 - Flags: feedback?(philipp) → feedback+
> Since we're planning to rip a couple more things out of Service (error
> reporting policy, logging policy, etc.), how about we call this file
> policies.js?

I was thinking maybe error handling shouldn't be in the same file as scheduling. Otherwise we'll just end up getting another huge file just like service.js which I think is what we're trying to avoid.  I agree that so far this file is looking more like a sync scheduler but I was hesitant to add error handling stuff in there for this reason.

 
> >+  _calculateScore: function _calculateScore() {
> ...
> >+  _checkSyncStatus: function _checkSyncStatus() {
> ...
> >+  _syncIfMPUnlocked: function _syncIfMPUnlocked() {
> etc.
> 
> Can we get rid of the ugly underscore on methods we move over?
 
As far as I could tell, the underscores are used to identify a function as "private". So functions with an underscore are only used within the component they're defined in. If this isn't a generally followed convention then sure.
More bikeshedding! :)

(In reply to comment #32)
> > Since we're planning to rip a couple more things out of Service (error
> > reporting policy, logging policy, etc.), how about we call this file
> > policies.js?
> 
> I was thinking maybe error handling shouldn't be in the same file as
> scheduling. Otherwise we'll just end up getting another huge file just like
> service.js which I think is what we're trying to avoid.

It isn't really the huge file but the huge object that does sort of everything. Having well-defined borders between them (e.g. observer notifications) helps with maintainability as it separates concerns.

I could see policies.js taking over about half of service.js and I think that'd be just fine.

> I agree that so far
> this file is looking more like a sync scheduler but I was hesitant to add
> error handling stuff in there for this reason.

Right, well, I'm not saying the SyncScheduler should contain *all* error handling, just the teeny bits for the backoff stuff. All the other error handling is for another policy object which we'll create as part of Death to Unknown Error.

> > Can we get rid of the ugly underscore on methods we move over?
>  
> As far as I could tell, the underscores are used to identify a function as
> "private". So functions with an underscore are only used within the
> component they're defined in. If this isn't a generally followed convention
> then sure.

private v. public isn't really a big concern here since this is JavaScript (your use of quotation mark was quite correct.) The SyncScheduler doesn't really have any public methods anyway, does it? I think it mostly communicates via notifications, so we might as well make the code a bit more readable by removing the underscores.
(In reply to comment #33)

> private v. public isn't really a big concern here since this is JavaScript
> (your use of quotation mark was quite correct.) The SyncScheduler doesn't
> really have any public methods anyway, does it? I think it mostly
> communicates via notifications, so we might as well make the code a bit more
> readable by removing the underscores.

Indeed, this convention is horribly misused throughout Sync, with methods that other classes invoke sometimes starting with an underscore, and some methods that aren't safe for public consumption not so indicated. Oh, and sometimes it just means "this is a wrapped version", as in Engine._sync/Engine.sync.

Oh well.
* final minor changes
Attachment #538598 - Attachment is obsolete: true
(In reply to comment #31)
> Comment on attachment 539372 [details] [diff] [review] [review]
> Part 4 (WIP): creating instantsync component

> >     // Figure out what the last modified time is for each collection
> >     let info = this._fetchInfo(infoURL);
> >-    this.globalScore = 0;
> >+    InstantSync.globalScore = 0;
> 
> That too.

This change causes a problem in test_service_sync_401.js in the following line:

do_check_eq(SyncScheduler.globalScore, GLOBAL_SCORE);

It looks like we want to score to only get reset if there were no exceptions thrown due to login rejection. However, there are other exceptions that could
...

cause an abort in sync later on in the code. So it's not clear whether we want the score to only get reset if sync is successful or if it's simply attempted. What is the desired behaviour here? I think it makes the most sense to me to reset the score only if sync was successful (on sync finish) ?
(In reply to comment #37)
> I think it makes the most sense to me to reset the score only if sync
> was successful (on sync finish) ?

This seems logical at first, but do keep in mind that users might easily get into a situation where no sync ever actually completes for them successfully (because they have a bogus record on the server, and even though we may only report the first failure to the user thanks to bug 659107, Service.sync() still fails.)

I would look at it this way: The score exists to trigger a sync at the appropriate time. As soon as Service.sync() gets called, it has fulfilled its purpose. Whatever happens afterwards isn't the SyncScheduler's concern.

So I would say we change the test here to reflect the new reality.
This bug is getting pretty big, so we've decided to split this work into two bugs. We'll make this bug about tuning the score updates to trigger a sync sooner (part 1 + 2). Bug 664792 will take over the part of scheduling sync based on user behaviour (part 3 + 4 + 5).
Summary: refine sync frequency to better match user activity → Tune score increments to trigger a sync more quickly
Attachment #539372 - Attachment is obsolete: true
Attachment #538178 - Attachment is obsolete: true
Attachment #538339 - Attachment is obsolete: true
STRs for QA:

These actions should now trigger syncs immediately (within a 100ms):

* Adding/removing/moving/... a bookmark
* Adding/removing a password 
* wiping your history
* changing one of the preferences that's synced
Pushed a small follow-up to ensure logger names have the Sync prefix (cf bug 661587):
https://hg.mozilla.org/services/services-central/rev/b6b2f9663847
note:  You MUST have at least two clients for this to work correctly.

Changes in bookmarks, passwords and prefs are triggering instant syncs correctly.  However, history syncs are blocked by bug 578694. (which I marked as a dependency)

per irc: philikon said to proceed with known bugs.
Depends on: 578694
Whiteboard: [fixed in services] → [verifed in services]
http://hg.mozilla.org/mozilla-central/rev/cabd5ab4129f
http://hg.mozilla.org/mozilla-central/rev/6d462434be3a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Status: RESOLVED → VERIFIED
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.