Closed Bug 1084009 Opened 5 years ago Closed 4 years ago

parse (large) sync scripts off the main thread

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: luke, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

As mentioned in bug 1061886 comment 1, we could significantly improve main-thread responsiveness when parsing large cold scripts if we applied our existing off-main-thread parsing machinery to sync scripts.  Additionally, even though they are "sync" scripts, iiuc, the content itself can still be partially responsive (to events, and builtin behaviors like scrolling) while parsing is happening off the main thread.
Blocks: 1154987
Forwarding r+ from :smaugh on bug 1167409.
Attachment #8659468 - Flags: review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/68ca35b8034b for what I at first thought was just the exact same failure it caused when it last landed back in July, opt b2g emulator mochitest-chrome's test_settings_service.xul, but it did manage to also grow a new failure in the meantime, every single debug b2g emulator mochitest run failing to start up like https://treeherder.mozilla.org/logviewer.html#?job_id=13920654&repo=mozilla-inbound
And for even more fun, frequent but not quite permanent debug Windows and Mac failures in browser_UITour_loop.js like https://treeherder.mozilla.org/logviewer.html#?job_id=13927222&repo=mozilla-inbound
(In reply to Phil Ringnalda (:philor) from comment #4)
> And for even more fun, frequent but not quite permanent debug Windows and
> Mac failures in browser_UITour_loop.js like
> https://treeherder.mozilla.org/logviewer.html#?job_id=13927222&repo=mozilla-
> inbound

Argh.  I fixed that test, it's broken again.

I pushed because this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=105f5a67a096
Looked promisingly green.
Forwarding review from old patch. This patch is slightly modified in that scripts from the pre-load cache are not off-main-thread compiled (the logic that does that has just been eliminated).
Attachment #8659468 - Attachment is obsolete: true
Attachment #8662450 - Flags: review+
Don't do off-thread compilation on single-core systems.  This is mainly because of a bunch of new test failures on B2G ICS Emulator where tests were timing out because things were taking too long.  The thread dispatch/process/rejoin overhead seems to push the emulator to take too long to complete.
Attached patch 3-fix-test.patchSplinter Review
Test fix for browser_UITour_loop.js.  Just modifying some timeouts to wait a bit longer for some page events to occur.
Attachment #8662451 - Flags: review?(luke)
Comment on attachment 8662452 [details] [diff] [review]
3-fix-test.patch

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

This is necessary because this test sometimes oranges with the new off-thread script parsing code, due to too-aggressive timeouts on this assert.
Attachment #8662452 - Flags: review?(mdeboer)
Comment on attachment 8662451 [details] [diff] [review]
2-only-enable-on-multiprocessor-systems.patch

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

::: dom/base/nsScriptLoader.cpp
@@ +1656,5 @@
>    // Mark this as loaded
>    aRequest->mProgress = nsScriptLoadRequest::Progress_DoneLoading;
>  
>    // If this is currently blocking the parser, attempt to compile it off-main-thread.
> +  if (aRequest == mParserBlockingRequest && (PR_GetNumberOfProcessors() > 1)) {

I don't know how hot this code is, but do you think it's worth caching (since I'd expect it's a syscall)?
(In reply to Luke Wagner [:luke] from comment #11)
> Comment on attachment 8662451 [details] [diff] [review]
> 2-only-enable-on-multiprocessor-systems.patch
> 
> Review of attachment 8662451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsScriptLoader.cpp
> @@ +1656,5 @@
> >    // Mark this as loaded
> >    aRequest->mProgress = nsScriptLoadRequest::Progress_DoneLoading;
> >  
> >    // If this is currently blocking the parser, attempt to compile it off-main-thread.
> > +  if (aRequest == mParserBlockingRequest && (PR_GetNumberOfProcessors() > 1)) {
> 
> I don't know how hot this code is, but do you think it's worth caching
> (since I'd expect it's a syscall)?

This codepath comes at the end of a network request completion, so one of these will happen every time a network script request completes, which are already several syscalls to both send the request and receive the contents, and then for the inter-thread messaging to deliver the completed buffer here.

I figure PR_GetNumberOfProcessors is small fry in that scheme of things.
Comment on attachment 8662451 [details] [diff] [review]
2-only-enable-on-multiprocessor-systems.patch

Fair enough
Attachment #8662451 - Flags: review?(luke) → review+
Comment on attachment 8662452 [details] [diff] [review]
3-fix-test.patch

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

I could live without the 'tryCount = ' comment...
Attachment #8662452 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> Comment on attachment 8662452 [details] [diff] [review]
> 3-fix-test.patch
> 
> Review of attachment 8662452 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I could live without the 'tryCount = ' comment...

Fixed.
Depends on: 1207987
Blocks: 1211286
You need to log in before you can comment on or make changes to this bug.