One of the main costs of session restore is serializing + sending data across threads. Bug 838577 and bug 867143 introduce a mechanism that lest us determine which parts of the session state has been modified since the last data collection. We can use this to only send the changed data to the I/O thread. This might complicate the code base but this has the potential to improve considerably the speed.

Looks like if we do this, we are going to destroy one add-on: https://addons.mozilla.org/en-US/firefox/addon/private-tab (see https://mxr.mozilla.org/addons/source/422756/bootstrap.js#932 )

Filed as https://github.com/Infocatcher/Private_Tab/issues/82

> Filed as https://github.com/Infocatcher/Private_Tab/issues/82 As I already replied on GitHub, I use "sessionstore-state-write" only to remove private tabs from session data (that tabs have associated nsILoadContext with usePrivateBrowsing == true). So, you can filter such tabs yourself or introduce some alternative API to modify session data without unwanted deserialization/serialization calls. Also about API. I see special handling for Scratchpad's windows, but don't see any similar API for extensions. In other words, we currently can save some tab- and browser window-related data, but what about some "global" data?

(In reply to Infocatcher from comment #3) > Also about API. I see special handling for Scratchpad's windows, but don't > see any similar API for extensions. > In other words, we currently can save some tab- and browser window-related > data, but what about some "global" data? You're right that this should be refactored and offered as an official API. Can you please file a separate bug for this?

(In reply to Tim Taubert [:ttaubert] from comment #4) > You're right that this should be refactored and offered as an official API. > Can you please file a separate bug for this? Done: https://bugzilla.mozilla.org/show_bug.cgi?id=899213 (My English isn't so good, sorry.)

Here's a first prototype. In the test case, this divides the size of message by ~10 after the first save.

Same one, but with a more meaningful test.

Why don't you add the new code to compress communications to _saveStateObject using stateString returned by observer?

(In reply to onemen.one from comment #8) > Why don't you add the new code to compress communications to > _saveStateObject using stateString returned by observer? Because doing this would require: 1. collect state; 2. stringify full state; 3. notify observers; 4. parse modified full state; 5. compress modified full state; 6. serialize/send compressed modified full state to worker. We have approximate benchmarks for operation 2. For ~4.5% of users (~22 million users), just this operation lasts at least 29 milliseconds on the main thread, so causes 2 frames to be skipped. Skipping 2., 3., 4. is much faster.

you are doing 1,5 and 6 anyhow. what about this: 1. collect state; 2. give temporary public access to the value of aStateObj; 3. notify observer with an option to cancel the save; 4. check if observer did not cancel the save; 5. compress modified (or not) full state; 6. serialize/send compressed modified full state to worker.

Your idea makes sense. However, to do it cleanly, we would need to do this with an event listener-style mechanism, rather than with an observer notification. I'd rather do that, if necessary, in a followup bug.

I'd rather evaluate if keeping the observer notification is necessary at all. So far we only have one add-on that does something we should actually do, remove private tabs. I'm very much in favor of throwing away old APIs and *maybe* replacing them by new, specialized ones.

I'm trying to improve Tabmix Session Manager and use SessionStore as much as i can. If stringify/parse have bad impact why don't let getBrowserState/getWindowState etc.. return object instead of string most of the time the caller have to parse the returned value any how.

(In reply to onemen.one from comment #13) > I'm trying to improve Tabmix Session Manager and use SessionStore as much as > i can. > > If stringify/parse have bad impact why don't let > getBrowserState/getWindowState etc.. return object instead of string > > most of the time the caller have to parse the returned value any how. Well, they don't do this for historical reasons. We are planning to provide better alternatives for getBrowserState, getWindowState, etc. that don't block the main thread (so, no JSON stringify/parse if possible, but also async implementation). We just haven't designed them yet.

(In reply to onemen.one from comment #13) > I'm trying to improve Tabmix Session Manager and use SessionStore as much as > i can. If you have any special API requirements, please file bugs. I'd still be in favor of removing the state-write notification. > If stringify/parse have bad impact why don't let > getBrowserState/getWindowState etc.. return object instead of string > most of the time the caller have to parse the returned value any how. Yes, I totally agree. Back when SessionStore was started we didn't have the possibility to return jsvals from IDL interfaces, strings were the only method to do that. Nowadays we indeed *can* return objects but we can't easily switch to it because we'd break lots of API users.

(In reply to Tim Taubert [:ttaubert] from comment #15) > (In reply to onemen.one from comment #13) > > > If stringify/parse have bad impact why don't let > > getBrowserState/getWindowState etc.. return object instead of string > > most of the time the caller have to parse the returned value any how. > > Yes, I totally agree. Back when SessionStore was started we didn't have the > possibility to return jsvals from IDL interfaces, strings were the only > method to do that. Nowadays we indeed *can* return objects but we can't > easily switch to it because we'd break lots of API users. maybe first step is to add getBrowserStateObj , getWindowStateObj without removing the current function also need to make sure that all the get___State will have an option to get an object

(In reply to onemen.one from comment #16) > maybe first step is to add getBrowserStateObj , getWindowStateObj without > removing the current function > also need to make sure that all the get___State will have an option to get > an object Can you give us a use case? Can you point us to an add-on that requires that functionality? I still haven't seen a valid point that convinces me we shouldn't remove the write-state notification. To be clear, even if we would implement getBrowserStateObj etc. we would definitely not return a shared object reference. That just opens a can of worms and is really really bad API design.

(In reply to Tim Taubert [:ttaubert] from comment #17) > Can you give us a use case? Can you point us to an add-on that requires that > functionality? I still haven't seen a valid point that convinces me we > shouldn't remove the write-state notification. It would be easier if the API have function to add extra data in the session level something like getSessionValue, setSessionValue, deleteSessionValue > To be clear, even if we would implement getBrowserStateObj etc. we would > definitely not return a shared object reference. That just opens a can of > worms and is really really bad API design. That's Ok Regarding this bug, why you only compress tabs data, what about _closedTabs and _closedWindows both this object most of the time don't change much between each save. one of the common action is closing tab(s), if you can some how use TabStateCache /tabInfos and pass the info to something like closedTabsCache

(In reply to onemen.one from comment #18) > It would be easier if the API have function to add extra data in the session > level > something like getSessionValue, setSessionValue, deleteSessionValue So you're saying that bug 899213 is what you want and we can get rid of the notification, right?

(In reply to Tim Taubert [:ttaubert] from comment #19) > (In reply to onemen.one from comment #18) > > It would be easier if the API have function to add extra data in the session > > level > > something like getSessionValue, setSessionValue, deleteSessionValue > > So you're saying that bug 899213 is what you want and we can get rid of the > notification, right? Yes, as long as this bug and bug 899213 will be implement in the same Firefox version.

Sure, that's the requirement. Basically we'll make sure the SessionValue (or whatever we'll call it) API will land first. Great.

(In reply to onemen.one from comment #18) > Regarding this bug, why you only compress tabs data, what about _closedTabs > and _closedWindows both this object most of the time don't change much > between each save. > one of the common action is closing tab(s), if you can some how use > TabStateCache /tabInfos and pass the info to something like closedTabsCache I don't remember that we save _closedTabs and _closedWindows in sessionstore.js, do we?

Yes, we do.

In that case, this sounds like a good idea. I will investigate it.

Do you think it is possible to split sessionstore.js into 2-3 files generalSessionData.js openedWindows.js closedWindows.js you only have to write data to the file that changed on startup you don't need to read all the closedWindows.js file at all

(In reply to onemen.one from comment #25) > Do you think it is possible to split sessionstore.js into 2-3 files > > generalSessionData.js > openedWindows.js > closedWindows.js > > you only have to write data to the file that changed > on startup you don't need to read all the closedWindows.js file at all That sounds quite possible, at a later stage. We have discussed a few manners of cutting the data in several files to avoid too much loading/writing, and this one is definitely a possibility.

Now that bug 930967 has landed, I will try and resume work on this bug. It is probably possible to reuse the data broadcasted by the child processes and send it immediately to the worker thread. I will investigate it. I suspect, however, that in a first version, we should keep the strategy of the above attachment. Any thought, smacleod, ttaubert?

Before doing something as invasive as the patch proposed here, do we have actual numbers on how much time we spend sending data around? It would be great to know beforehand whether it pays to invest time and complicate the code more.

Sorry, I meant http://telemetry.mozilla.org/#path=nightly/28/FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS + http://telemetry.mozilla.org/#path=nightly/28/FX_SESSION_RESTORE_SERIALIZE_DATA_MS The latter is the annoying one.

Ok, so JSON.stringify() is an overhead, as expected. Would we gain anything by getting rid of the "sessionstore-state-write" notification (what we're planning to do anyway) and sending the object to the worker and stringifying off the main thread? How would that affect WRITE_FILE_LONGEST_OP_MS?

Well, the whole plan was the following: 1. introduce some support for private tabs; 2. get rid of the sessionstore-state-write value (requires 1.); 3. optimize communications (requires 2.); By themselves, steps 1. and 2. will not improve performance noticeably.

(In reply to Tim Taubert [:ttaubert] from comment #31) > Ok, so JSON.stringify() is an overhead, as expected. Would we gain anything > by getting rid of the "sessionstore-state-write" notification (what we're > planning to do anyway) and sending the object to the worker and stringifying > off the main thread? How would that affect WRITE_FILE_LONGEST_OP_MS? Yeah, from looking at the Telemetry that was linked, it really seems like the bigger win would just be serializing off the main thread. Am I missing something?

(In reply to Steven MacLeod [:smacleod] from comment #33) > Yeah, from looking at the Telemetry that was linked, it really seems like > the bigger win would just be serializing off the main thread. Am I missing > something? Well, one thing: to send the data off the main thread, we need to serialize it first on the main thread. According to my benchmarks, whether we do this with JSON.stringify + TextEncoder or with postMessage doesn't change the results noticeably.

David, this sounds a lot like a meta bug. IIRC we require differential updates being sent to the worker in order to support a journaled storage. Should we file a new bug for that? Morph this one?

Meta bug sounds good.

Not working on this at the moment.

I'm seeing ~500ms worth of worker.postMessage() in a profile here, immediately after 1000ms of sessionStore.getCurrentState(). Maybe it would make sense to to move the promise resolution to the macro event loop?

I had a patch that did that but died of lack of review. I haven't managed to find it, though.