Session-restore service causes Ts regression

RESOLVED WONTFIX

Status

()

enhancement
RESOLVED WONTFIX
13 years ago
13 years ago

People

(Reporter: dietrich, Assigned: dietrich)

Tracking

2.0 Branch
Firefox 2 beta1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [swag: 1d], )

Attachments

(6 attachments, 4 obsolete attachments)

Assignee

Description

13 years ago
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.
Assignee

Comment 1

13 years ago
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
Assignee: nobody → dietrich
Status: NEW → ASSIGNED
Attachment #221497 - Flags: review?(mconnor)
Attachment #221497 - Flags: approval-branch-1.8.1?(mconnor)

Comment 2

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

Comment 3

13 years ago
Attachment #221497 - Attachment is obsolete: true
Attachment #221504 - Flags: review?(mconnor)
Attachment #221504 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #221497 - Flags: review?(mconnor)
Attachment #221497 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #221504 - Flags: review?(mconnor)
Attachment #221504 - Flags: review+
Attachment #221504 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #221504 - Flags: approval-branch-1.8.1+
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.
I filed bug 337512 to investigate JS component loading perf a tad.
Assignee

Comment 7

13 years ago
Attachment #222637 - Flags: review?(mconnor)
Assignee

Comment 8

13 years ago
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.
Attachment #222711 - Flags: review?(mconnor)
Attachment #222637 - Flags: review?(mconnor) → review+
Assignee

Updated

13 years ago
Whiteboard: [swag: 1d]
Assignee

Comment 9

13 years ago
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.
Attachment #222711 - Attachment is obsolete: true
Attachment #224158 - Flags: review?(mconnor)
Attachment #222711 - Flags: review?(mconnor)

Comment 10

13 years ago
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.
Assignee

Comment 11

13 years ago
Steffen,

This patch re-enables the service. I believe the requirement was to mitigate the TS regression prior to re-enabling.
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?
Assignee

Comment 13

13 years ago
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

13 years ago
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.
Assignee

Comment 15

13 years ago

(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 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
Attachment #224158 - Flags: review?(mconnor) → review-
Assignee

Comment 17

13 years ago
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
Attachment #224158 - Attachment is obsolete: true
Attachment #224767 - Flags: review?(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)
Attachment #224767 - Flags: review?(mconnor) → review+
Assignee

Comment 19

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

Comment 20

13 years ago
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.
Attachment #224767 - Attachment is obsolete: true
Attachment #225158 - Flags: review?(mconnor)

Updated

13 years ago
Blocks: 328154
Attachment #225158 - Flags: review?(mconnor) → review+

Comment 21

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

Comment 22

13 years ago
(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.
Assignee

Comment 23

13 years ago
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.
Attachment #225158 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #225158 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
No longer depends on: 342039
Depends on: 342039
Assignee

Comment 24

13 years ago
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.
Assignee

Comment 25

13 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.