Baldr: remove wasmCompilationInProgress pigeonhole

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

10 months ago
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.
(Assignee)

Comment 1

10 months ago
Created attachment 8903226 [details] [diff] [review]
shrink-mg-interface

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)
(Assignee)

Comment 2

10 months ago
Created attachment 8903228 [details] [diff] [review]
rm-func-generator

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)
(Assignee)

Comment 3

10 months ago
Created attachment 8903231 [details] [diff] [review]
allow-multiple-parallel

The main patch; easier than expected.
Attachment #8903231 - Flags: review?(lhansen)
(Assignee)

Comment 4

10 months ago
Created attachment 8903457 [details] [diff] [review]
shrink-mg-interface

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)
(Assignee)

Comment 5

10 months ago
Created attachment 8903458 [details] [diff] [review]
rm-func-generator

(Rebased)
Attachment #8903228 - Attachment is obsolete: true
Attachment #8903228 - Flags: review?(lhansen)
Attachment #8903458 - Flags: review?(lhansen)
(Assignee)

Comment 6

10 months ago
Created attachment 8903459 [details] [diff] [review]
allow-multiple-parallel

(Rebased)
Attachment #8903231 - Attachment is obsolete: true
Attachment #8903231 - Flags: review?(lhansen)
Attachment #8903459 - Flags: review?(lhansen)

Comment 7

10 months ago
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 8

10 months ago
Comment on attachment 8903458 [details] [diff] [review]
rm-func-generator

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

Nice.
Attachment #8903458 - Flags: review?(lhansen) → review+
(Assignee)

Comment 9

10 months ago
(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 10

10 months ago
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+
(Assignee)

Comment 11

10 months ago
(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.

Comment 12

10 months ago
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)
https://hg.mozilla.org/mozilla-central/rev/7189690845fb
https://hg.mozilla.org/mozilla-central/rev/32df4db6c150
https://hg.mozilla.org/mozilla-central/rev/3bd70f5f356b
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.