894969 - [SessionStore] Cache generated JSON

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

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

Bug 867143 introduces a mechanism that lets us cache tab data to avoid re-collecting data. The logical next step is to also cache pre-serialized data to avoid having to re-serialize all the time.

Comment 1

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

Attachment: Caching generated JSON

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 777194 [details] [diff] [review]
Caching generated JSON

Review of attachment 777194 [details] [diff] [review]:
-----------------------------------------------------------------

Neat idea. The stringify() and parse() methods could be moved to their own object, like JSONCache maybe?

I only see one problem: with the patch from bug 867143 we call _updateTextAndScrollDataForTab() before returning cached TabData. This does actually invalidate the JSON cache every time the cache is accessed. This patch in its current version would lose scroll and text information that has changed in the meantime.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +4380,5 @@
> +    return JSON.stringify(obj, function replacer(key, value) {
> +      if (value instanceof TabData) {
> +        let json = value._cachedJSON;
> +        if (!json) {
> +          value._cachedJSON = json = JSON.stringify(value);

The problem is that _cachedJSON would be serialized as well, right? I think we can omit this by using Name() to create a private name object.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> I only see one problem: with the patch from bug 867143 we call
> _updateTextAndScrollDataForTab() before returning cached TabData. This does
> actually invalidate the JSON cache every time the cache is accessed. This
> patch in its current version would lose scroll and text information that has
> changed in the meantime.

Sorry, I got that wrong. We call _updateTextAndScrollDataForTab() only once before we put data into the cache. Why is that even a separate function? Argh.

Comment 4

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

(In reply to Tim Taubert [:ttaubert] from comment #3)
> Sorry, I got that wrong. We call _updateTextAndScrollDataForTab() only once
> before we put data into the cache. Why is that even a separate function?
> Argh.

Indeed, we call it only when either putting data into the cache or when computing data that we're not going to cache at all.

Well, I've kept it as a separate function despite the fact that it's always called after collectBaseTabData simply because these two functions are large enough and I didn't want to make them even larger.

(In reply to Tim Taubert [:ttaubert] from comment #2)
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +4380,5 @@
> > +    return JSON.stringify(obj, function replacer(key, value) {
> > +      if (value instanceof TabData) {
> > +        let json = value._cachedJSON;
> > +        if (!json) {
> > +          value._cachedJSON = json = JSON.stringify(value);
> 
> The problem is that _cachedJSON would be serialized as well, right? I think
> we can omit this by using Name() to create a private name object.

I have just set it to |undefined| instead of |null|, this should prevent it from being serialized.

Comment 5

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

Attachment: Caching generated JSON, v2

Oh, I forgot.

> Neat idea. The stringify() and parse() methods could be moved to their own object, like JSONCache maybe?

Yes, we certainly could. I have kept them here thinking that we might want to trigger them from the outside for testing purposes, but I have no objection to moving them.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> Well, I've kept it as a separate function despite the fact that it's always
> called after collectBaseTabData simply because these two functions are large
> enough and I didn't want to make them even larger.

Yeah I didn't want to imply that we should merge them. I was just wondering why we built it that way moons ago.

> > The problem is that _cachedJSON would be serialized as well, right? I think
> > we can omit this by using Name() to create a private name object.
> 
> I have just set it to |undefined| instead of |null|, this should prevent it
> from being serialized.

Ok, that should work, too.

Comment 7

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

For the moment, I will concentrate on caching stuff to avoid communication between workers. This bug might end up WONTFIX or the code be moved completely off main thread.