* How much space in sessionstore.js is devoted to DOM Session Storage? * How much space in sessionstore.js is devoted to closed tabs? * How much space in sessionstore.js is devoted to closed windows?

To determine any of these, we would need to produce and serialize the contents of sessionstore.js. The optimal way to do this would probably be to do this as part of the rewrite to optimize communications: - separate DOM Session Storage from the rest of tab stuff; - serialize separate DOM Session Storage, closed windows, closed tabs, open windows/tabs; - take the opportunity to compute the length of each of these.

Alternatively, we can during some idle time ask the SessionWorker to extract data from sessionstore.js

So, the strategy in comment 1 requires refactoring communications as part of bug 886447. The strategy in comment 2 requires parsing, rewriting, serializing data on the worker. Not very nice, but probably simpler.

This is one possible way to measure stuff. The idea is to send |extractStatistics| on idle-daily. Does this sounds ok to you, Tim?

Ok, here are "a few" more measures. Untested so far.

Comment on attachment 8338865 [details] [diff] [review] Additional telemetry for sessionstore.js Review of attachment 8338865 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionSaver.jsm @@ +331,5 @@ > }; > + > +// On the first gather-telemetry, extract the relevant statistics > +Services.obs.addObserver(function observer() { > + SessionSaverInternal._extractStatistics(); Instead of explicitly starting to collect and fill tab caches can we maybe just send a hint to the SessionWorker that we want it to return statistics after the next write has finished? ::: browser/components/sessionstore/src/SessionWorker.js @@ +134,5 @@ > + * for use with Telemetry. > + * > + * @return {object} > + */ > + extractStatistics: function (stateString) { I wonder if we should move this into a separate object, including the statistics gathering in the Agent seems confusing. @@ +213,5 @@ > + let result = { > + telemetry: { > + } > + }; > + let encoder = new TextEncoder(); We can just use the global "Encoder" defined at the top. @@ +214,5 @@ > + telemetry: { > + } > + }; > + let encoder = new TextEncoder(); > + for (let k in objects) { objects? Should that be global? And we should use (for let k of Object.keys(global)) here. @@ +221,5 @@ > + let string = JSON.stringify(obj); > + let encoded = encoder.encode(string); > + let stop = Date.now(); > + result.telemetry[TOTAL_PREFIX + k + SIZE_SUFFIX] = encoded.byteLength; > + result.telemetry[TOTAL_PREFIX + k + DURATION_SUFFIX] = stop - start; I don't understand why we need to measure the duration it takes to serialize a specific chunk of data. @@ +235,5 @@ > + size.push(encoded.byteLength); > + duration.push(stop - start); > + } > + result.telemetry[INDIVIDUAL_PREFIX + k + SIZE_SUFFIX] = size; > + result.telemetry[INDIVIDUAL_PREFIX + k + DURATION_SUFFIX] = duration; Same here, I don't think measuring the duration is valuable. @@ +240,5 @@ > + } > + } > + > + let stop = Date.now(); > + result.telemetry["FX_SESSIONRESTORE_EXTRACTING_STATISTICS_DURATION_MS"] = stop - start; Good idea to measure this. We surely want to know how much impact this has. Even if we end up ignoring because we need it, it's off main thread and only once per session or something?

(In reply to Tim Taubert [:ttaubert] from comment #6) > Comment on attachment 8338865 [details] [diff] [review] > Additional telemetry for sessionstore.js > > Review of attachment 8338865 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/sessionstore/src/SessionSaver.jsm > @@ +331,5 @@ > > }; > > + > > +// On the first gather-telemetry, extract the relevant statistics > > +Services.obs.addObserver(function observer() { > > + SessionSaverInternal._extractStatistics(); > > Instead of explicitly starting to collect and fill tab caches can we maybe > just send a hint to the SessionWorker that we want it to return statistics > after the next write has finished? I thought about that, but I believe that it's a bad idea. This extraction will probably keep the CPU busy for a few hundred milliseconds, plus will briefly eat up lots of memory, so we want to do this on idle. Since the session is basically never written on idle, we can't just piggyback on a call to write(). Plus I believe that gather-telemetry is sent only if telemetry is active, so we automatically save battery for users that don't have telemetry. > > ::: browser/components/sessionstore/src/SessionWorker.js > @@ +134,5 @@ > > + * for use with Telemetry. > > + * > > + * @return {object} > > + */ > > + extractStatistics: function (stateString) { > > I wonder if we should move this into a separate object, including the > statistics gathering in the Agent seems confusing. I guess we could, but I don't really see the point. > @@ +221,5 @@ > > + let string = JSON.stringify(obj); > > + let encoded = encoder.encode(string); > > + let stop = Date.now(); > > + result.telemetry[TOTAL_PREFIX + k + SIZE_SUFFIX] = encoded.byteLength; > > + result.telemetry[TOTAL_PREFIX + k + DURATION_SUFFIX] = stop - start; > > I don't understand why we need to measure the duration it takes to serialize > a specific chunk of data. I believed that this was an interesting measure, because serialization duration is not correlated only to the size of the result, but also to the complexity of the data. I won't insist, though. > @@ +240,5 @@ > > + } > > + } > > + > > + let stop = Date.now(); > > + result.telemetry["FX_SESSIONRESTORE_EXTRACTING_STATISTICS_DURATION_MS"] = stop - start; > > Good idea to measure this. We surely want to know how much impact this has. > Even if we end up ignoring because we need it, it's off main thread and only > once per session or something?

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #7) > > > +// On the first gather-telemetry, extract the relevant statistics > > > +Services.obs.addObserver(function observer() { > > > + SessionSaverInternal._extractStatistics(); > > > > Instead of explicitly starting to collect and fill tab caches can we maybe > > just send a hint to the SessionWorker that we want it to return statistics > > after the next write has finished? > > I thought about that, but I believe that it's a bad idea. This extraction > will probably keep the CPU busy for a few hundred milliseconds, plus will > briefly eat up lots of memory, so we want to do this on idle. Since the > session is basically never written on idle, we can't just piggyback on a > call to write(). I wasn't talking about doing this on every write but only the first write following a "gather-telemetry" notification. Whenever we receive that notification we should just set a flag in the worker. > Plus I believe that gather-telemetry is sent only if telemetry is active, so > we automatically save battery for users that don't have telemetry. Not a problem if the flag is only set whenever the notification is sent. > > > + extractStatistics: function (stateString) { > > > > I wonder if we should move this into a separate object, including the > > statistics gathering in the Agent seems confusing. > > I guess we could, but I don't really see the point. Well the point is to not have a huge unreadable method.

(In reply to Tim Taubert [:ttaubert] from comment #8) > I wasn't talking about doing this on every write but only the first write > following a "gather-telemetry" notification. Whenever we receive that > notification we should just set a flag in the worker. Indeed, but on the next write, we're not idle anymore, so the CPU burning will take place while the user is in front of the machine. We could adopt a sophisticated strategy, keeping the string in memory after a until the following idle, though, but that's more complicated and I don't think there is any gain. > > > > + extractStatistics: function (stateString) { > > > > > > I wonder if we should move this into a separate object, including the > > > statistics gathering in the Agent seems confusing. > > > > I guess we could, but I don't really see the point. > > Well the point is to not have a huge unreadable method. Ah, got it. Moving this to another object.

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #9) > (In reply to Tim Taubert [:ttaubert] from comment #8) > > I wasn't talking about doing this on every write but only the first write > > following a "gather-telemetry" notification. Whenever we receive that > > notification we should just set a flag in the worker. > > Indeed, but on the next write, we're not idle anymore, so the CPU burning > will take place while the user is in front of the machine. We could adopt a > sophisticated strategy, keeping the string in memory after a until the > following idle, though, but that's more complicated and I don't think there > is any gain. So on the one hand we're maybe introducing a little wait time off the main thread when writing. Which shouldn't really affect the user if there is no I/O? The way we would do it with your current patch would be to kick off an async data collection with all its messages and overhead. We would then send the whole string to the worker to let that do its things (why is that in the worker, just to be off main thread?) and then wait for the result. Doesn't the latter have a lot more overhead in terms of work and code? I'd really just like to keep it simple and the first method doesn't actually seem to have user-facing disadvantages afaict.

(In reply to Tim Taubert [:ttaubert] from comment #10) > The way we would do it with your current patch would be to kick off an async > data collection with all its messages and overhead. We would then send the > whole string to the worker to let that do its things (why is that in the > worker, just to be off main thread?) and then wait for the result. Yes, it's on the worker because we're serializing lots of stuff. I've tested the code on the main thread, for debugging purposes, and I see noticeable jank with my everyday profile. > Doesn't the latter have a lot more overhead in terms of work and code? I'd > really just like to keep it simple and the first method doesn't actually > seem to have user-facing disadvantages afaict. Well, my strategy involves more overhead, but when the user is not using the browser. I'm not going to insist, though.

Applied feedback (except the gather-telemetry stuff), added more measures, a simple test and a number of bugfixes. Oh, and I took the opportunity to improve SessionWorker error reporting.

Comment on attachment 8339317 [details] [diff] [review] Additional telemetry for sessionstore.js, v2 Review of attachment 8339317 [details] [diff] [review]: ----------------------------------------------------------------- Are you sure that such a huge list of measurements is useful? It doesn't seem like we could watch/gather all of those effectively... How could we ever (somewhat) quickly get an overview over the current state of sessionstore disk space usage when we have to wade through such a list of measurements? ::: browser/components/sessionstore/src/SessionFile.jsm @@ +226,5 @@ > SessionWorker.post("wipe"); > }, > > _recordTelemetry: function(telemetry) { > + for (let [id, value] of Iterator(telemetry)){ I was told not to use Iterator() anymore but rather Object.keys(). ::: browser/components/sessionstore/src/SessionSaver.jsm @@ +283,5 @@ > > + _gatherTelemetry: function() { // FIXME: Connect > + SessionStore.fillTabCachesAsynchronously().then(function() { > + let state = SessionStore.getCurrentState(false); > + return SessionFile.gatherTelemetry(state); So you're passing an object here? See bug 886447. @@ +330,5 @@ > } > }; > + > +// On the first gather-telemetry, extract the relevant statistics > +Services.obs.addObserver(function observer() { This doesn't feel like it should be long to the SessionSaver. Can we maybe move this all to SessionStore.observe()? ::: browser/components/sessionstore/src/SessionWorker.js @@ +135,5 @@ > + * > + * @return {object} > + */ > + gatherTelemetry: function (stateString) { > + return Statistics.collect(stateString); This will receive an object, not a state string. @@ +258,5 @@ > + let state = JSON.parse(stateString); > + > + // Gather all data > + let subsets = {}; > + for (let [k, obj] of Iterator(this.gatherSimpleData(state, subsets))) { Object.keys(). @@ +261,5 @@ > + let subsets = {}; > + for (let [k, obj] of Iterator(this.gatherSimpleData(state, subsets))) { > + subsets[k] = obj; > + } > + for (let [k, obj] of Iterator(this.gatherComplexData(state, subsets))) { Object.keys(). @@ +270,5 @@ > + let result = { > + telemetry: { > + } > + }; > + for (let [k, obj] of Iterator(subsets)) { Object.keys(). @@ +378,5 @@ > + walk: function(root, cb, acc) { > + if (!root || typeof root !== "object") { > + return; > + } > + for (let [k, obj] of Iterator(root)) { Object.keys().

(In reply to Tim Taubert [:ttaubert] from comment #13) > Comment on attachment 8339317 [details] [diff] [review] > Additional telemetry for sessionstore.js, v2 > > Review of attachment 8339317 [details] [diff] [review]: > ----------------------------------------------------------------- > > Are you sure that such a huge list of measurements is useful? It doesn't > seem like we could watch/gather all of those effectively... How could we > ever (somewhat) quickly get an overview over the current state of > sessionstore disk space usage when we have to wade through such a list of > measurements? This is a good question. At the moment, the problem is that we don't know what to optimize. Having these statistics, even for one week, would at least lead us in the right direction. We could deactivate the statistics once we have this sample. > > ::: browser/components/sessionstore/src/SessionFile.jsm > @@ +226,5 @@ > > SessionWorker.post("wipe"); > > }, > > > > _recordTelemetry: function(telemetry) { > > + for (let [id, value] of Iterator(telemetry)){ > > I was told not to use Iterator() anymore but rather Object.keys(). Ok. > ::: browser/components/sessionstore/src/SessionSaver.jsm > @@ +283,5 @@ > > > > + _gatherTelemetry: function() { // FIXME: Connect > > + SessionStore.fillTabCachesAsynchronously().then(function() { > > + let state = SessionStore.getCurrentState(false); > > + return SessionFile.gatherTelemetry(state); > > So you're passing an object here? See bug 886447. Ah, oops, I meant getBrowserState(), as in my test. > > @@ +330,5 @@ > > } > > }; > > + > > +// On the first gather-telemetry, extract the relevant statistics > > +Services.obs.addObserver(function observer() { > > This doesn't feel like it should be long to the SessionSaver. Can we maybe > move this all to SessionStore.observe()? Good idea.

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #14) > (In reply to Tim Taubert [:ttaubert] from comment #13) > > Comment on attachment 8339317 [details] [diff] [review] > > Additional telemetry for sessionstore.js, v2 > > > > Review of attachment 8339317 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Are you sure that such a huge list of measurements is useful? It doesn't > > seem like we could watch/gather all of those effectively... How could we > > ever (somewhat) quickly get an overview over the current state of > > sessionstore disk space usage when we have to wade through such a list of > > measurements? > > This is a good question. At the moment, the problem is that we don't know > what to optimize. Having these statistics, even for one week, would at least > lead us in the right direction. We could deactivate the statistics once we > have this sample. We can try and collect less data, though. We can definitely do without HISTORY_OWNER and POSTDATA. Do any others seem superfluous to you?

I wonder if a better approach would be to start small and just measure open windows, open tabs, closed windows and closed tabs? If any of those sticks out we can then dig deeper and measure that more accurately?

Same stuff, less measurements.

Comment on attachment 8346614 [details] [diff] [review] Additional telemetry for sessionstore.js, v3 Review of attachment 8346614 [details] [diff] [review]: ----------------------------------------------------------------- Any particular reason you are not using ES6 generators (i.e. old syntax)? I found two instances: ::: browser/components/sessionstore/src/SessionFile.jsm @@ +163,5 @@ > }); > }, > > + gatherTelemetry: function(aStateString) { > + return Task.spawn(function() { see above ::: browser/components/sessionstore/test/browser_telemetry.js @@ +4,5 @@ > + > +/** > + * Test that Telemetry collection doesn't cause any error. > + */ > +add_task(function() { see above

Comment on attachment 8346614 [details] [diff] [review] Additional telemetry for sessionstore.js, v3 Review of attachment 8346614 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionFile.jsm @@ +226,5 @@ > SessionWorker.post("wipe"); > }, > > _recordTelemetry: function(telemetry) { > + for (let [id, value] of Iterator(telemetry)){ Please use Object.keys(). Iterator is deprecated. ::: browser/components/sessionstore/src/SessionWorker.js @@ +257,5 @@ > + let state = JSON.parse(stateString); > + > + // Gather all data > + let subsets = {}; > + for (let [k, obj] of Iterator(this.gatherSimpleData(state, subsets))) { Iterator is deprecated and gatherSimpleData() doesn't take a second argument. @@ +260,5 @@ > + let subsets = {}; > + for (let [k, obj] of Iterator(this.gatherSimpleData(state, subsets))) { > + subsets[k] = obj; > + } > + for (let [k, obj] of Iterator(this.gatherComplexData(state, subsets))) { Iterator is deprecated and gatherComplexData() doesn't take a second argument. @@ +269,5 @@ > + let result = { > + telemetry: { > + } > + }; > + for (let [k, obj] of Iterator(subsets)) { Iterator is deprecated. @@ +281,5 @@ > + let string = JSON.stringify(item); > + let encoded = Encoder.encode(string); > + size.push(encoded.byteLength); > + } > + result.telemetry[INDIVIDUAL_PREFIX + k + SIZE_SUFFIX] = size; Use map() here maybe? @@ +287,5 @@ > + } > + > + let stop = Date.now(); > + result.telemetry["FX_SESSION_RESTORE_EXTRACTING_STATISTICS_DURATION_MS"] = stop - start; > + return result; Maybe |return {telemetry: result}|, I think that would make code above a little more readable. @@ +295,5 @@ > + /** > + * Collect data that doesn't require a recursive walk through the > + * data structure. > + */ > + gatherSimpleData: function(state) { If you passed an object to this function we wouldn't have to merge its return value. @@ +304,5 @@ > + // The subset of sessionstore.js dealing with open windows > + OPEN_WINDOWS: null, > + > + // The subset of sessionstore.js dealing with closed windows > + CLOSED_WINDOWS: null, Can't you just init those fields with their respective values right here? @@ +335,5 @@ > + * @param {object} root The object from which to start walking. > + * @param {function(key, value, acc)} cb Callback, called for each > + * item except the root. Returns the new value of the accumulator > + * passed recursively to the children, or |null| to not walk the > + * subtree rooted at |value|. Returns a boolean. @@ +341,5 @@ > + */ > + walk: function(root, cb, acc) { > + if (!root || typeof root !== "object") { > + return; > + } |typeof root !== "object"| should suffice here. @@ +342,5 @@ > + walk: function(root, cb, acc) { > + if (!root || typeof root !== "object") { > + return; > + } > + for (let [k, obj] of Iterator(root)) { Iterator is deprecated. @@ +343,5 @@ > + if (!root || typeof root !== "object") { > + return; > + } > + for (let [k, obj] of Iterator(root)) { > + let acc2 = cb(k, obj, acc); |acc| passed to |cb| doesn't seem to be used. @@ +344,5 @@ > + return; > + } > + for (let [k, obj] of Iterator(root)) { > + let acc2 = cb(k, obj, acc); > + if (acc2 !== null) { Shouldn't this check if cb() returned true? And acc2 should probably have a better name, if we need that variable at all. @@ +353,5 @@ > + > + /** > + * Collect data that requires walking through the data structure > + */ > + gatherComplexData: function(state) { If you passed an object to this function we wouldn't have to merge its return value. ::: browser/components/sessionstore/test/browser_telemetry.js @@ +5,5 @@ > +/** > + * Test that Telemetry collection doesn't cause any error. > + */ > +add_task(function() { > + let tab = gBrowser.addTab("http://example.org:80/"); We should maybe wait until the tab has loaded? @@ +14,5 @@ > + info("Collecting session data"); > + let state = ss.getBrowserState(); > + > + info("Collecting statistics"); > + let statistics = yield SessionFile.gatherTelemetry(state); Should we check some properties that are expected?

Here we go, with more tests. As discussed over IRC, there are no tests for POSTDATA yet, hum, keeping in line with the rest of Session Restore and posibly bug 952092.

(In reply to Florian Bender from comment #18) > Any particular reason you are not using ES6 generators (i.e. old syntax)? I > found two instances: No particular reason other than that we have tons of code using the old syntax currently. And it doesn't make a big difference whether the script someone's going to write has to fix two instances more or less :) But thanks for bringing that to our attention, I should start picking this up for future reviews.

Comment on attachment 8350053 [details] [diff] [review] Additional telemetry for sessionstore.js, v4 Review of attachment 8350053 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: browser/components/sessionstore/src/SessionFile.jsm @@ +247,5 @@ > + histogram.add(sample); > + } > + } catch (ex) { > + Cu.reportError("Error while updating histogram " + id); > + Cu.reportError(ex); Do we really need that try-catch here? We would fail when running the tests if one of the Histograms was not specified. This seems overkill to me. ::: browser/components/sessionstore/src/SessionStore.jsm @@ +1437,5 @@ > + // On the first gather-telemetry notification of the session, > + // gather telemetry data. > + Services.obs.removeObserver(this, "gather-telemetry"); > + this.fillTabCachesAsynchronously().then(function() { > + let stateString = SessionStore.getBrowserState(); I wonder, if gather-telemetry is only called when we're idling, wouldn't it be better to finish gathering ASAP and thus not to use fillTabCachesAsynchronously()? ::: browser/components/sessionstore/src/SessionWorker.js @@ +275,5 @@ > + if (Array.isArray(obj)) { > + let size = obj.map(function(item) { > + let string = JSON.stringify(item); > + return Encoder.encode(string).byteLength; > + }); You could define a small helper function like function getByteLength(str) { return Encoder.encode(JSON.stringify(str)).byteLength; } And use it here (and above) to have: let size = obj.map(getByteLength); ::: browser/components/sessionstore/test/browser_telemetry.js @@ +5,5 @@ > +let tmp = {}; > +Cu.import("resource:///modules/sessionstore/SessionFile.jsm", tmp); > +Cu.import("resource:///modules/sessionstore/TabStateCache.jsm", tmp); > +Cu.import("resource://gre/modules/Promise.jsm", tmp); > +let {SessionFile, TabStateCache, Promise} = tmp; Nit: I think Promise should be defined already. @@ +42,5 @@ > + tab.linkedBrowser.contentWindow.history.pushState({foo:1}, "ref"); > + SyncHandlers.get(tab.linkedBrowser).flush(); > + let statistics2 = yield promiseStats(); > + > +// We have changed history, so it must have increased nit: please align properly @@ +46,5 @@ > +// We have changed history, so it must have increased > + ok(KEY in statistics, "Key is defined"); > + ok(statistics2[KEY] > statistics[KEY], "The total size of HISTORY has increased"); > + > +// Almost nothing else should nit: please align properly @@ +76,5 @@ > + > + ok(KEY in statistics, "Key is defined"); > + ok(statistics2[KEY] > statistics[KEY], "The total size of CLOSED_TABS_IN_OPEN_WINDOWS has increased"); > + > +// Almost nothing else should change Nit: align please (and the rest of the comments in this file)

(In reply to Tim Taubert [:ttaubert] from comment #22) > ::: browser/components/sessionstore/src/SessionStore.jsm > @@ +1437,5 @@ > > + // On the first gather-telemetry notification of the session, > > + // gather telemetry data. > > + Services.obs.removeObserver(this, "gather-telemetry"); > > + this.fillTabCachesAsynchronously().then(function() { > > + let stateString = SessionStore.getBrowserState(); > > I wonder, if gather-telemetry is only called when we're idling, wouldn't it > be better to finish gathering ASAP and thus not to use > fillTabCachesAsynchronously()? Would it change anything?

(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #23) > (In reply to Tim Taubert [:ttaubert] from comment #22) > > I wonder, if gather-telemetry is only called when we're idling, wouldn't it > > be better to finish gathering ASAP and thus not to use > > fillTabCachesAsynchronously()? > > Would it change anything? Not a lot but .fillTabCachesAsynchronously() can in theory fail or time out. Gathering data synchronously is probably not that bad here. If the machine is idle we can assume most (all?) of the caches to be filled anyway.

Same one, with additional cleanup at the start of the test.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b3c62f7c0128

This is bitrotted. Please rebase against fx-team tip.

Same one, unbitrotten.

Try: https://tbpl.mozilla.org/?tree=Try&rev=c8b65f1018f4

Try: https://tbpl.mozilla.org/?tree=Try&rev=a35d438c0cd8