Closed Bug 1329018 Opened 6 years ago Closed 6 years ago

Wasm: Move validation to helper threads


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
firefox53 --- fixed


(Reporter: away, Assigned: away)



(2 files, 2 obsolete files)

Currently the wasm baseline compiler doesn't get any benefit from using more than 4 threads. I suspect that the main thread can only queue up 4x as much work as the main thread itself does per unit time. Luke suggested moving validation to the helper threads in order to improve the helper:main work ratio, and it's working great. I can see improvement up to the full number of cores on my machine:

Threads:       1   2   3   4   5   6   7   8
m-c baseline:  621 442 357 316 334 334 335 330
with patches:  621 441 347 305 292 283 275 268

(Times relative to mozilla-central from 4 January, which includes bug 1320374 and bug 1326302, with CPU affinity masks set to match the thread count)
Boring changes separated out to avoid distracting from the next patch.

I'm not super happy about the extent of this plumbing. Is there a better way to do this?
Assignee: nobody → dmajor
Attachment #8824229 - Flags: review?(luke)
Attachment #8824232 - Flags: review?(luke)
Comment on attachment 8824229 [details] [diff] [review]
Part 1: Add plumbing for compile threads to report validation errors

Review of attachment 8824229 [details] [diff] [review]:

Nicely done!  Only style nits:

::: js/src/vm/HelperThreads.h
@@ +220,5 @@
>          numWasmFailedJobs = 0;
>          return n;
>      }
> +    UniqueChars harvestError(const AutoLockHelperThreadState&) {
> +        return Move(firstError);

nit: could you rename harvestError/setError/firstError to harvestWasmError/setWasmError/firstWasmError?

::: js/src/wasm/WasmGenerator.h
@@ +232,5 @@
>      CompileTaskPtrVector            freeTasks_;
>      UniqueFuncBytesVector           freeFuncBytes_;
>      CompileTask*                    currentTask_;
>      uint32_t                        batchedBytecode_;
> +    UniqueChars*                    error_;

nit: this doesn't feel like part of the implementation of parallel compilation; could you move it up to the top in the "Constant parameters" section right under 'alwaysBaseline_'?

@@ +265,5 @@
>      ~ModuleGenerator();
> +    MOZ_MUST_USE bool init(UniqueModuleEnvironment env,
> +                           const CompileArgs& args,
> +                           UniqueChars* error,

nit: could you move 'error' to be a ctor argument?  The only reason the other ones aren't ctor arguments is because, in AsmJS.cpp, they're not available in time for the ModuleGenerator ctor.

::: js/src/wasm/WasmIonCompile.h
@@ +29,5 @@
>  // Generates very fast code at the expense of compilation time.
>  MOZ_MUST_USE bool
> +IonCompileFunction(CompileTask* task, FuncCompileUnit* unit,
> +                   UniqueChars* error);

nit: in SM, we have the luxury of 100-char column limits so I think this can be on one line (and in WasmIonCompile.cpp and for BaselineCompileFunction).
Attachment #8824229 - Flags: review?(luke) → review+
Comment on attachment 8824232 [details] [diff] [review]
Part 2: Move validation to helper threads

Review of attachment 8824232 [details] [diff] [review]:

Looking great and quite simple.  Two ideas:

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +7770,5 @@
> +    if (!ValidateFunctionBody(task->env(), func.index(), d))
> +        return false;
> +
> +    if (d.currentPosition() != func.bytes().end())
> +        return"function body length mismatch");

It looks like this body length mismatch check was already duplicated between DecodeFunctionBody() in WasmValidate.cpp and DecodeFunctionBody() in WasmCompile.cpp, and now it's in both baseline + ion.  It feels like this check should be factored out of these 3 places into ValidateFunctionBody() by having the caller pass in bodySize.

::: js/src/wasm/WasmCompile.cpp
@@ +46,5 @@
>      if (!mg.startFuncDef(offsetInModule, &fg))
>          return false;
> +    // Skip over the function body; we'll validate it later.
> +    d.rollbackPosition(bodyBegin + bodySize);

Although I suggested this as a quick hack for setting cur_, I think the better use of the API would be to call d.readBytes(bodySize, &bodyBegin) and to do this above, right after the d.readVarU32(&bodySize).  This readBytes() will check bytesRemain() < bodySize for you, so you can remove that now-redundant check from this function.

So with this removed (and with a previous load of refactoring done by bbouvier), FunctionGenerator and the startFuncDef/finishFuncDef dance is now entirely vestigial and we could remove FunctionGenerator and collapse startFuncDef+resize+memcpy+finishFuncDef into a single ModuleGenerator::compileFuncDef().  That sounds like a nice simplification; would you be up for making it?
One more note on removing FunctionGenerator: BaselineCanCompile() would no longer take any arguments and, since its value is invariant over an entire MG, it'd be good to test it once, up front, to determine whether the entire MG would be baseline or ion.
Updated part 1
Attachment #8824229 - Attachment is obsolete: true
Attachment #8824575 - Flags: review+
Moved body length check into ValidateFunctionBody and re-worked the rollback hack.

AsmJS.cpp's FunctionValidator is still using FunctionGenerator, so I couldn't remove it.
Attachment #8824232 - Attachment is obsolete: true
Attachment #8824232 - Flags: review?(luke)
Attachment #8824576 - Flags: review?(luke)
Comment on attachment 8824576 [details] [diff] [review]
Part 2: Move validation to helper threads

Review of attachment 8824576 [details] [diff] [review]:

Perfect, thanks!
Attachment #8824576 - Flags: review?(luke) → review+
Pushed by
Part 1: Wasm: Add plumbing for compile threads to report validation errors. r=luke
Part 2: Wasm: Move function validation to helper threads. r=luke
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
This showed a solid 12% speedup on AngryBots baseline compilation (no impact on ion perf), sweet!
You need to log in before you can comment on or make changes to this bug.