1284056 - Baldr: make wasm::Compile cx-free

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

To support both async compilation (bug 1283924) and recompile-on-structured-clone-read-of-stale-machine-code (bug 1276029), wasm::Compile should not depend on a cx or rt, taking all state explicitly, so that it can execute on any thread and compile code for any (or multiple) runtime.

Comment 1

[Luke Wagner [:luke]]

Attachment: mv-compile-args

Trivial renaming patch to clear up the name "CompileArgs" for a new, more-appropriate struct that is actually the argument to wasm::Compile :)

Comment 2

[Luke Wagner [:luke]]

Attachment: mv-machine-id

This simple patch renames MachineId to Assumptions (since this is a more apt description of what this struct means: it's the assumptions we compiled the code under which must remain valid after deserialization), stores the Assumptions in Metadata, and moves SignalUse into Assumptions.  The root motivation here is removing the 'cx' arg in a bunch of places.

Comment 3

[Luke Wagner [:luke]]

Attachment: rm-compile-runtime

Same motivation stated in bug 1283924 comment 1 (just moved the patch here).

Comment 4

[Luke Wagner [:luke]]

Attachment: empty-jcx

Same motivation as stated in bug 1283924 comment 4 (just moved patch here).

Comment 5

[Luke Wagner [:luke]]

Attachment: simplify-parallel-compilation-test

Same motivation stated in bug 1283924 comment 5, just moved patch here.

Comment 6

[Luke Wagner [:luke]]

Attachment: rm-mg-cx

This patch removes 'cx' from ModuleGenerator which is pretty simple and mostly just converting some TempAllocPolicy to SystemAllocPolicy and hoisting out the wasmAlwaysBaseline setting via a new CompileArgs struct argument to wasm::Compile.

Comment 7

[Luke Wagner [:luke]]

Attachment: rm-compile-cx

With the preceding refactorings, the only issue left is validation-error reporting in WasmCompile.cpp.  This patch pipes around a UniqueChars to hold the error string which the wasm::Compile-caller can then use to do the Right Thing.

Comment 8

[Luke Wagner [:luke]]

Attachment: rm-mg-cx

Now with 'filename' as a CompileArg.

Comment 9

[Luke Wagner [:luke]]

Attachment: rm-mg-cx

Now, qref'd.

Comment 10

[Luke Wagner [:luke]]

Attachment: rm-compile-cx

Rebased on top of previous updated patch.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8767491 [details] [diff] [review]
rm-compile-runtime

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

::: js/src/vm/HelperThreads.cpp
@@ -1347,5 @@
>      {
>          AutoUnlockHelperThreadState unlock(locked);
> -
> -        TraceLoggerThread* logger = TraceLoggerForCurrentThread();
> -        AutoTraceLog logCompile(logger, TraceLogger_WasmCompilation);

Why can we remove this? It'd be good to not regress TL.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8767492 [details] [diff] [review]
empty-jcx

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

Nice

Comment 13

[Luke Wagner [:luke]]

(In reply to Jan de Mooij [:jandem] from comment #11)
Oops, I had thought that there was a dependence on the CompileRuntime, but putting it back in and trying again, I guess not.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8767489 [details] [diff] [review]
mv-compile-args

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

It's a fine name, thanks.

::: js/src/asmjs/WasmGenerator.h
@@ +64,5 @@
>  typedef Vector<ImportModuleGeneratorData, 0, SystemAllocPolicy> ImportModuleGeneratorDataVector;
>  
>  struct ModuleGeneratorData
>  {
> +    SignalUse                       signalUse;

Maybe that's just too cute, but how about naming this "useSignal" instead, so that reading it is very natural:

if (mg.useSignal.forOOB) {
 ...
}

::: js/src/asmjs/WasmTypes.h
@@ +779,4 @@
>  // generation. It also currently is the single source of truth for whether or
>  // not to use signal handlers for different purposes.
>  
> +struct SignalUse

You're the English native speaker here, but should this be SignalUsage? (as HeapUse vs HeapUsage before)

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8767490 [details] [diff] [review]
mv-machine-id

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

Nice.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8767493 [details] [diff] [review]
simplify-parallel-compilation-test

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

::: js/src/asmjs/WasmGenerator.cpp
@@ +731,5 @@
>      MOZ_ASSERT(!finishedFuncDefs_);
>  
> +    // The wasmCompilationInProgress atomic ensures there is only one parallel
> +    // compilation in progress at a time. Since maxWasmCompilationThreads uses
> +    // cpuCount which is less than threadCount, this ensures we cannot have a

Isn't it that maxWasmCompilationThreads returns 2 threads at least, and there's only one parser thread maximum, ensuring there's always one thread for compilation?

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8767509 [details] [diff] [review]
rm-mg-cx

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

::: js/src/asmjs/WasmGenerator.cpp
@@ -801,5 @@
> -    if (cx_->isJSContext())
> -        logger = TraceLoggerForMainThread(cx_->asJSContext()->runtime());
> -    else
> -        logger = TraceLoggerForCurrentThread();
> -    AutoTraceLog logCompile(logger, TraceLogger_WasmCompilation);

Is there a way to not regress Tracelogger? Compilation can take some time, so this will certainly show up in profiles. Or add a TODO that when we're sure to not compile on the main thread anymore, we can just use TraceLoggerForCurrentThread all the time?

Comment 18

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #14)
> Maybe that's just too cute, but how about naming this "useSignal" instead,
> so that reading it is very natural:
> 
> if (mg.useSignal.forOOB) {

I like it; I'll change s/SignalUse/UseSignal/ to match.

(In reply to Benjamin Bouvier [:bbouvier] from comment #16)
> Isn't it that maxWasmCompilationThreads returns 2 threads at least, and
> there's only one parser thread maximum, ensuring there's always one thread
> for compilation?

You're right, although that makes me realize maxWasmCompilationThreads is irrelevant: it could return any number; all that matters is the number of helper threads which can be blocked waiting for other helper threads (which the atomic caps at 1) and that threadCount is always >1.  I'll update the comment.

Also, we can stop worrying about this altogether (b/c we might want to have a long-running helper task for background-ion-compilation) if we can have ModuleGenerator, when it would today block, instead process IonCompileTasks.  Then there is always progress.

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
Ah yes, I'll do TraceLoggerForCurrentThread() which actually can now be hoisted from HelperThreads.cpp/ModuleGenerator.cpp into wasm::CompileFunction.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8767510 [details] [diff] [review]
rm-compile-cx

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

\o/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c666dc1d7001
Baldr: rename CompileArgs to UseSignal (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/425c7ce749c7
Baldr: rename MachineId to Assumptions (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3649be85315
Baldr: don't set CompileRuntime (r=jandem)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c508af98ec66
Baldr: create totally empty JitContext in ModuleGenerator (r=jandem)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1647bedef92e
Baldr: simplify the parallel compilation guard (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/433ae510d1fd
Baldr: remove cx_ from ModuleGenerator (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aeb6b6748db
Baldr: remove cx from wasm::Compile (r=bbouvier)

Comment 21

[Luke Wagner [:luke]]

Attachment: re-add-always-baseline

Uh whoops, rm-mg-cx made a bool CompileArgs::alwaysBaseline but forgot to actually set it, effectively disabling baseline testing.

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8768399 [details] [diff] [review]
re-add-always-baseline

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

Doh!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad84e1c04632
Baldr: initialize CompileArgs::alwaysBaseline (r=bbouvier)

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c666dc1d7001
https://hg.mozilla.org/mozilla-central/rev/425c7ce749c7
https://hg.mozilla.org/mozilla-central/rev/f3649be85315
https://hg.mozilla.org/mozilla-central/rev/c508af98ec66
https://hg.mozilla.org/mozilla-central/rev/1647bedef92e
https://hg.mozilla.org/mozilla-central/rev/433ae510d1fd
https://hg.mozilla.org/mozilla-central/rev/8aeb6b6748db
https://hg.mozilla.org/mozilla-central/rev/ad84e1c04632