1371216 - Wasm: Make thread running the ModuleGenerator also perform compilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Lars T Hansen [:lth]]

There's a subtle piece of reasoning in ModuleGenerator::startFuncDefs wherein it is explained that we avoid deadlocks for asm.js compilation by having only a single parallel compile in progress at a time, and guaranteeing that there are at least two threads available.

For tiered compilation we'll want to have at least two parallel compilations in progress at a time (tier-2 for the first module overlapping with tier-1 for the subsequent module), so it's useful to just remove the restriction.  The simple way to do that is for the ModuleGenerator thread to also do compilation work.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1371216-modulegenerator-idle-compilation.patch

If a ModuleGenerator is running on a helper thread and it would wait for a notification from the compiler threads, it instead calls to the helper thread to opportunistically pick up a compile task.

The only tricky bit here seems to be to ensure that the proper parts of the HelperThread state are saved and restored when the thread is reused like this.

If the ModuleGenerator is not on a helper thread then there's no risk of deadlock and though we could still drive compilation ahead it doesn't seem necessary to do so at this time.

I have not lifted the restriction on one parallel compile at a time here; that will be addressed by bug 1277562.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8875647 [details] [diff] [review]
bug1371216-modulegenerator-idle-compilation.patch

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

Makes sense.

::: js/src/wasm/WasmGenerator.cpp
@@ +297,5 @@
>                  break;
>              }
>  
> +            if (!CurrentHelperThread() ||
> +                !CurrentHelperThread()->handleWasmIdleWorkload(lock))

CurrentHelperThread() looks kinda expensive if you have a lot of threads.  I think you can hoist it out of the loop into a field (maybeHelperThread_) of ModuleGenerator since it shouldn't change over the lifetime of the ModuleGenerator.

Also, can you add a short comment explaining that processing workloads on this thread instead of unconditionally waiting avoids potential deadlocks.

@@ +299,5 @@
>  
> +            if (!CurrentHelperThread() ||
> +                !CurrentHelperThread()->handleWasmIdleWorkload(lock))
> +            {
> +                HelperThreadState().wait(lock, GlobalHelperThreadState::CONSUMER);

Can you MOZ_ASSERT(outstanding_ > 0 && wasmWorklist.empty()) and comment that we can't deadlock because at least one other thread is actively working on a task which will notify us.

@@ +878,5 @@
>      // The wasmCompilationInProgress atomic ensures that there is only one
> +    // parallel compilation in progress at a time.  This restriction can be
> +    // lifted, but it will be somewhat desirable to keep it when tiered
> +    // compilation is introduced (and tier-2 code for one module can be compiled
> +    // in parallel with the tier-1 code for the next module).

IIUC, though, tier-2 codegen could grab wasmCompilationInProgress and then "lock out" a tier-1 codegen which should, I think, get preference since it's on the critical path.  It seems like we should have separate worklists of tier1 and tier2 compilation tasks and only process tier2 tasks when tier1 is empty.

Given that, I think we should kill this wasmCompilationInProgress; it's a real unnecessary perf cliff b/c if two compilation jobs, one long-running, overlap in the slightest, it can permanently serialize the long-running job.  I really should've fixed this a long time ago...

Not in this patch, of course, but I wouldn't say that wasmCompilationInProgress is desirable for tiering.

Comment 3

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 8875647 [details] [diff] [review]
> bug1371216-modulegenerator-idle-compilation.patch
> 

> @@ +878,5 @@
> >      // The wasmCompilationInProgress atomic ensures that there is only one
> > +    // parallel compilation in progress at a time.  This restriction can be
> > +    // lifted, but it will be somewhat desirable to keep it when tiered
> > +    // compilation is introduced (and tier-2 code for one module can be compiled
> > +    // in parallel with the tier-1 code for the next module).
> 
> IIUC, though, tier-2 codegen could grab wasmCompilationInProgress and then
> "lock out" a tier-1 codegen which should, I think, get preference since it's
> on the critical path.  It seems like we should have separate worklists of
> tier1 and tier2 compilation tasks and only process tier2 tasks when tier1 is
> empty.

We do have separate worklists and priorities; the way it's set up in my (now massive) patch queue is that tier-1 tasks always have priority.  What happens is that there can be one *each* of tier-1 and tier-2 compilations in progress at a time.

(And of course this comment block will change when tiering lands.)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c74284491275
Wasm, make the ModuleGenerator thread also perform compilation work. r=luke

Comment 5

[Lars T Hansen [:lth]]

Try run was here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df829a7812243f89514ac877356b0cf5af0b6a04

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c74284491275