886447 - [Session Restore] Optimize communications [meta]

Status: RESOLVED FIXED

(Firefox :: Session Restore, task)

Description

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

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 )

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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

Comment 3

[Infocatcher]

> 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?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 5

[Infocatcher]

(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.)

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Compressing communications

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

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Compressing communications, v2

Same one, but with a more meaningful test.

Comment 8

[tabmix.onemen]

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

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 10

[tabmix.onemen]

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.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 13

[tabmix.onemen]

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.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 16

[tabmix.onemen]

(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

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 18

[tabmix.onemen]

(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

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 20

[tabmix.onemen]

(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.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 22

[David Teller [:Yoric] - still alive but not very active]

(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?

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Yes, we do.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

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

Comment 25

[tabmix.onemen]

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

Comment 26

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

http://telemetry.mozilla.org/#path=nightly/28/FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS

Comment 30

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 32

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 33

[Steven MacLeod [:smacleod]]

(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?

Comment 34

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Meta bug sounds good.

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Not working on this at the moment.

Comment 38

[The 8472]

Attachment: postMessage.png

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?

Comment 39

[David Teller [:Yoric] - still alive but not very active]

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

Comment 40

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Hey sclements, looks like all blocking bugs for this meta are closed. Think it's safe to close this one out as done?

Comment 42

[Sarah Clements [:sclements]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #41)

Hey sclements, looks like all blocking bugs for this meta are closed. Think it's safe to close this one out as done?

Sorry for the delay, yes I'll mark it as fixed.