630720 - Form sync: consider batching in transactions to avoid fsyncs

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Each time we add or modify a form item, we cause a SQLite transaction and an fsync which hurts a lot on mobile. As a stop gap before we get bug 630705 we could limit the number of fsyncs by having Satchel operate in a fewer transactions.

Demo code by sdwilsh:


var db = ...;
var hasTransaction = false;
try {
  db.beginTransaction();
  hasTransaction = true;
}
catch(e) { /* om nom nom exceptions */ }
// DO NOT EVER HAVE ANY CODE THAT COULD EVER THROW HERE EVER
try {
  // Do your work
}
finally {
  if (hasTransaction) {
    db.commitTransaction();
  }
}

Comment 1

[Shawn Wilsher :sdwilsh]

You probably want to actually make a helper function in Utils for this since you are likely to want to use it for passwords too:

Utils = {
  runInTransaction: function(aDB, aCallback) {
  var hasTransaction = false;
    try {
     db.beginTransaction();
      hasTransaction = true;
    }
    catch(e) { /* om nom nom exceptions */ }
    // DO NOT EVER HAVE ANY CODE THAT COULD EVER THROW HERE EVER
    try {
      aCallback();
    }
    finally {
      if (hasTransaction) {
        db.commitTransaction();
      }
    }
  }
};

Comment 2

[Philipp von Weitershausen [:philikon]]

Yep, good idea, except the password engine doesn't expose the DB connection :(

Now that we have bug 622762 in place we could turn on batching for the form engine easily and wrap store.applyIncomingBatch() in Utils.runInTransaction().

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Yep, good idea, except the password engine doesn't expose the DB connection :(
That will be fixed in bug 630730 (has patch!).

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: v1

That was pretty quick to whip up. Tests and crossweave pass, but I haven't tested this in the wild (=on Fennec) yet to see whether it makes a difference.

Stuart, Doug: is there a way to get a Fennec try build with this patch in place?

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to comment #4)
> Stuart, Doug: is there a way to get a Fennec try build with this patch in
> place?

Never mind, I see the try server makes 'try-mb-br-andrd-r7-bld' builds, so I pushed off a try build:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=3ae2d95a3e9d

Builds should appear here: https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3ae2d95a3e9d

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 509005 [details] [diff] [review]
v1



>+  applyIncomingBatchSize: HISTORY_STORE_BATCH_SIZE,

Can we rename this const, or create a new one if we want to tweak independently?

>+  applyIncomingBatch: function applyIncomingBatch(records) {
>+    return Utils.runInTransaction(Svc.Form.DBConnection, function() {
>+      return Store.prototype.applyIncomingBatch.call(this, records);
>+    }, this);
>+  },
>+

I wonder if this should just be in the main store impl, since we'll want to do this for passwords at some point.  I guess we can move it then, but maybe we want this exposed to third party stuff.

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

>+  runInTransaction: function(db, callback, thisObj) {

>+    } catch(e) { /* om nom nom exceptions */ }

<3 this comment.

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to comment #6)
> >+  applyIncomingBatch: function applyIncomingBatch(records) {
> >+    return Utils.runInTransaction(Svc.Form.DBConnection, function() {
> >+      return Store.prototype.applyIncomingBatch.call(this, records);
> >+    }, this);
> >+  },
> >+
> 
> I wonder if this should just be in the main store impl, since we'll want to do
> this for passwords at some point.  I guess we can move it then, but maybe we
> want this exposed to third party stuff.

Well, it's a workaround (read: hack) for now, we really want to call into the respective batch APIs here in the future. Also, the first argument to Utils.runInTransaction is going to change. In fact, for password sync (bug 631001) we need to do this stuff conditionally depending on whether we can access the DB connection or not (we won't be able to on Firefox <4.0b11 or so).

So, I don't think it makes a whole lot of sense to put this in the main Store impl.

Comment 8

[Philipp von Weitershausen [:philikon]]

Just did a test on my Nexus S with an account that *only* has 312 records of form data (no other engines enabled, no history data either).

I enabled the Error Console and set the error console debug level for the sync log to Debug to avoid screwing with the results by adding disk I/O with the file-based log.

The 2011-02-01 nightly needed 55682 ms for processIncoming in Engine.Forms. The above mentioned try build needed 10514 ms for the same records. Further improvements could be achieved by making the batch size bigger (currently 50), but that means we also have to pull down records in a bigger batch size. We should leave that for a separate bug.

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to comment #8)
> Further improvements could be achieved by making the batch size bigger
> (currently 50), but that means we also have to pull down records in a bigger
> batch size. We should leave that for a separate bug.

Filed bug 631133.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> The 2011-02-01 nightly needed 55682 ms for processIncoming in Engine.Forms. The
> above mentioned try build needed 10514 ms for the same records.
Hurray for 81% speed improvements?  Yeah, I think that's hurray.

Comment 11

[Richard Newman [:rnewman]]

Nice job! :D

Comment 12

[Philipp von Weitershausen [:philikon]]

Landed with const change addressed:
https://hg.mozilla.org/services/fx-sync/rev/94142de8000a

Comment 13

[Tony Chung [:tchung]]

flagging for fennec beta 5

Comment 14

[Tony Chung [:tchung]]

noting to self to verify this later on fennec.

philikon: enable error console, set services.sync.log.appender.console to Debug
[1:40pm] philikon: restart fennec
[1:40pm] philikon: connect sync to an account that *only* has form data
[1:40pm] philikon: measure sync time
[1:41pm] philikon: for me it was 5x faster as before
[1:41pm] philikon: i have an account with only form data if you want