485976 - Move writing sessionstore.js off the main thread

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now as far as I can tell session store writes data to disk synchronously on the UI thread.  It might make sense to move this to a worker thread.

Comment 1

[Dietrich Ayala (:dietrich)]

the writes occur every 10 seconds by default. given this high frequency, moving to the background could be beneficial.

Comment 2

[Shawn Wilsher :sdwilsh]

s/could/would/

Comment 3

[Thomas Bertels]

Bug 504780 could depend on this, but I'm not sure since the freeze could be caused by the page parsing.

Comment 4

[Rob Arnold [:robarnold]]

Should we also consider using the OS's (or our own) prioritized IO to ensure we don't IO starve the main thread?

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Use NetUtil's asyncCopy to do the writing on a background thread.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Was there a reason to not use convertToInputStream here?

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Was there a reason to not use convertToInputStream here?
I didn't know about it ;)

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

per bz's comment

Comment 9

[Simon Bünzli]

Not knowing stream internals: Are we still using the equivalent of a safe output stream with this change (i.e. not overwriting the existing file until all data has been safely written to disk and fsync'ing once done)?

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> Not knowing stream internals: Are we still using the equivalent of a safe
> output stream with this change (i.e. not overwriting the existing file until
> all data has been safely written to disk and fsync'ing once done)?
Yes - we are still creating the safe file output stream and using that.  The difference with this patch is that the input stream (from the Unicode converter) is copied to the output stream on a background thread, and the fsync call is also done on a background thread.

v1.0 passed unit tests on the try server, and v1.1 passed them locally.

Comment 11

[Simon Bünzli]

(In reply to comment #10)
> Yes - we are still creating the safe file output stream and using that.

That's what I get for focusing too much on the removed lines... obviously.

> v1.0 passed unit tests on the try server, and v1.1 passed them locally.

IIRC, we don't have any unit tests for the bit of code you're changing here. :-(

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 392776 [details] [diff] [review]
v1.1

r=me. yes, while this is well covered by litmus, would like some automated testing of this since it's fairly trivial to do so.

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Talked it over with dietrich on irc, and it turns out this is a lot harder to test than expected.  We'll have to go with a litmus test for now.

Made some changes so it might be easier to test in the future though.  Works just fine locally.

Comment 14

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/d657eb912b33

Comment 15

[u279076]

Shawn, is there a specific test case you need written for this?  Would the test here be more about checking for performance regressions against our current set of litmus tests (https://litmus.mozilla.org/show_test.cgi?searchType=by_category&product_id=1&branch_id=32&testgroup_id=137&subgroup_id=1262)

Comment 16

[Shawn Wilsher :sdwilsh]

It may be covered by other existing tests.  Basically, just making sure that the file is actually written out when it is supposed to is fine.

Comment 17

[u279076]

We have tests which check the file exists when it is supposed to and that the session actually restores.  I don't think we need a specific test for this bug.  We should keep an eye out for any regressions due to this bug.  I can do a test run of our Session Store tests in Litmus on Windows, Mac and Linux to spotcheck for regressions and mark this verified.

Marking in-litmus+ as per this discussion.

Comment 19

[chris hofmann]

anthony,  bug 511964 suggests another user visible kind of test case that we should try to construct.    see if youtube videos can be delayed when big session restore updates are written out.

Comment 20

[u279076]

(In reply to comment #19)
> anthony,  bug 511964 suggests another user visible kind of test case that we
> should try to construct.    see if youtube videos can be delayed when big
> session restore updates are written out.

Created a litmus test case specifically for the YouTube issue:
https://litmus.mozilla.org/show_test.cgi?id=9484