1342641 - Baldr: validate and compile in single pass in compilation threads

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Now that compilation happens in the compilation threads and all the validation is encapsulated in the OpIter, it should be pretty easy to simply delete the first validation pass and set Validate=true for compilation pass.  This should be a nice little compile-time boost, especially for baseline.

Given this change, and assuming WebAssembly.validate() doesn't have to be lightning fast, I think it would significantly simplify OpIter to remove Validate/Output static flags and have OpIter always validate/output.

Comment 1

[Luke Wagner [:luke]]

Attachment: single-pass (WIP)

Here's the Ion portion.  With --no-threads and --thread-count=1 it's a 1.3% speedup (b/c compilation time dwarfs).  Baseline next.

Comment 2

[Luke Wagner [:luke]]

Attachment: single-pass-ion

This required a bit of refactoring because WasmIonCompile was assuming already-valid wasm bytecode in a number of ways.  Now the loop structure matches DecodeFunctionBodyExprs in WasmValidate.cpp.  There was also the hazard of possibly accidentally accepting asm.js opcodes as wasm so to audit this I reordered the opcodes to match WasmBinaryConstants.h so then it was clear that all the asm.js ops at the end have CHECK_ASMJS().  Also added a test for this.

Comment 3

[Luke Wagner [:luke]]

Attachment: single-pass-baseline

This patch starts at an 11% speedup on Unity-Tanks-demo for a single core and gradually decreases to a 6% speedup for 8 cores.

What's neat is that the different between Validate=true vs. Validate=false seems negligible (in the noise).  So we don't need to spend any time in future tiering configurations trying to avoid re-doing validation between baseline/ion.

This patch was much easier than Ion's b/c baseline already was fallibly checking everything (although silently assuming these checks never failed via ~Label() assertions that had to be changed).  The code movement is done to be symmetric with DecodeFunctionBodyExprs() in WasmValidate.cpp and EmitBodyExprs() in WasmIonCompile.cpp.

Comment 4

[Luke Wagner [:luke]]

Attachment: rm-validate-output

As partially measured by the previous comment, Validate and Output have little performance benefit so this patch removes them which significantly simplifies OpIter.  DecodeFunctionBodyExprs() gets a bit grosser; I put the outparams into local blocks for each case so that the inlining compiler can trivially see they are dead and remove the writes.

Fun fact: we weren't checking Output in some places and so "writing" to a nullptr when performing validation.  However the ValueType was Nothing and Nothing has no fields, there were no stores emitted so no segfault.

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 8845731 [details] [diff] [review]
single-pass-baseline

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

Nice!

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8845729 [details] [diff] [review]
single-pass-ion

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

Looks good, thank you!

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +7532,5 @@
>  
>      if (!deadCode_)
>          doReturn(sig.ret(), PopStack(false));
>  
> +    if (!iter_.readFunctionEnd(iter_.end()))

It's a bit heavy to force passing iter_.end() here, since emitBody() returns when we have iter_.done(), which calls into Decoder::done(), which tests that cur_ == end_, doing effectively the same check that we have here. Could we remove this change? or make the bodyEnd parameter optional and check only if it's there?

::: js/src/wasm/WasmIonCompile.cpp
@@ +3213,5 @@
> +    if (!(c))                                                                 \
> +        return false;                                                         \
> +    break
> +
> +#define CHECK_ASMJS(c)                                                        \

Another possibility would be to have an enum value Last_NonAsmJS which would be equal to F64ReinterpretI64; then after decoding the opcode, if it's > Last_NonAsmJS and !isAsmJS(), report the error. This would also have the nice side-effect of enforcing the ordering of opcodes in WasmBinaryConstants. And we wouldn't need to have to worry to use CHECK or CHECK_ASMJS here (but it's an external constraint that's not obviously enforceable, so I'm not too strongly for it, just an idea)

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8845732 [details] [diff] [review]
rm-validate-output

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

Much simpler to read and understand, thanks.

Comment 8

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> > +    if (!iter_.readFunctionEnd(iter_.end()))
> 
> It's a bit heavy to force passing iter_.end() here, since emitBody() returns
> when we have iter_.done(), which calls into Decoder::done(), which tests
> that cur_ == end_, doing effectively the same check that we have here. Could
> we remove this change? or make the bodyEnd parameter optional and check only
> if it's there?

Actually all that changes in the next patch which uses the final Op::End and the readFunctionEnd() to terminate the loop (matching validate and ion).

> Another possibility would be to have an enum value Last_NonAsmJS which would
> be equal to F64ReinterpretI64; then after decoding the opcode, if it's >
> Last_NonAsmJS and !isAsmJS(), report the error. This would also have the
> nice side-effect of enforcing the ordering of opcodes in
> WasmBinaryConstants. And we wouldn't need to have to worry to use CHECK or
> CHECK_ASMJS here (but it's an external constraint that's not obviously
> enforceable, so I'm not too strongly for it, just an idea)

Hmm, that's a good idea to consider.  I think it'd be a bit verbose, since you'd have to pass the Op::X code to the CHECK() macro (or use the 'op' variable which feels a bit too implicit).  Also, while maybe negligible, it'd add another branch to every non-asm.js op.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1dc2fb5beb1
Baldr: validate and compile in a single pass in Ion (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/86f066bb403d
Baldr: validate and compile in a single pass in Rabaldr (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b7c2585ab8
Baldr: make OpIter always validate and produce output (r=bbouvier)

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/b1dc2fb5beb1
https://hg.mozilla.org/mozilla-central/rev/86f066bb403d
https://hg.mozilla.org/mozilla-central/rev/84b7c2585ab8