We have a pref called browser.sessionstore.dom_storage_limit that currently defaults to 10M. For localStorage however the default is 5M, so that doesn't seem to make a lot of sense. And this limit doesn't apply to partial sessionStorage updates at all, which probably completely bypasses the limit. We should set a small limit, let's say 2kB, and shouldn't store/serialize DOMSessionStorage data beyond that. This will help with Google search pages spamming the file. I can't imagine there being a lot of value for huge sessionStorage entries being persisted to disk. If a game stores textures they'll simply fetch them again, and search pages can preload next time too. This should severely reduce I/O and make sessionstore.js a lot smaller. Also we will spend less time serializing hundreds of kilobytes of data.

Can I just make this a P1? I think it should be. Let's see.

We'll also switch to using the API I introduced in bug 1361974.

Basically, I expect: * FX_SESSION_RESTORE_FILE_SIZE_BYTES * FX_SESSION_RESTORE_READ_FILE_MS * FX_SESSION_RESTORE_SERIALIZE_DATA_MS * FX_SESSION_RESTORE_WRITE_FILE_MS to decrease. It might unfortunately take a while until this applies to existing sessions. If a tab isn't loaded, the limit won't apply to its DOMSessionStorage data. As soon as a tab is loaded however, we'll restore any previously collected DOMSessionStorage data and then check against the limit before writing it back to disk.

Comment on attachment 8864542 [details] [diff] [review] 0001-Bug-1362058-Further-limit-the-amount-of-sessionStora.patch Review of attachment 8864542 [details] [diff] [review]: ----------------------------------------------------------------- Nice! I'm I was following along with the DWU changes you made (new storage usage retrieval API). As a very-much aside: have you thought about using MozReview in the future, perhaps? I don't care either way, but it makes your future landings a bit easier, perhaps. ::: browser/components/sessionstore/SessionStorage.jsm @@ +184,5 @@ > storage = null; > } > > + if (!storage || !storage.length) { > + return {}; nit: or just `return hostData;`? (Same in the early return below). @@ +192,5 @@ > + let usage = window.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils) > + .getStorageUsage(storage); > + Services.telemetry.getHistogramById("FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS").add(usage); > + if (usage > Preferences.get(DOM_STORAGE_LIMIT_PREF)) { Why import Preferences.jsm when you already have `Services.prefs` at your disposal? You're not taking advantage of its default value or observer features, so I don't necessarily see an advantage to using it here.

(In reply to Mike de Boer [:mikedeboer] from comment #5) > As a very-much aside: have you thought about using MozReview in the future, > perhaps? I don't care either way, but it makes your future landings a bit > easier, perhaps. Yes, yes, I have :) I was always a little too lazy to set it up. But I should. > ::: browser/components/sessionstore/SessionStorage.jsm > @@ +184,5 @@ > > storage = null; > > } > > > > + if (!storage || !storage.length) { > > + return {}; > > nit: or just `return hostData;`? (Same in the early return below). Sure, will do. > @@ +192,5 @@ > > + let usage = window.QueryInterface(Ci.nsIInterfaceRequestor) > > + .getInterface(Ci.nsIDOMWindowUtils) > > + .getStorageUsage(storage); > > + Services.telemetry.getHistogramById("FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS").add(usage); > > + if (usage > Preferences.get(DOM_STORAGE_LIMIT_PREF)) { > > Why import Preferences.jsm when you already have `Services.prefs` at your > disposal? You're not taking advantage of its default value or observer > features, so I don't necessarily see an advantage to using it here. Fair point, will fix.

Pushed by ttaubert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/262e008b0bc4 Further limit the amount of sessionStorage data we store and serialize r=mikedeboer

Comment on attachment 8873393 [details] [diff] [review] 0002-Bug-1362058-Update-FX_SESSION_RESTORE_DOM_STORAGE_SI.patch Review of attachment 8873393 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Remember to file a follow-up bug for the removal/expiry extension of FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS in 60.

Pushed by ttaubert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cf72d18df9 Update FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS histogram metadata r=chutten