698565 - stop excluding keys when calling JSON.stringify()

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Dietrich Ayala (:dietrich)]

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)]

keys:

_tabStillLoading
_hosts
_formDataSaved
_shouldRestore
_host
_scheme

and sometimes __lastSessionWindowID

Comment 2

[Dietrich Ayala (:dietrich)]

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 bugmail, email directly)]

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)]

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 bugmail, email directly)]

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 bugmail, email directly)]

Attachment: Patch 1 v0.1

1/6: Stop excluding keys.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch 2 v0.1

2/6: Move _tabStillLoading from data to the browser

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch 3 v0.1

3/6: Move _formDataSaved from data to the browser

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: 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 bugmail, email directly)]

Attachment: 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 bugmail, email directly)]

Attachment: Patch 6 v0.1

6/6: Clear out lastSessionWindowID and shouldRestore when possible.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

https://hg.mozilla.org/integration/fx-team/rev/8a62b79f6382
https://hg.mozilla.org/integration/fx-team/rev/e46a0677db7c
https://hg.mozilla.org/integration/fx-team/rev/0606e23bc306
https://hg.mozilla.org/integration/fx-team/rev/74df455a5125
https://hg.mozilla.org/integration/fx-team/rev/d57acad5beb4
https://hg.mozilla.org/integration/fx-team/rev/3b2109405ac7

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8a62b79f6382
https://hg.mozilla.org/mozilla-central/rev/e46a0677db7c
https://hg.mozilla.org/mozilla-central/rev/0606e23bc306
https://hg.mozilla.org/mozilla-central/rev/74df455a5125
https://hg.mozilla.org/mozilla-central/rev/d57acad5beb4
https://hg.mozilla.org/mozilla-central/rev/3b2109405ac7

Comment 14

[[e-mail]]

Just for my clarification, this makes session restore files backwards-incompatible, right?

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to [e-mail] 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]

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.