970495 - [Session Restore] Switch SessionFile.read() to use the native OS.File read()

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Steven MacLeod [:smacleod]]

Using NetUtil to read the session file in Bug 959130 is just a stop-gap until the Native OS.File read() lands in Bug 961665.

We should change SessionFile.read() to use OS.File read() when it lands.

Comment 1

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

This should be trivial to do now.

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 3

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

Comment on attachment 8392458 [details] [diff] [review]
Patch

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

r+ with the change below

::: browser/components/sessionstore/src/SessionFile.jsm
@@ +160,3 @@
>      let durationMs = Date.now();
>  
> +    return OS.File.read(path).then(function(array) {

Since bug 961665, the best way to do this is
 return OS.File.read(path, { encoding: "utf-8"})
(resolves to a string).

This will ensure that decoding is done off the main thread, and resolved without copy. For session restore, this should be much faster. Plus that's one line less.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Wasn't there some other cleanup to do WRT the session worker?

Comment 5

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

There's some more cleanup in bug 883609.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Alternative patch

In this patch I've also removed |_readSessionFile| and modified |read| a bit. I'd prefer this one.

Comment 7

[Marco Castelluccio [:marco]]

Oh, you're a fast reviewer, I didn't see your comments. Which one do you prefer?

Comment 8

[Marco Castelluccio [:marco]]

(I'll add the encoding option to the patch you choose)

Comment 9

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

Comment on attachment 8392466 [details] [diff] [review]
Alternative patch

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

I prefer the second patch, thanks.

::: browser/components/sessionstore/src/SessionFile.jsm
@@ +138,3 @@
>        for (let filename of [this.path, this.backupPath]) {
>          try {
> +          let durationMs = Date.now();

Nit: It's not a duration yet, could you give it another name?

@@ +138,5 @@
>        for (let filename of [this.path, this.backupPath]) {
>          try {
> +          let durationMs = Date.now();
> +
> +          let data = yield OS.File.read(filename);

Let's add option { encoding: "utf-8" } and get rid of the decoder below.

@@ +143,5 @@
> +
> +          durationMs = Date.now() - durationMs;
> +
> +          Telemetry.getHistogramById("FX_SESSION_RESTORE_READ_FILE_MS").add(durationMs);
> +          Telemetry.getHistogramById("FX_SESSION_RESTORE_FILE_SIZE_BYTES").add(data.byteLength);

On my screen, the indentation looks wrong. Can you make sure that it's ok?

Comment 10

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #9)
> @@ +138,5 @@
> >        for (let filename of [this.path, this.backupPath]) {
> >          try {
> > +          let durationMs = Date.now();
> > +
> > +          let data = yield OS.File.read(filename);
> 
> Let's add option { encoding: "utf-8" } and get rid of the decoder below.

There's a problem here, this way I don't know what the size of the file is (for "FX_SESSION_RESTORE_FILE_SIZE_BYTES").

Comment 11

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

Mmmmhh...
On the other hand, this new version should be *much* faster. I guess we could patch OS.File.read() to provide this information, but that would be rather complicated just for that purpose.

I guess you could use OS.File.stat() to get the length of the file – just make sure that you do this *after* returning the result.

Comment 12

[Marco Castelluccio [:marco]]

Attachment: Patch

I'm not blocking on the promise returned by OS.File.stat, so the function returns the data right away.

Comment 13

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

It dawns to me that we could also get the information from the return value of writeAtomic, which would probably be faster, by patching write() in SessionWorker.js. This would save us a file I/O.

Tim, any preference?

Comment 14

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

Comment on attachment 8392525 [details] [diff] [review]
Patch

Canceling review until we have discussed the best strategy to extract the size.

Comment 15

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

Actually, I have made up my mind.
Let's keep SessionFile.read() simple and fast, without a call to lstat, and let's get the data from SessionWorker's write(). In bug 883609, I'll patch SessionWorker.js further to make sure that we only extract the data once per run.

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Patch v2

We can also stop importing NetUtil.jsm and FileUtils.jsm.

Comment 18

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

Comment on attachment 8395217 [details] [diff] [review]
Patch v2

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

Looks good to me, thanks.
Here's hoping that this makes our startup faster.

::: browser/components/sessionstore/src/SessionFile.jsm
@@ +141,5 @@
> +
> +          Telemetry.getHistogramById("FX_SESSION_RESTORE_READ_FILE_MS")
> +                   .add(Date.now() - startMs);
> +
> +          return data; 

Nit: whitespace at the end of the line.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #13)
> It dawns to me that we could also get the information from the return value
> of writeAtomic, which would probably be faster, by patching write() in
> SessionWorker.js. This would save us a file I/O.

Sounds good to me!

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Thanks for working on this, Marco!

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18cf9739d197

Comment 24

[Marco Mucci [:MarcoM]]

Hi Anthony, can you review to determine if this bug should be marked as [qa+] or [qa-] for verification.

Comment 25

[u279076]

qa+ to ensure this doesn't regress session restore.

Comment 26

[Jenn Chaulk (:jchaulk)]

Hi Cornel.  When can verification for this item be completed?  Our iteration ends on Monday, March 31st.

Comment 27

[Ada [:adalucinet]]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:31.0) Gecko/20100101 Firefox/31.0

No regressions were found while doing exploratory testing on Session Restore with latest Nightly (Build ID: 20140326030203), on the following Operating systems: Windows 7 64bit, Ubuntu 13.10 32bit and Mac OS X 10.8.

Marking this as verified.

Comment 28

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

Telemetry has... interesting results.
http://telemetry.mozilla.org/#nightly/31/FX_SESSION_RESTORE_READ_FILE_MS indicates that the wallclock time to read and decode has exploded, but
http://telemetry.mozilla.org/#nightly/31/SIMPLE_MEASURES_SESSIONRESTORED indicates that the wallclock time to the end of the operation has decreased.

Waiting for metrics to help us decode this.