Bug 1304389 - Don't write session file if content hasn't changed; r?Yoric
(THIS COMMIT CURRENTLY FAILS BECAUSE Components IS NOT AVAILABLE IN
THE PROMISEWORKER. I'M NOT SURE HOW TO FIX IT.)
I saw a HN post about Firefox chewing through I/O writing recovery.js
every few seconds. I figured I'd take a stab in the dark at fixing it.
This commit adds logic to the sessionstore worker to no-op writes
if written content hasn't changed. It does so by maintaining a hash
of the last written data. If the hash of data we're writing is
identical to a previous write, we skip the write.
The code doesn't achieve absolute minimum write I/O. For example, we
could hash the file at read time to avoid the initial write. This
optimization can be performed later, if wanted. I didn't do it because
I was striving to keep the change as small as possible.
For this change to have the desired effect to reduce I/O, an idle
Firefox's session state must not change. If there are things like
timestamps derived from Date.now() sneaking into the session data,
this would undermine this optimization.
This change also assumes that it is acceptable to not update the
mtime of the session file. I don't see any code looking at the mtime,
so I think this will be fine.
MozReview-Commit-ID: 7FSkgmUdO8T
--- a/browser/components/sessionstore/SessionWorker.js
+++ b/browser/components/sessionstore/SessionWorker.js
@@ -3,16 +3,18 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */
/**
* A worker dedicated to handle I/O for Session Store.
*/
"use strict";
+const { classes: Cc, interfaces: Ci } = Components;
+
importScripts("resource://gre/modules/osfile.jsm");
var PromiseWorker = require("resource://gre/modules/workers/PromiseWorker.js");
var File = OS.File;
var Encoder = new TextEncoder();
var Decoder = new TextDecoder();
@@ -78,16 +80,21 @@ var Agent = {
state: null,
/**
* Number of old upgrade backups that are being kept
*/
maxUpgradeBackups: null,
/**
+ * Hash of last written content.
+ */
+ lastWriteHash: null,
+
+ /**
* Initialize (or reinitialize) the worker
*
* @param {string} origin Which of sessionstore.js or its backups
* was used. One of the `STATE_*` constants defined above.
* @param {object} paths The paths at which to find the various files.
* @param {object} prefs The preferences the worker needs to known.
*/
init(origin, paths, prefs = {}) {
@@ -145,16 +152,24 @@ var Agent = {
tab.entries = tab.entries.slice(lower, upper);
tab.index -= lower;
}
}
}
let stateString = JSON.stringify(state);
let data = Encoder.encode(stateString);
+
+ let hasher = Cc["@mozilla.org/security/hash;1"]
+ .createInstance(Ci.nsICryptoHash);
+ hasher.init(hasher.SHA1);
+ hasher.update(data, data.length);
+ let currentHash = hasher.finish(false);
+
+ let performedWrite = false;
let startWriteMs, stopWriteMs;
try {
if (this.state == STATE_CLEAN || this.state == STATE_EMPTY) {
// The backups directory may not exist yet. In all other cases,
// we have either already read from or already written to this
// directory, so we are satisfied that it exists.
@@ -162,17 +177,18 @@ var Agent = {
}
if (this.state == STATE_CLEAN) {
// Move $Path.clean out of the way, to avoid any ambiguity as
// to which file is more recent.
File.move(this.Paths.clean, this.Paths.cleanBackup);
}
- if (true) {
+ /* Avoid I/O if the end result would be the same. */
+ if (currentHash != this.lastWriteHash) {
startWriteMs = Date.now();
if (options.isFinalWrite) {
// We are shutting down. At this stage, we know that
// $Paths.clean is either absent or corrupted. If it was
// originally present and valid, it has been moved to
// $Paths.cleanBackup a long time ago. We can therefore write
// with the guarantees that we erase no important data.
@@ -194,16 +210,18 @@ var Agent = {
// In other cases, either $Path.recovery is not necessary, or
// it doesn't exist or it has been corrupted. Regardless,
// don't backup $Path.recovery.
File.writeAtomic(this.Paths.recovery, data, {
tmpPath: this.Paths.recovery + ".tmp"
});
}
+ performedWrite = true;
+ this.lastWriteHash = currentHash;
stopWriteMs = Date.now();
}
} catch (ex) {
// Don't throw immediately
exn = exn || ex;
}
@@ -269,31 +287,34 @@ var Agent = {
}
this.state = STATE_RECOVERY;
if (exn) {
throw exn;
}
+ if (performedWrite) {
+ telemetry[FX_SESSION_RESTORE_WRITE_FILE_MS] = stopWriteMs - startWriteMs;
+ telemetry[FX_SESSION_RESTORE_FILE_SIZE_BYTES] = data.byteLength;
+ }
+
return {
result: {
upgradeBackup: upgradeBackupComplete
},
- telemetry: {
- FX_SESSION_RESTORE_WRITE_FILE_MS: stopWriteMs - startWriteMs,
- FX_SESSION_RESTORE_FILE_SIZE_BYTES: data.byteLength,
- }
+ telemetry: telemetry,
};
},
/**
* Wipes all files holding session data from disk.
*/
wipe: function () {
+ this.lastWriteHash = null;
// Don't stop immediately in case of error.
let exn = null;
// Erase main session state file
try {
File.remove(this.Paths.clean);
} catch (ex) {