1330891 - Baldr: don't lose error during parallel compilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: fix-start-off-thread-wasm-compile

After bug 1329018, validation now happens in helper threads.  Before, the only real error that could happen in a helper thread was OOM.  This patch removes a check that used to make sense under this assumption but now loses the validation error message (triggering an "out of memory" error to be reported).  This isn't observed in all our tests because mostly they fit into a single batch.

Comment 1

[Luke Wagner [:luke]]

Attachment: simplify-wasm-gen

While I was here, this patch makes some simplifications based on the fact that bug 1329018 disabled the wasm baseline for asm.js.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8826489 [details] [diff] [review]
fix-start-off-thread-wasm-compile

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

So iiuc, when there is an error in validation on an helper thread, StartOffThreadCompile returns false before we get a chance to harvest the error in finishOutstandingTask.

It seems important to note this won't trigger compilation of the batch passed in StartOffThreadCompile, since GTS::canStartWasmCompile() will eagerly return false if there's been a wasm error.

Looks good to me.

rs=me to change the 2-spaces indents into 4-spaces indents at the following locations:
- vm/HelperThreads.h: setWasmError()
- wasm/WasmGenerator.cpp: finishOutstandingTask(), below if (error_)
- wasm/WasmIonCompile.cpp: in IonCompileFunction(), below if (!ValidateFunctionBody ...)

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8826490 [details] [diff] [review]
simplify-wasm-gen

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

So either ion compiles all the batches, or the baseline does, for a given module? If so, you can even go the extra mile.

- the compileMode_ field in FuncCompileUnit could be hoisted to the CompileTask
- then we could invert the for/switch in wasm::CompileFunction
- update the directives.txt file in jit-tests/tests/asm.js to remove the directive for testing also baseline
- and also touch this line: http://searchfox.org/mozilla-central/source/js/src/tests/lib/jittests.py#141
- now since a batch can't contain ion and baseline compiled functions, then we could have two thresholds for batch compilation, instead of one threshold and a scaling factor

Of course, if this is supposed to be a temporary situation, we should not do all of this right now, and the patch as is looks good and r=me. Otherwise, I'd like to see another version.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef05499df1c3
Baldr: don't lose error during parallel compilation (r=bbouvier)

Comment 5

[Luke Wagner [:luke]]

Attachment: simplify-wasm-gen

Great list, all applied.

Comment 6

[Phil Ringnalda (:philor)]

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

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8826746 [details] [diff] [review]
simplify-wasm-gen

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

Thanks!

::: js/src/tests/lib/jittests.py
@@ -137,5 @@
>          t.allow_overrecursed = self.allow_overrecursed
>          t.valgrind = self.valgrind
>          t.tz_pacific = self.tz_pacific
>          t.test_also_noasmjs = self.test_also_noasmjs
> -        t.test_also_wasm_baseline = self.test_also_noasmjs

It should just copy the value from self instead, not remove it.

::: js/src/wasm/WasmGenerator.cpp
@@ +996,3 @@
>      }
>  
> +    batchedBytecode_ += funcBytecodeLength;

I think we need to keep the CheckedInt and assertions here (if the funcBytecodeLength is very big, close to UINT32_MAX for instance).

Comment 8

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> > +    batchedBytecode_ += funcBytecodeLength;
> 
> I think we need to keep the CheckedInt and assertions here (if the
> funcBytecodeLength is very big, close to UINT32_MAX for instance).

But it can't be because that would mean that the overall module's size was bigger than MaxModuleBytes.  I can add an assert, though, to make this assumption clear.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #8)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> > > +    batchedBytecode_ += funcBytecodeLength;
> > 
> > I think we need to keep the CheckedInt and assertions here (if the
> > funcBytecodeLength is very big, close to UINT32_MAX for instance).
> 
> But it can't be because that would mean that the overall module's size was
> bigger than MaxModuleBytes.  I can add an assert, though, to make this
> assumption clear.

Right; an assert would be good here then, thank you.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b353d014c22b
Baldr: simplify ModuleGenerator (r=bbouvier)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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