891360 - Move SessionStore I/O logic to a dedicated worker

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Moving most of the _SessionFile I/O to a dedicated worker has a couple of advantages. We can do more stuff in the worker without blocking the main thread and reduce communication overhead. It will also help us getting bug 887394 right.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: move SessionStore I/O logic to a dedicated worker

This patch moves most of the logic from _SessionFile.jsm to SessionWorker.js.

syncRead() now yields a deprecation warning.

The session's loadState=running is now written to disk with minimal communication overhead as we keep the state in the worker and do all the JSON fun in there. syncRead() passes the file contents to the worker as a fallback.

Tests are passing locally.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Try looks good:

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

Comment 3

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

Comment on attachment 772658 [details] [diff] [review]
move SessionStore I/O logic to a dedicated worker

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

::: browser/components/sessionstore/src/SessionWorker.js
@@ +29,5 @@
> +    throw new Error("Cannot find method " + data.fun);
> +  }
> +
> +  let result;
> +  let id = data.id;

You don't have |id| in your documentation.
This might, somehow, suggest that I also forgot to put this in my own documentation, does it?

@@ +48,5 @@
> +  self.postMessage({ok: result, id: id});
> +};
> +
> +let Agent = {
> +  // The initial session read from disk.

Nit: Type information?

@@ +56,5 @@
> +  path: OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js"),
> +
> +  // The path to sessionstore.bak
> +  backupPath: OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.bak"),
> +

All these methods would deserve some documentation.

@@ +57,5 @@
> +
> +  // The path to sessionstore.bak
> +  backupPath: OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.bak"),
> +
> +  setInitialState: function (aState) {

The role of this function is a little unclear to me. Also, shouldn't it throw rather than noop if there's already an initial state?

@@ +66,5 @@
> +
> +  /**
> +   * In case sessionstore.js file does not exist or is corrupted (something
> +   * happened between backup and write), attempt to read the sessionstore.bak
> +   * instead.

We are actually not determining whether sessionstore.js was corrupted, are we?

Doing this (in a followup bug) might be a useful information, e.g. to not overwrite backups with corrupted files.

@@ +88,5 @@
> +  },
> +
> +  writeLoadStateOnceAfterStartup: function (loadState) {
> +    if (!this.initialState) {
> +      // No initial state available.

That will need some clarification. If we don't have an initial state or if it is empty... then what?

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +221,4 @@
>    },
>  
> +  read: function () {
> +    return SessionWorker.post("read").then(msg => msg.ok);

You seem to have removed the Telemetry.
Could you either re-add it or file a followup bug to re-add it?

@@ +246,5 @@
>        }
>      }.bind(this));
>    },
>  
> +  writeLoadStateOnceAfterStartup: function (aLoadState) {

It might be useful to throw an error if this method has been called already.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: move SessionStore I/O logic to a dedicated worker, v2

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> You don't have |id| in your documentation.

Added.

> All these methods would deserve some documentation.

They do, fixed.

> > +  setInitialState: function (aState) {
> 
> The role of this function is a little unclear to me. Also, shouldn't it
> throw rather than noop if there's already an initial state?

I added a comment describing the purpose of the method and also how we handle the case of this.initialState being set already.

> We are actually not determining whether sessionstore.js was corrupted, are
> we?

No, we're not. Removed that comment.

> Doing this (in a followup bug) might be a useful information, e.g. to not
> overwrite backups with corrupted files.

Yes, that would indeed be a good follow-up. Will file.

> > +  writeLoadStateOnceAfterStartup: function (loadState) {
> > +    if (!this.initialState) {
> > +      // No initial state available.
> 
> That will need some clarification. If we don't have an initial state or if
> it is empty... then what?

Then... we should fail. That's what the patch does now.

> > +  read: function () {
> > +    return SessionWorker.post("read").then(msg => msg.ok);
> 
> You seem to have removed the Telemetry.
> Could you either re-add it or file a followup bug to re-add it?

Yes, that would be a good follow-up, too. Will file.

> > +  writeLoadStateOnceAfterStartup: function (aLoadState) {
> It might be useful to throw an error if this method has been called already.

It does now.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Looking good on try:

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

Comment 6

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

Comment on attachment 774331 [details] [diff] [review]
move SessionStore I/O logic to a dedicated worker, v2

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

Looks good, thanks.

::: browser/components/sessionstore/src/SessionWorker.js
@@ +54,5 @@
> +  initialState: null,
> +
> +  // Boolean that tells whether we already wrote
> +  // the loadState to disk once after startup.
> +  wroteLoadStateOnce: false,

I'd call this |isWriteLoadStateOnceDone| or |hasWrittenLoadStateOnce|.

@@ +66,5 @@
> +  /**
> +   * This method is only intended to be called by _SessionFile.syncRead() and
> +   * can be removed when we're not supporting synchronous SessionStore
> +   * initialization anymore. When sessionstore.js is read from disk
> +   * synchronously the state string must be supplied to the worker manually.

Nit: "by calling this method"

@@ +72,5 @@
> +  setInitialState: function (aState) {
> +    // Initial state might have been filled by read() already but yet we might
> +    // be called by _SessionFile.syncRead() before SessionStore.jsm had a chance
> +    // to call writeLoadStateOnceAfterStartup(). It's safe to ignore
> +    // setInitialState() calls if this happens.

Ah, good, I hadn't thought about that.

@@ +75,5 @@
> +    // to call writeLoadStateOnceAfterStartup(). It's safe to ignore
> +    // setInitialState() calls if this happens.
> +    if (!this.initialState) {
> +      this.initialState = aState;
> +    }

Perhaps you could also fail if |wroteLoadStateOnce| is true.

@@ +110,5 @@
> +   * after startup so that we detect crashes on startup correctly.
> +   */
> +  writeLoadStateOnceAfterStartup: function (loadState) {
> +    if (this.wroteLoadStateOnce) {
> +      throw "writeLoadStateOnceAfterStartup() must only be called once.";

Just because I'm a nervous guy, I would throw |new Error(...)|, because I like my stack traces in my exceptions.

@@ +134,5 @@
> +    return this.write(JSON.stringify(state));
> +  },
> +
> +  /**
> +   * Moves sessionstore.js to sessionstore.bak.

I completely forgot why we move instead of copying. Might be a good opportunity to document this.

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +71,5 @@
>      return SessionFileInternal.write(aData);
>    },
>    /**
> +   * Writes the initial state to disk again
> +   * only to change the session's load state.

Note: Calling this a second time is [an error|a noop].

@@ -149,5 @@
>  let SessionFileInternal = {
>    /**
> -   * A promise fulfilled once initialization is complete
> -   */
> -  promiseInitialized: Promise.defer(),

Oh, that was dead code? Thanks for spotting this.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

All feedback addressed.

https://hg.mozilla.org/integration/fx-team/rev/242b07a68acf

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Pushed a bustage fix, sorry :|

https://hg.mozilla.org/integration/fx-team/rev/ad8cff9af5c4

Comment 9

[Bill Gianopoulos [:WG9s]]

It is not clear to me from comment #0.  Is this patch sufficient to fix the regressions from bug 887394, or is it just a prerequisite?

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

This should be sufficient.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/242b07a68acf
https://hg.mozilla.org/mozilla-central/rev/ad8cff9af5c4

Comment 12

[Guilherme Lima]

Can this bug be responsible for memory that don't go away easily?

├───92.18 MB (25.25%) -- workers/workers()
│   ├──83.61 MB (22.90%) -- worker(resource:///modules/sessionstore/SessionWorker.js, 0x656f800)
│   │  ├──81.10 MB (22.21%) -- zone(0xcc09c00)
│   │  │  ├──53.46 MB (14.64%) -- string-chars
│   │  │  │  ├──53.13 MB (14.55%) ++ huge
│   │  │  │  └───0.33 MB (00.09%) ── non-huge
│   │  │  ├──27.53 MB (07.54%) -- compartment(web-worker)
│   │  │  │  ├──26.89 MB (07.37%) -- objects-extra
│   │  │  │  │  ├──26.86 MB (07.36%) ── elements/non-asm.js
│   │  │  │  │  └───0.03 MB (00.01%) ── slots
│   │  │  │  └───0.64 MB (00.17%) ++ (5 tiny)
│   │  │  └───0.11 MB (00.03%) ++ (2 tiny)
│   │  └───2.51 MB (00.69%) ++ (4 tiny)

only goes away when I press 'Minimize memory usage'

Comment 13

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

It could Is 53Mb roughly the size of your file sessionstore.js?

Comment 14

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

It could. Is 53Mb roughly the size of your file sessionstore.js?

Comment 15

[Guilherme Lima]

Right now:

 ├──83.96 MB (17.96%) -- worker(resource:///modules/sessionstore/SessionWorker.js, 0x6f25c00)
│   │  ├──81.50 MB (17.44%) -- zone(0xcaa0000)
│   │  │  ├──53.90 MB (11.53%) -- string-chars
│   │  │  │  ├──53.86 MB (11.52%) ++ huge
│   │  │  │  └───0.04 MB (00.01%) ── non-huge
│   │  │  ├──27.51 MB (05.88%) -- compartment(web-worker)
│   │  │  │  ├──27.09 MB (05.80%) -- objects-extra
│   │  │  │  │  ├──27.07 MB (05.79%) ── elements/non-asm.js
│   │  │  │  │  └───0.03 MB (00.01%) ── slots
│   │  │  │  └───0.41 MB (00.09%) ++ (5 tiny)
│   │  │  └───0.09 MB (00.02%) ++ (2 tiny)
│   │  └───2.47 MB (00.53%) ++ (4 tiny)

and sessionstore is 142KB.
If I click on the ++huge, it shows 
0.98 MB (00.21%) ── string(length=510730, "{"windows":[{"tabs":[{"entries"...")
a lot of times, but each time the size is smaller, until the last one is 0.26 MB (00.06%) ── string(length=134977, "{"windows":[{"tabs":[{"entries"...")

Comment 16

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

Let's move this discussion to bug 894993.
Could you also describe (in that bug) all the details that you think may be important? Right now, I would be interested in knowing how long your session has lasted before you reached these 83.96 Mb.