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

VERIFIED FIXED in Firefox 31

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: smacleod, Assigned: marco)

Tracking

Trunk
Firefox 31
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=0 s=it-31c-30a-29b.1 [qa!])

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

5 years ago
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.
This should be trivial to do now.
Assignee

Comment 2

5 years ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #8392458 - Flags: review?(dteller)
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.
Attachment #8392458 - Flags: review?(dteller) → review+
Wasn't there some other cleanup to do WRT the session worker?
There's some more cleanup in bug 883609.
Assignee

Comment 6

5 years ago
Posted patch Alternative patch (obsolete) — Splinter Review
In this patch I've also removed |_readSessionFile| and modified |read| a bit. I'd prefer this one.
Attachment #8392466 - Flags: review?(dteller)
Assignee

Comment 7

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

Comment 8

5 years ago
(I'll add the encoding option to the patch you choose)
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?
Assignee

Comment 10

5 years ago
(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").
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.
Assignee

Comment 12

5 years ago
Posted patch Patch (obsolete) — Splinter Review
I'm not blocking on the promise returned by OS.File.stat, so the function returns the data right away.
Attachment #8392458 - Attachment is obsolete: true
Attachment #8392466 - Attachment is obsolete: true
Attachment #8392466 - Flags: review?(dteller)
Attachment #8392525 - Flags: review?(dteller)
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?
Flags: needinfo?(ttaubert)
Comment on attachment 8392525 [details] [diff] [review]
Patch

Canceling review until we have discussed the best strategy to extract the size.
Attachment #8392525 - Flags: review?(dteller)
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.
Flags: needinfo?(ttaubert)
Assignee

Comment 16

5 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #8392525 - Attachment is obsolete: true
Attachment #8395212 - Flags: review?(dteller)
Assignee

Comment 17

5 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
We can also stop importing NetUtil.jsm and FileUtils.jsm.
Attachment #8395212 - Attachment is obsolete: true
Attachment #8395212 - Flags: review?(dteller)
Attachment #8395217 - Flags: review?(dteller)
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.
Attachment #8395217 - Flags: review?(dteller) → review+
(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!
Thanks for working on this, Marco!
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Assignee

Comment 21

5 years ago
Posted patch Patch v2Splinter Review
Attachment #8395217 - Attachment is obsolete: true
Attachment #8395626 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/18cf9739d197
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Hi Anthony, can you review to determine if this bug should be marked as [qa+] or [qa-] for verification.
Flags: needinfo?(anthony.s.hughes)
Whiteboard: p=0 s=it-31c-30a-29b.1
qa+ to ensure this doesn't regress session restore.
Flags: needinfo?(anthony.s.hughes)
QA Contact: cornel.ionce
Whiteboard: p=0 s=it-31c-30a-29b.1 → p=0 s=it-31c-30a-29b.1 [qa+]
Hi Cornel.  When can verification for this item be completed?  Our iteration ends on Monday, March 31st.
Flags: needinfo?(cornel.ionce)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(cornel.ionce)
QA Contact: cornel.ionce → alexandra.lucinet
Whiteboard: p=0 s=it-31c-30a-29b.1 [qa+] → p=0 s=it-31c-30a-29b.1 [qa!]
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.
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
You need to log in before you can comment on or make changes to this bug.