Bug 1304389 - Don't write session file if content hasn't changed; r?Yoric draft
authorGregory Szorc <gps@mozilla.com>
Fri, 23 Sep 2016 10:09:12 -0700
changeset 417098 abfa99ad90633d1f3a875e98fe7556a013876a71
parent 417097 04c37779beda52b343ff125b429a265dc91e1115
child 532030 0c96a0e284698acac201544e666d03068918bc33
push id30338
push usergszorc@mozilla.com
push dateFri, 23 Sep 2016 17:09:43 +0000
reviewersYoric
bugs1304389
milestone52.0a1
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
browser/components/sessionstore/SessionWorker.js
--- 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) {