synchronous I/O sucks. We should stop doing that.

With bug 609395 and bug 609395 we are going to remove the only two consumers for Utils.json{Load|Save}, changedIDs and toFetch, making this obsolete.

We're not going to be able to get rid of all disk I/O, so I'm reopening this.

Utils.jsonLoad() is already async, but Utils.jsonSave() has an ill-named 'callback' parameter which this renames to 'obj'. An optional callback parameter (in the async sense) is added. Tests are refactored to be async.

Comment on attachment 507054 [details] [diff] [review] Part 1: Utils.jsonSave/Load should be async Gah, uploaded the wrong file. This was actually part 1.

Sorry for the confusion. To summarize: * Part 0 simply refactors the API and tests for Utils.json{Save|Load} to be async. * Part 1 changes them to use the asynchronous I/O methods from NetUtil when available.

Comment on attachment 507054 [details] [diff] [review] Part 1: Utils.jsonSave/Load should be async >+ let fos = Cc["@mozilla.org/network/file-output-stream;1"] >+ .createInstance(Ci.nsIFileOutputStream); >+ fos.init(file, MODE_WRONLY | MODE_CREATE | MODE_TRUNCATE, PERMS_FILE, 0); >+ let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"] >+ .createInstance(Ci.nsIScriptableUnicodeConverter); >+ converter.charset = "UTF-8"; >+ let is = converter.convertToInputStream(out); >+ NetUtil.asyncCopy(is, fos, function (result) { >+ fos.close(); >+ if (typeof callback == "function") { >+ callback.call(that); >+ } >+ }); I suspect you actually want to use an safe file output stream here like session store too: https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#3875 That, and you shouldn't have to close the output stream. asyncCopy does that for you.

Thx for the drive-by feedback, sdwilsh. Updated!

Comment on attachment 507056 [details] [diff] [review] Part 0: Make API and tests for Utils.json{Load|Save} async > loadChangedIDs: function T_loadChangedIDs() { > Utils.jsonLoad("changes/" + this.file, this, function(json) { >- this.changedIDs = json; >+ if (json) { >+ this.changedIDs = json; >+ } do we init this.changedIDs to something? > loadToFetch: function loadToFetch() { > // Initialize to empty if there's no file > this._toFetch = []; > Utils.jsonLoad("toFetch/" + this.name, this, function(toFetch) { >- this._toFetch = toFetch; >+ if (toFetch) { >+ this._toFetch = toFetch; >+ } this._toFetch = toFetch || []; ?

Comment on attachment 507166 [details] [diff] [review] Part 1: Utils.jsonSave/Load should be async (v1.1) >- let json; >- try { >- let [is] = Utils.open(file, "<"); >- json = JSON.parse(Utils.readStream(is)); >+ // Gecko < 2.0 itym gecko < 1.9.2 given that NetUtil is on 3.6... or am I missing something? >+ if (!NetUtil || !NetUtil.newChannel) {

(In reply to comment #10) > itym gecko < 1.9.2 given that NetUtil is on 3.6... or am I missing something? There have been recent additions to NetUtil in 2.0 (namely, readInputStreamToString).

(In reply to comment #9) > > loadChangedIDs: function T_loadChangedIDs() { > > Utils.jsonLoad("changes/" + this.file, this, function(json) { > >- this.changedIDs = json; > >+ if (json) { > >+ this.changedIDs = json; > >+ } > > do we init this.changedIDs to something? Yes, in the Tracker constructor. > > loadToFetch: function loadToFetch() { > > // Initialize to empty if there's no file > > this._toFetch = []; > > Utils.jsonLoad("toFetch/" + this.name, this, function(toFetch) { > >- this._toFetch = toFetch; > >+ if (toFetch) { > >+ this._toFetch = toFetch; > >+ } > > this._toFetch = toFetch || []; ? Doesn't seem like a big win to me. We need to make sure we have this.toFetch set immediately so we need to keep that first line (this._toFetch = []). (In reply to comment #10) > >- let json; > >- try { > >- let [is] = Utils.open(file, "<"); > >- json = JSON.parse(Utils.readStream(is)); > >+ // Gecko < 2.0 > > itym gecko < 1.9.2 given that NetUtil is on 3.6... or am I missing something? > > >+ if (!NetUtil || !NetUtil.newChannel) { You are missing something, namely the line directly below your comment. Firefox 3.6 doesn't have NetUtil.newChannel which NetUtil uses when you pass in a file to NetUtil.asyncFetch(). So this definitely is 4.0 only. I'm not sure I care much, though. First off, it's 3.6, so meh. Secondly, Utils.jsonLoad() is only used at startup, the performance critical bit is Util.jsonSave() because that's called during syncs and tracking.

Part 0: https://hg.mozilla.org/services/fx-sync/rev/499d6d614ffe Part 1: https://hg.mozilla.org/services/fx-sync/rev/01710174d221