stop excluding keys when calling JSON.stringify()

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Session Restore
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: dietrich, Assigned: zpao)

Tracking

(Blocks: 2 bugs, {hang, perf})

unspecified
Firefox 11
hang, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tsnap])

Attachments

(6 attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Blocks: 698514
(Reporter)

Comment 1

6 years ago
keys:

_tabStillLoading
_hosts
_formDataSaved
_shouldRestore
_host
_scheme

and sometimes __lastSessionWindowID
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Updated

6 years ago
Keywords: hang, perf
Whiteboard: [tsnap]
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)
Assignee: nobody → paul
(Reporter)

Updated

6 years ago
No longer blocks: 698514
(Reporter)

Updated

6 years ago
Blocks: 669034
(Reporter)

Comment 4

6 years ago
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.
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.
Created attachment 572620 [details] [diff] [review]
Patch 1 v0.1

1/6: Stop excluding keys.
Attachment #572620 - Flags: review?(dietrich)
Created attachment 572621 [details] [diff] [review]
Patch 2 v0.1

2/6: Move _tabStillLoading from data to the browser
Attachment #572621 - Flags: review?(dietrich)
Created attachment 572622 [details] [diff] [review]
Patch 3 v0.1

3/6: Move _formDataSaved from data to the browser
Attachment #572622 - Flags: review?(dietrich)
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.
Attachment #572623 - Flags: review?(dietrich)
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.
Attachment #572624 - Flags: review?(dietrich)
Created attachment 572625 [details] [diff] [review]
Patch 6 v0.1

6/6: Clear out lastSessionWindowID and shouldRestore when possible.
Attachment #572625 - Flags: review?(dietrich)
(Reporter)

Updated

6 years ago
Attachment #572620 - Flags: review?(dietrich) → review+
(Reporter)

Updated

6 years ago
Attachment #572621 - Flags: review?(dietrich) → review+
(Reporter)

Updated

6 years ago
Attachment #572622 - Flags: review?(dietrich) → review+
(Reporter)

Updated

6 years ago
Attachment #572623 - Flags: review?(dietrich) → review+
(Reporter)

Updated

6 years ago
Attachment #572624 - Flags: review?(dietrich) → review+
(Reporter)

Updated

6 years ago
Attachment #572625 - Flags: review?(dietrich) → review+
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
Whiteboard: [tsnap] → [tsnap][fixed-in-fx-team]
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [tsnap][fixed-in-fx-team] → [tsnap]
Target Milestone: --- → Firefox 11
Just for my clarification, this makes session restore files backwards-incompatible, right?
(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.
Depends on: 701481
Depends on: 702556

Comment 16

4 years ago
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.
Depends on: 822078

Updated

4 years ago
Blocks: 869900

Updated

4 years ago
Blocks: 886116
You need to log in before you can comment on or make changes to this bug.