Closed
Bug 1395587
Opened 7 years ago
Closed 7 years ago
Baldr: remove wasmCompilationInProgress pigeonhole
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(3 files, 3 obsolete files)
59.31 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
36.70 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
29.44 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
The main patch; easier than expected.
Attachment #8903231 -
Flags: review?(lhansen)
Assignee | ||
Comment 4•7 years ago
|
||
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•7 years ago
|
||
(Rebased)
Attachment #8903228 -
Attachment is obsolete: true
Attachment #8903228 -
Flags: review?(lhansen)
Attachment #8903458 -
Flags: review?(lhansen)
Assignee | ||
Comment 6•7 years ago
|
||
(Rebased)
Attachment #8903231 -
Attachment is obsolete: true
Attachment #8903231 -
Flags: review?(lhansen)
Attachment #8903459 -
Flags: review?(lhansen)
Comment 7•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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)
Comment 13•7 years ago
|
||
bugherder |
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
Closed: 7 years 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.
Description
•