Last Comment Bug 698565 - stop excluding keys when calling JSON.stringify()
: stop excluding keys when calling JSON.stringify()
Status: RESOLVED FIXED
[tsnap]
: hang, perf
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: Firefox 11
Assigned To: Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
:
:
Mentors:
Depends on: 701481 702556 822078
Blocks: sessionRestoreJank 869900 886116
  Show dependency treegraph
 
Reported: 2011-10-31 13:27 PDT by Dietrich Ayala (:dietrich)
Modified: 2013-06-23 06:17 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 v0.1 (2.32 KB, patch)
2011-11-07 14:26 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review
Patch 2 v0.1 (6.54 KB, patch)
2011-11-07 14:27 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review
Patch 3 v0.1 (4.00 KB, patch)
2011-11-07 14:28 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review
Patch 4 v0.1 (10.68 KB, patch)
2011-11-07 14:31 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review
Patch 5 v0.1 (10.26 KB, patch)
2011-11-07 14:35 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review
Patch 6 v0.1 (1.93 KB, patch)
2011-11-07 14:36 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review

Description Dietrich Ayala (:dietrich) 2011-10-31 13:27:09 PDT
can take 4x longer to stringify for large sessions.

https://bugzilla.mozilla.org/show_bug.cgi?id=698514#c1

need to figure out:

* which keys need to be removed manually

* if there are keys we can hang off tabs/windows/etc instead of the data objects

* if there are keys we can safely live with unnecessarily writing and restoring
Comment 1 Dietrich Ayala (:dietrich) 2011-10-31 15:07:18 PDT
keys:

_tabStillLoading
_hosts
_formDataSaved
_shouldRestore
_host
_scheme

and sometimes __lastSessionWindowID
Comment 2 Dietrich Ayala (:dietrich) 2011-10-31 21:50:55 PDT
Further testing shows this makes a massive difference. In a test session of 3.9mb worth of session data (due to a totally f'd up Facebook page), I was seeing hangs for 5-10 seconds.

Disabling the key exclusion made the hangs all but disappear.
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-01 10:54:44 PDT
Ok, I'll make this a priority.

I think we're probably ok to just keep those keys & associated data in the string we output. I'll need to check, but we ignore most, if not all, of those when reading data already. For those that we don't ignore, we can start ignoring them.

(In reply to Dietrich Ayala (:dietrich) from comment #1)
> _tabStillLoading
OK - We set this on restore for each tab to short-circuit data collection, but it gets wiped once we save state properly - could hang on tab/browser.

> _hosts
OK - We never read this - it gets reset fairly often - could hang on window.

> _formDataSaved
OK - Only used to short-circuit, never read - could hang on tab/browser.

> _shouldRestore
OK - Never read - can't hang it on a window (it's used for closed window data)

> _host
OK - Not explicitly read, but if we don't finish restoring, might be used (but in that case it would be accurate) - with some work, it could hang on tab/browser (it's 1 per shistory entry, but could push an array up)

> _scheme
OK - same as _host


> and sometimes __lastSessionWindowID

OK (I think) - it's only used during deferred session restore (but would get reset then anyway). Even if it's not reset, I think it's ok. - can't hang on a window (used for association with unrestored data)
Comment 4 Dietrich Ayala (:dietrich) 2011-11-03 15:52:09 PDT
I was overly optimistic. This shows some improvement, but in my pathologically-big sessionstore.js, there are still sessionstore-related hangs due to other issues.

Absolutely worth doing, just not as complete a culprit as I'd thought.
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 14:20:41 PST
Patches coming (a performance in 6 parts), but here's the breakdown...

(In reply to Paul O'Shannessy [:zpao] from comment #3)
> > _tabStillLoading
> OK - We set this on restore for each tab to short-circuit data collection,
> but it gets wiped once we save state properly - could hang on tab/browser.

Hung this on the tab.

> > _hosts
> OK - We never read this - it gets reset fairly often - could hang on window.

Stored in a separate object now

> > _formDataSaved
> OK - Only used to short-circuit, never read - could hang on tab/browser.

Hung this on the browser

> > _shouldRestore
> OK - Never read - can't hang it on a window (it's used for closed window
> data)

This will now be filtered out when quitting (as soon as it's used in the quitting process, it will get deleted so it won't end up on disk after a clean quit)

> > _host
> OK - Not explicitly read, but if we don't finish restoring, might be used
> (but in that case it would be accurate) - with some work, it could hang on
> tab/browser (it's 1 per shistory entry, but could push an array up)
> 
> > _scheme
> OK - same as _host

Ohai bug 690992. I changed this up a bit & I'm hanging the data on the browser now.


> > and sometimes __lastSessionWindowID
> 
> OK (I think) - it's only used during deferred session restore (but would get
> reset then anyway). Even if it's not reset, I think it's ok. - can't hang on
> a window (used for association with unrestored data)

It's ok to leave this in since the values on disk will be overwritten in the case where it's relevant. During a restore-at-startup process, these will get deleted to prevent unnecessary perpetuation on disk.
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 14:26:04 PST
Created attachment 572620 [details] [diff] [review]
Patch 1 v0.1

1/6: Stop excluding keys.
Comment 7 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 14:27:16 PST
Created attachment 572621 [details] [diff] [review]
Patch 2 v0.1

2/6: Move _tabStillLoading from data to the browser
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 14:28:30 PST
Created attachment 572622 [details] [diff] [review]
Patch 3 v0.1

3/6: Move _formDataSaved from data to the browser
Comment 9 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 14:31:03 PST
Created attachment 572623 [details] [diff] [review]
Patch 4 v0.1

4/6: Move _hosts from data to a new internal data object
I didn't attach to each window because I didn't want to constantly be looking up a window by it's _SSi. That would have required more in-depth changes that aren't really necessary.
Comment 10 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 14:35:16 PST
Created attachment 572624 [details] [diff] [review]
Patch 5 v0.1

5/6: Move _hosts & _scheme out of each entry and onto the browser.
This also involved a bit of rethinking how we do this. Now instead of having to look at each entry (often recursively), we'll push all the host data into a single array on each browser. During the session we only have to iterate over this flat structure.
Comment 11 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 14:36:42 PST
Created attachment 572625 [details] [diff] [review]
Patch 6 v0.1

6/6: Clear out lastSessionWindowID and shouldRestore when possible.
Comment 14 neil@parkwaycc.co.uk 2011-11-09 03:23:27 PST
Just for my clarification, this makes session restore files backwards-incompatible, right?
Comment 15 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-09 10:22:35 PST
(In reply to neil@parkwaycc.co.uk from comment #14)
> Just for my clarification, this makes session restore files
> backwards-incompatible, right?

Incorrect. These things haven't been stored on disk for a while and for the most part still aren't. Even for the 2 things that might end up on disk now (_shouldRestore and _lastSessionWindowID) they aren't used in the startup process. So new files should load up just fine in Firefox 4 and visa versa.
Comment 16 ithinc 2012-12-15 22:54:10 PST
Comment on attachment 572621 [details] [diff] [review]
Patch 2 v0.1

Review of attachment 572621 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1701,5 @@
>      
>      if (!browser || !browser.currentURI)
>        // can happen when calling this function right after .addTab()
>        return tabData;
> +    else if (browser.__SS_data && browser.__SS_tabStillLoading) {

No need to test browser.__SS_data now.

Note You need to log in before you can comment on or make changes to this bug.