337320 - Session-restore service causes Ts regression

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, enhancement)

Description

[Dietrich Ayala (:dietrich)]

The amount is unspecified, as there were so many check-ins prior to freeze for Fx2A2.

A patch yesterday seemed to reduce Ts substantially on Mac/Linux but not Windows. Currently investigating why.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: moves initialization back to observer, moves file processing back into init

This patch moves initialization from browserglue back into an observer, and init's on final-ui-startup. It also moves file processing back into init(), instead of onLoad().

Here are the test numbers from my local tinderbox:

http://numsum.com/spreadsheet/show/20730

Comment 2

[Simon Bünzli]

Please make sure to not forget also moving the check for sessionstore.enabled over to init().

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: updated to include sessionstore.enable

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: turns session-restore off by default

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Yesterday night I checked in a fix to remove sessionstore from the build entirely to see what effect it would have on Ts.

From looking at pacifica's Ts graphs:
Initial state: Fluctuation between 500 and 515
Then bug 76111 was checked in at 2006-05-02 11:08, which moved it to 641 and 656
Then bug 328159 was checked in at 2006-05-05 11:01, which moved it to 672
Then bug 76111's Ts fix was checked in at 2006-05-07 22:30, which moved it back down to 531
Then sessionstore was disabled at 2006-05-09 21:48, which had no effect on Ts
Then the removal of sessionstore from the build was checked in at 2006-05-09 23:18, and the build was clobbered, which brought it down to 515.

So it looks that just loading the extra component is the main cause of the Ts spike. I don't think there is much that can be done about that, beyond optimizing the JavaScript component loader. Similar Ts issues were seen when the EM landed, and when the Search service landed (bug 330887), both of which see added new JS components.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I filed bug 337512 to investigate JS component loading perf a tad.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: patch #1 - sync branch to trunk

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: patch #2 - split into 2 services

If Ts is not affected by patch #1, then we should apply this patch. It splits the service into 2 parts:

SessionStartup: Loads the session file, determines crash state, provides access to the session state.

SessionStoreService: Gets the session state from SessionStartup, and if there's anything to restore, does so.

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: patch #2 - split into 2 services redux

This patch splits session restore into two services.

The first service, nsSessionStartup, loads the session file, analyzes prefs and determines if the session should be restored. It is started at application startup, and does it business at final-ui-startup.

The second service, nsSessionStore, is started at delayedStartup. It restores the session using the data it gets from nsSessionStartup, and manages the rest of the current session.

Would it be worth testing this on a custom tinderbox, as we did with safe-browsing? I'm seeing a non-trivial decrease in Ts in my local tinderbox, but that doesn't mean anything until we get it on an official build box.

Comment 10

[Steffen Wilberg]

Can we reenable sessionstore on the trunk so we can test enhanced features like bug 335864? I applied the fix for that and wondered why it doesn't work until I found out that it's still disabled on trunk.

Comment 11

[Dietrich Ayala (:dietrich)]

Steffen,

This patch re-enables the service. I believe the requirement was to mitigate the TS regression prior to re-enabling.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Wouldn't it make sense to re-enable it before landing the Ts fix, to get a better (more recent, if nothing else) baseline measurement?

Comment 13

[Dietrich Ayala (:dietrich)]

I asked about a special tinderbox, to do that as well as shield us from noise of other checkins. But enabling the service will probably stand out enough for us to tell it apart :)

After review, we can enable it for a cycle or 2 before checkin.

Comment 14

[Simon Bünzli]

With the current Tinderbox setup, you'll get too high Ts values with SessionStore enabled but not recovering from crashes.

The problem is that since the file sessionstore.ini isn't removed at a regular shutdown (supposing that we're measuring Ts for clean startup and not resuming a session), it is parsed again at startup. Without the file, you not only omit the parsing but also the checking for what to do with the previous session state.

Taking this into account, it shouldn't be necessary to split the service in two. But if you still insist, at least keep both services in the same file -- otherwise we additionally get duplicated code.

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to comment #14)
> With the current Tinderbox setup, you'll get too high Ts values with
> SessionStore enabled but not recovering from crashes.
> 
> The problem is that since the file sessionstore.ini isn't removed at a regular
> shutdown (supposing that we're measuring Ts for clean startup and not resuming
> a session), it is parsed again at startup. Without the file, you not only omit
> the parsing but also the checking for what to do with the previous session
> state.

Yes, this is true. Any startup from a crash has 2 disk operations that the normal behavior would not have:

1. reading & parsing the file
2. backing up the file

I did some tinderbox runs to test this, and unsurprisingly got conflicting results. The first set of tests showed that it was slower when removing the ini file between Ts runs (I added a line to build-seamonkey-utils.pl, that rm'd the session file between tests), which is counter-intuitive. But the second set of tests showed that it made a positive difference. The results are here:

http://numsum.com/spreadsheet/show/23214

This is yet another argument for implementing proper shutdown of the application by external means.

Until then our options are to hack bugzilla again, or to add an absurd SS pref: browser.sessionstore.neverWriteToDisk :) Any other ideas?

> Taking this into account, it shouldn't be necessary to split the service in
> two. But if you still insist, at least keep both services in the same file --
> otherwise we additionally get duplicated code.

Wouldn't this have the same performance penalty that a single large component has?

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 224158 [details] [diff] [review]
patch #2 - split into 2 services redux


>+    var wType = window.document.documentElement.getAttribute("windowtype");
>+    if(wType == "navigator:browser") {

nit: space after if

>+  saveStateDelayed: function sss_saveStateDelayed(aWindow, aDelay) {

>+  saveState: function sss_saveState(aUpdateAll) {

>+  _clearDisk: function sss_clearDisk() {

why are these here now?


>+  /**
>+   * get session datafile (or its backup)
>+   * @returns nsIFile 
>+   */
>+  _getSessionFile: function sss_getSessionFile(aBackup) {
>+    return aBackup ? this._sessionFileBackup : this._sessionFile;
>+  },
>+
>+/* ........ Auxiliary Functions .............. */
>+
>+  /**
>+   * Whether or not to resume session, if not recovering from a crash
>+   * Returns true if:
>+   * - pref is set to always resume sessions
>+   * - pref is set to resume session once
>+   * - user configured startup page to be the last-visited page
>+   * @returns bool
>+   */
>+  _doResumeSession: function sss_doResumeSession() {
>+    return this._getPref("sessionstore.resume_session", DEFAULT_RESUME_SESSION)
>+      || this._getPref("sessionstore.resume_session_once", DEFAULT_RESUME_SESSION_ONCE)
>+      || this._getPref("startup.page", 1) == 2;
>+  },


really, this should be more like:

    return this._getPref("sessionstore.resume_session", DEFAULT_RESUME_SESSION) ||
           this._getPref("sessionstore.resume_session_once", DEFAULT_RESUME_SESSION_ONCE) ||
           this._getPref("startup.page", 1) == 2;


>+    }
>+    catch (ex) { dump(ex); } // if the prompt fails for some reason, recover anyway

on the same line as the brace, be consistent

>+  /**
>+   * Convenience method to get localized string bundles
>+   * @param aURI
>+   * @returns nsIStringBundle
>+   */
>+  _getStringBundle: function sss_getStringBundle(aURI) {
>+     var bundleService = Cc["@mozilla.org/intl/stringbundle;1"].
>+                           getService(Ci.nsIStringBundleService);
>+     var appLocale = Cc["@mozilla.org/intl/nslocaleservice;1"].
>+                       getService(Ci.nsILocaleService).getApplicationLocale();

fix the indenting to line up to match elsewhere

>+  /**
>+   * reads a file into a string
>+   * @param aFile
>+   *        nsIFile
>+   * @returns string
>+   */
>+  _readFile: function sss_readFile(aFile) {
>+    try {
>+      var stream = Cc["@mozilla.org/network/file-input-stream;1"].
>+                     createInstance(Ci.nsIFileInputStream);


consistent indenting

>+      stream.init(aFile, 0x01, 0, 0);
>+      var cvstream = Cc["@mozilla.org/intl/converter-input-stream;1"].
>+                       createInstance(Ci.nsIConverterInputStream);

and here

>+  /**
>+   * write file to disk
>+   * @param aFile
>+   *        nsIFile
>+   * @param aData
>+   *        String data
>+   */
>+  _writeFile: function sss_writeFile(aFile, aData) {
>+    // save the file in the current thread
>+    // (making sure we don't get killed at shutdown)
>+    (new FileWriter(aFile, aData)).run();
>+    return;
>+  },

can we trim this?

>+/* ........ QueryInterface .............. */
>+
>+  QueryInterface: function(aIID) {
>+    if (!aIID.equals(Ci.nsISupports) && !aIID.equals(Ci.nsIObserver)
>+       && !aIID.equals(Ci.nsISupportsWeakReference)
>+       && !aIID.equals(Ci.nsISessionStartup)) {

operators on the end of the previous line

>+
>+/* :::::::::: FileWriter :::::::::::::: */
>+
>+// a threaded file writer for somewhat better performance in the UI thread
>+function FileWriter(aFile, aData) {
>+  this._file = aFile;
>+  this._data = aData;
>+}
>+
>+FileWriter.prototype = {
>+  run: function FileWriter_run() {
>+    // init stream
>+    var stream = Cc["@mozilla.org/network/safe-file-output-stream;1"].
>+                   createInstance(Ci.nsIFileOutputStream);
>+    stream.init(this._file, 0x02 | 0x08 | 0x20, 0600, 0);
>+
>+    // convert to UTF-8
>+    var converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>+                      createInstance(Ci.nsIScriptableUnicodeConverter);
>+    converter.charset = "UTF-8";
>+    var convertedData = converter.ConvertFromUnicode(this._data);
>+    convertedData += converter.Finish();
>+
>+    // write and close stream
>+    stream.write(convertedData, convertedData.length);
>+    if (stream instanceof Ci.nsISafeOutputStream) {
>+      stream.finish();
>+    } else {
>+      stream.close();
>+    }
>+  },
>+};

pretty sure we don't need this either, right?

>+/* :::::::: Service Registration & Initialization ::::::::::::::: */

s/sessionstore/sessionstartup in this section, no?

>Index: browser/locales/en-US/chrome/browser/browser.properties
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v
>retrieving revision 1.26
>diff -u -8 -p -r1.26 browser.properties
>--- browser/locales/en-US/chrome/browser/browser.properties	16 May 2006 08:15:48 -0000	1.26
>+++ browser/locales/en-US/chrome/browser/browser.properties	1 Jun 2006 05:04:38 -0000
>@@ -114,8 +114,12 @@ updatesItem_pending=Apply Downloaded Upd
> updatesItem_pendingFallback=Apply Downloaded Update Now...
> 
> # RSS Pretty Print
> feedShowFeed=Add '%S' as Live Bookmark...
> feedHasFeeds=Add Live Bookmark...
> feedNoFeeds=Page has no feeds
> feedShowFeedNew=Subscribe to '%S'...
> feedHasFeedsNew=Subscribe to this page...
>+
>+# tab context menu additions
>+tabContext.undoCloseTab=Undo Close Tab
>+tabContext.undoCloseTabAccessKey=U

different patch :)

>Index: browser/locales/en-US/profile/localstore.rdf
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/profile/localstore.rdf,v
>retrieving revision 1.2
>diff -u -8 -p -r1.2 localstore.rdf
>--- browser/locales/en-US/profile/localstore.rdf	30 Nov 2004 21:26:12 -0000	1.2
>+++ browser/locales/en-US/profile/localstore.rdf	1 Jun 2006 05:04:38 -0000

careful diffing please (the joy of Mac!)

close, but I think I'll need to see another rev

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: patch #2 - split into 2 services + c16 fixes

Comments addressed...

(In reply to comment #16)
> (From update of attachment 224158 [details] [diff] [review] [edit])
> 
> >+    var wType = window.document.documentElement.getAttribute("windowtype");
> >+    if(wType == "navigator:browser") {
> 
> nit: space after if

fixed

> >+  saveStateDelayed: function sss_saveStateDelayed(aWindow, aDelay) {
> 
> >+  saveState: function sss_saveState(aUpdateAll) {
> 
> >+  _clearDisk: function sss_clearDisk() {
> 
> why are these here now?

removed

> >+  /**
> >+   * get session datafile (or its backup)
> >+   * @returns nsIFile 
> >+   */
> >+  _getSessionFile: function sss_getSessionFile(aBackup) {
> >+    return aBackup ? this._sessionFileBackup : this._sessionFile;
> >+  },
> >+
> >+/* ........ Auxiliary Functions .............. */
> >+
> >+  /**
> >+   * Whether or not to resume session, if not recovering from a crash
> >+   * Returns true if:
> >+   * - pref is set to always resume sessions
> >+   * - pref is set to resume session once
> >+   * - user configured startup page to be the last-visited page
> >+   * @returns bool
> >+   */
> >+  _doResumeSession: function sss_doResumeSession() {
> >+    return this._getPref("sessionstore.resume_session", DEFAULT_RESUME_SESSION)
> >+      || this._getPref("sessionstore.resume_session_once", DEFAULT_RESUME_SESSION_ONCE)
> >+      || this._getPref("startup.page", 1) == 2;
> >+  },
> 
> 
> really, this should be more like:
> 
>     return this._getPref("sessionstore.resume_session", DEFAULT_RESUME_SESSION)
> ||
>            this._getPref("sessionstore.resume_session_once",
> DEFAULT_RESUME_SESSION_ONCE) ||
>            this._getPref("startup.page", 1) == 2;

fixed

> >+    }
> >+    catch (ex) { dump(ex); } // if the prompt fails for some reason, recover anyway
> 
> on the same line as the brace, be consistent
 
not on same line is consistent with this code, so fixed the other inconsistencies

> >+  /**
> >+   * Convenience method to get localized string bundles
> >+   * @param aURI
> >+   * @returns nsIStringBundle
> >+   */
> >+  _getStringBundle: function sss_getStringBundle(aURI) {
> >+     var bundleService = Cc["@mozilla.org/intl/stringbundle;1"].
> >+                           getService(Ci.nsIStringBundleService);
> >+     var appLocale = Cc["@mozilla.org/intl/nslocaleservice;1"].
> >+                       getService(Ci.nsILocaleService).getApplicationLocale();
> 
> fix the indenting to line up to match elsewhere

Cc indenting fixed in both services
 
> >+  /**
> >+   * write file to disk
> >+   * @param aFile
> >+   *        nsIFile
> >+   * @param aData
> >+   *        String data
> >+   */
> >+  _writeFile: function sss_writeFile(aFile, aData) {
> >+    // save the file in the current thread
> >+    // (making sure we don't get killed at shutdown)
> >+    (new FileWriter(aFile, aData)).run();
> >+    return;
> >+  },
> 
> can we trim this?

removed, doing backup file only in nsSessionStore now.

> >+/* ........ QueryInterface .............. */
> >+
> >+  QueryInterface: function(aIID) {
> >+    if (!aIID.equals(Ci.nsISupports) && !aIID.equals(Ci.nsIObserver)
> >+       && !aIID.equals(Ci.nsISupportsWeakReference)
> >+       && !aIID.equals(Ci.nsISessionStartup)) {
> 
> operators on the end of the previous line

fixed in both services

> >+
> >+/* :::::::::: FileWriter :::::::::::::: */
> >+
> >+// a threaded file writer for somewhat better performance in the UI thread
> >+function FileWriter(aFile, aData) {
> >+  this._file = aFile;
> >+  this._data = aData;
> >+}
> >+
> >+FileWriter.prototype = {
> >+  run: function FileWriter_run() {
> >+    // init stream
> >+    var stream = Cc["@mozilla.org/network/safe-file-output-stream;1"].
> >+                   createInstance(Ci.nsIFileOutputStream);
> >+    stream.init(this._file, 0x02 | 0x08 | 0x20, 0600, 0);
> >+
> >+    // convert to UTF-8
> >+    var converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
> >+                      createInstance(Ci.nsIScriptableUnicodeConverter);
> >+    converter.charset = "UTF-8";
> >+    var convertedData = converter.ConvertFromUnicode(this._data);
> >+    convertedData += converter.Finish();
> >+
> >+    // write and close stream
> >+    stream.write(convertedData, convertedData.length);
> >+    if (stream instanceof Ci.nsISafeOutputStream) {
> >+      stream.finish();
> >+    } else {
> >+      stream.close();
> >+    }
> >+  },
> >+};
> 
> pretty sure we don't need this either, right?

gone

> >+/* :::::::: Service Registration & Initialization ::::::::::::::: */
> 
> s/sessionstore/sessionstartup in this section, no?
 
 fixed

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 224767 [details] [diff] [review]
patch #2 - split into 2 services + c16 fixes

ok, ship it (some style nits over IM wrt indentation, easily fixed)

Comment 19

[Dietrich Ayala (:dietrich)]

Attachment: Enable SS on trunk

Staggering the re-enabling of SS, to get baseline. This patch turns SS back on.

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: patch #2 - nits and tinder prob fixed

This patch fixes the indentation problems. I'm asking for review again because I've also added a fix for the tinderbox issue talked about in the previous comments. Basically, there's no reason to read the session file at all if the prefs are configured such that there are no circumstances under which restoration should occur:

resume_session=false
resume_session_once=false
resume_from_crash=false

This change is on line #336 of the patch.

Comment 21

[TimoT]

Attachment: adds nsSessionStartup.js to packages-static files

The new file needs to be added to packages-static files so that it gets included in the builds, right?

Comment 22

[Dietrich Ayala (:dietrich)]

(In reply to comment #21)
> Created an attachment (id=226035) [edit]
> adds nsSessionStartup.js to packages-static files
> 
> The new file needs to be added to packages-static files so that it gets
> included in the builds, right?
> 

thanks! i fixed the filename (nsSessionStartup.js) and checked-in.

Comment 23

[Dietrich Ayala (:dietrich)]

Comment on attachment 225158 [details] [diff] [review]
patch #2 - nits and tinder prob fixed

The results for this are inconclusive:

- Enabling session-restore as-is on the trunk did not cause a Ts increase on Pacifica, and nothing really on any of the Linux boxes either. Atlantia showed a distinct 3% increase.

- Splitting the service into 2 did not cause a Ts change on Pacifica. It didn't really affect the Linux boxes in a tangible way, nor Atlantia. However, as is clear, it did not show any detectable reduction in Ts on any of the boxes (not that there was anything to decrease on any box besides Atlantia).

I think that the fact that enabling session-restore did not increase Ts on Pacifica justifies syncing these changes to the branch, hopefully decreasing Ts there, and allowing undo-close-tab to land, etc. If necessary, we can keep this bug open to track the increase on Atlantia.

Comment 24

[Dietrich Ayala (:dietrich)]

This has landed on the branch (and is now active in release builds, after fixing the filenames in the package files). The result was the same as on the trunk: Windows and Linux builds are flat, and Atlantia went up about 20ms.

Leaving this open for the Atlantia Ts regression.

Comment 25

[Dietrich Ayala (:dietrich)]

(In reply to comment #24)
> This has landed on the branch (and is now active in release builds, after
> fixing the filenames in the package files). The result was the same as on the
> trunk: Windows and Linux builds are flat, and Atlantia went up about 20ms.
> 
> Leaving this open for the Atlantia Ts regression.
> 

Marking WONTFIX, as Atlantia is no longer online from what I can tell (last build on ftp.m.o was in october), and toggling Session Restore didn't affect Ts at all on Pacifica at the time.