[SessionStore] Cache generated JSON

RESOLVED WONTFIX

Status

()

Firefox
Session Restore
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: Yoric, Unassigned)

Tracking

(Blocks: 3 bugs, {addon-compat})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async:P2])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 777194 [details] [diff] [review]
Caching generated JSON
Assignee: nobody → dteller
Attachment #777194 - Flags: feedback?(ttaubert)
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.
Attachment #777194 - Flags: feedback?(ttaubert) → feedback+
(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.
(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.
Created attachment 777739 [details] [diff] [review]
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.
Attachment #777194 - Attachment is obsolete: true
(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.
Status: NEW → ASSIGNED
No longer depends on: 838577
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.
Assignee: dteller → nobody
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.