Move writing sessionstore.js off the main thread

RESOLVED FIXED in Firefox 3.6b1

Status

()

Firefox
Session Restore
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bz, Assigned: sdwilsh)

Tracking

({perf})

Trunk
Firefox 3.6b1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.6 +
in-litmus +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: [tsnap], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

8 years ago
Flags: blocking-firefox3.6?
Keywords: perf
Whiteboard: [tsnap]
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
the writes occur every 10 seconds by default. given this high frequency, moving to the background could be beneficial.
(Assignee)

Comment 2

8 years ago
s/could/would/

Comment 3

8 years ago
Bug 504780 could depend on this, but I'm not sure since the freeze could be caused by the page parsing.
Should we also consider using the OS's (or our own) prioritized IO to ensure we don't IO starve the main thread?
(Assignee)

Updated

8 years ago
Depends on: 508605
(Assignee)

Comment 5

8 years ago
Created attachment 392773 [details] [diff] [review]
v1.0

Use NetUtil's asyncCopy to do the writing on a background thread.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #392773 - Flags: review?(dietrich)
(Assignee)

Updated

8 years ago
Whiteboard: [tsnap] → [tsnap][needs review dietrich]
(Reporter)

Comment 6

8 years ago
Was there a reason to not use convertToInputStream here?
(Assignee)

Comment 7

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

Comment 8

8 years ago
Created attachment 392776 [details] [diff] [review]
v1.1

per bz's comment
Attachment #392773 - Attachment is obsolete: true
Attachment #392776 - Flags: review?(dietrich)
Attachment #392773 - Flags: review?(dietrich)

Comment 9

8 years ago
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)?
(Assignee)

Comment 10

8 years ago
(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

8 years ago
(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 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.
Attachment #392776 - Flags: review?(dietrich) → review+
(Assignee)

Updated

8 years ago
Whiteboard: [tsnap][needs review dietrich] → [tsnap]
(Assignee)

Comment 13

8 years ago
Created attachment 393074 [details] [diff] [review]
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.
Attachment #392776 - Attachment is obsolete: true
Attachment #393074 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #393074 - Flags: review? → review?(dietrich)
(Assignee)

Updated

8 years ago
Attachment #393074 - Flags: review?(dietrich) → review+
(Assignee)

Updated

8 years ago
Whiteboard: [tsnap] → [tsnap][can land]
(Assignee)

Comment 14

8 years ago
http://hg.mozilla.org/mozilla-central/rev/d657eb912b33
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [tsnap][can land] → [tsnap]
Target Milestone: --- → Firefox 3.6b1

Updated

8 years ago
Depends on: 509668
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)
(Assignee)

Comment 16

8 years ago
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.
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.
Flags: in-litmus? → in-litmus+
Duplicate of this bug: 511964

Comment 19

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

Updated

8 years ago
Flags: blocking-firefox3.6? → blocking-firefox3.6+

Updated

8 years ago
Blocks: 547406
You need to log in before you can comment on or make changes to this bug.