Closed Bug 1395587 Opened 7 years ago Closed 7 years ago

Baldr: remove wasmCompilationInProgress pigeonhole

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(3 files, 3 obsolete files)

Since day 1, we've only been able to have at most one parallel wasm compilation in progress.  For a while, there was some dining-philosopher deadlock hazards, but those have since been removed.  That leaves only the issue that there is only "finished" queue (and related state) that that is logically owned by a single compilation.  Pushing this state into the compilation is pretty easy and reduces helper-thread-lock contention to boot.
Attached patch shrink-mg-interface (obsolete) — Splinter Review
This is mostly unrelated cleanup patch to shrink the ModuleGenerator interface in preparation for some later patches.  The patch also initializes some things in the ctor instead of dynamically in init().
Assignee: nobody → luke
Attachment #8903226 - Flags: review?(lhansen)
Attached patch rm-func-generator (obsolete) — Splinter Review
This patch removes FunctionGenerator and the whole system of passing Bytes arrays to and from tasks.  Passing copies of function bodies is a bit silly b/c the byte array from which they are copied is kept alive so pointers into the buffer could be easily used instead.  The original motivation was b/c I thought streaming would want to pass copies and I wasn't sure about bytecodes' lifetime.  But now the scheme I'm planning for bug 1347644 will maintain a bytecode array that can be pointed to by tasks.  So pretty big simplification and very mild speedup.
Attachment #8903228 - Flags: review?(lhansen)
Attached patch allow-multiple-parallel (obsolete) — Splinter Review
The main patch; easier than expected.
Attachment #8903231 - Flags: review?(lhansen)
Actually, I found an even cleaner cut point: hoisting the mode, tier and debug states into ModuleEnvironment to which everything can refer.  Simplifies the other patches too, for an overall diffstat of
  431 insertions(+), 795 deletions(-)
Attachment #8903226 - Attachment is obsolete: true
Attachment #8903226 - Flags: review?(lhansen)
Attachment #8903457 - Flags: review?(lhansen)
(Rebased)
Attachment #8903228 - Attachment is obsolete: true
Attachment #8903228 - Flags: review?(lhansen)
Attachment #8903458 - Flags: review?(lhansen)
(Rebased)
Attachment #8903231 - Attachment is obsolete: true
Attachment #8903231 - Flags: review?(lhansen)
Attachment #8903459 - Flags: review?(lhansen)
Comment on attachment 8903457 [details] [diff] [review]
shrink-mg-interface

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

::: js/src/wasm/WasmIonCompile.cpp
@@ +190,5 @@
>              return false;
>          if (!newBlock(/* prev */ nullptr, &curBlock_))
>              return false;
>  
> +        for (ABIArgIter<ValTypeVector> i(args); !i.done(); i++) {

Seems a little unmotivated?  But no real objection.
Attachment #8903457 - Flags: review?(lhansen) → review+
Comment on attachment 8903458 [details] [diff] [review]
rm-func-generator

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

Nice.
Attachment #8903458 - Flags: review?(lhansen) → review+
(In reply to Lars T Hansen [:lth] from comment #7)
> Seems a little unmotivated?  But no real objection.

Yeah, the only (light) motivation was removing the 3 seemingly-unrelated typedefs from the top of WasmGenerator.h.
Comment on attachment 8903459 [details] [diff] [review]
allow-multiple-parallel

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

::: js/src/vm/HelperThreads.cpp
@@ +1184,5 @@
>  bool
>  GlobalHelperThreadState::canStartWasmCompile(const AutoLockHelperThreadState& lock,
>                                               wasm::CompileMode mode)
>  {
> +    if (wasmWorklist(lock, mode).empty())

OK.  Took me a while to believe that the removal of the wasmFailed() guard here is correct & desirable, but so far as I can tell, relatively soon after a failure is registered we will return false from finishOutstandingTask() and things will unravel quickly from there, and that seems good enough.

::: js/src/wasm/WasmGenerator.h
@@ +85,5 @@
> +struct CompileTaskState
> +{
> +    ConditionVariable    failedOrFinished;
> +    CompileTaskPtrVector finished;
> +    uint32_t             numFailed;

Not obvious to me that 'numFailed' is properly initialized.  Neither ExclusiveData nor AlignedStorage2<T> which it uses seems to guarantee that.
Attachment #8903459 - Flags: review?(lhansen) → review+
(In reply to Lars T Hansen [:lth] from comment #10)
> Not obvious to me that 'numFailed' is properly initialized.  Neither
> ExclusiveData nor AlignedStorage2<T> which it uses seems to guarantee that.

Correctamundo!  I had actually run into this in local testing with later patches and forgot to repost the updated patch.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7189690845fb
Baldr: shrink the ModuleGenerator interface (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/32df4db6c150
Baldr: remove FunctionGenerator (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd70f5f356b
Baldr: allow multiple concurrent, parallel compilations (r=lth)
You need to log in before you can comment on or make changes to this bug.