Baldr: make wasm::Compile cx-free

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks: 1 bug)

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(8 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8767489 [details] [diff] [review]
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 :)
Attachment #8767489 - Flags: review?(bbouvier)
(Assignee)

Comment 2

a year ago
Created attachment 8767490 [details] [diff] [review]
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.
Attachment #8767490 - Flags: review?(bbouvier)
(Assignee)

Comment 3

a year ago
Created attachment 8767491 [details] [diff] [review]
rm-compile-runtime

Same motivation stated in bug 1283924 comment 1 (just moved the patch here).
Attachment #8767491 - Flags: review?(jdemooij)
(Assignee)

Comment 4

a year ago
Created attachment 8767492 [details] [diff] [review]
empty-jcx

Same motivation as stated in bug 1283924 comment 4 (just moved patch here).
Attachment #8767492 - Flags: review?(jdemooij)
(Assignee)

Comment 5

a year ago
Created attachment 8767493 [details] [diff] [review]
simplify-parallel-compilation-test

Same motivation stated in bug 1283924 comment 5, just moved patch here.
Attachment #8767493 - Flags: review?(bbouvier)
(Assignee)

Comment 6

a year ago
Created attachment 8767494 [details] [diff] [review]
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.
Attachment #8767494 - Flags: review?(bbouvier)
(Assignee)

Comment 7

a year ago
Created attachment 8767495 [details] [diff] [review]
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.
Attachment #8767495 - Flags: review?(bbouvier)
(Assignee)

Comment 8

a year ago
Created attachment 8767508 [details] [diff] [review]
rm-mg-cx

Now with 'filename' as a CompileArg.
Attachment #8767494 - Attachment is obsolete: true
Attachment #8767494 - Flags: review?(bbouvier)
Attachment #8767508 - Flags: review?(bbouvier)
(Assignee)

Comment 9

a year ago
Created attachment 8767509 [details] [diff] [review]
rm-mg-cx

Now, qref'd.
Attachment #8767508 - Attachment is obsolete: true
Attachment #8767508 - Flags: review?(bbouvier)
Attachment #8767509 - Flags: review?(bbouvier)
(Assignee)

Comment 10

a year ago
Created attachment 8767510 [details] [diff] [review]
rm-compile-cx

Rebased on top of previous updated patch.
Attachment #8767495 - Attachment is obsolete: true
Attachment #8767495 - Flags: review?(bbouvier)
Attachment #8767510 - Flags: review?(bbouvier)
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.
Attachment #8767491 - Flags: review?(jdemooij) → review+
Comment on attachment 8767492 [details] [diff] [review]
empty-jcx

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

Nice
Attachment #8767492 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 13

a year ago
(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 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)
Attachment #8767489 - Flags: review?(bbouvier) → review+
Comment on attachment 8767490 [details] [diff] [review]
mv-machine-id

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

Nice.
Attachment #8767490 - Flags: review?(bbouvier) → review+
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?
Attachment #8767493 - Flags: review?(bbouvier) → review+
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?
Attachment #8767509 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 18

a year ago
(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 on attachment 8767510 [details] [diff] [review]
rm-compile-cx

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

\o/
Attachment #8767510 - Flags: review?(bbouvier) → review+

Comment 20

a year ago
Pushed by lwagner@mozilla.com:
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)
(Assignee)

Comment 21

a year ago
Created attachment 8768399 [details] [diff] [review]
re-add-always-baseline

Uh whoops, rm-mg-cx made a bool CompileArgs::alwaysBaseline but forgot to actually set it, effectively disabling baseline testing.
Attachment #8768399 - Flags: review?(bbouvier)
Comment on attachment 8768399 [details] [diff] [review]
re-add-always-baseline

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

Doh!
Attachment #8768399 - Flags: review?(bbouvier) → review+

Comment 23

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad84e1c04632
Baldr: initialize CompileArgs::alwaysBaseline (r=bbouvier)

Comment 24

a year ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.