Closed
Bug 1504288
Opened 2 years ago
Closed 2 years ago
Build with wasm thread ops by default (still disabled by default by pref)
Categories
(Core :: Javascript: WebAssembly, enhancement)
Core
Javascript: WebAssembly
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(2 files)
1.47 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
25.70 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
This allows us or users / demoers to use wasm threads by setting javascript.options.shared_memory=true without additionally requiring a NIGHTLY_BUILD. Ideally we'd land this change on both trunk (65) and Beta (64).
![]() |
Assignee | |
Comment 1•2 years ago
|
||
Here's the minimal nightly/beta patch to build thread ops always. Lars: is there any reason not to immediately follow-up with a patch to remove ENABLE_WASM_THREAD_OPS altogether, or might we want the ability to stop building these in the future?
Assignee: nobody → luke
Attachment #9024866 -
Flags: review?(lhansen)
Comment 2•2 years ago
|
||
Comment on attachment 9024866 [details] [diff] [review] enable-atomics Review of attachment 9024866 [details] [diff] [review]: ----------------------------------------------------------------- > is there any reason not to immediately follow-up with a patch to remove ENABLE_WASM_THREAD_OPS altogether, or might we want the ability to stop building these in the future? I can't think of any reason. The threads proposal is pretty much done, and with last night's resolution about atomics.notify / atomics.wake not changing its signature, I don't see anything breaking happening. From what I can tell (it will be useful for you to verify this) the atomic ops are properly gated by the reader methods in WasmOpIter: they all depend on the module having a shared memory. Presumably since we're building with shared memory by default on all channels we are confident that shared memory is properly gated. It follows that it's safe to remove the threads ifdef. Of course, until we remove the shared memory ifdef we depend on the atomic wasm ops building properly even if shared memory is not available. So another option is to just rename ENABLE_WASM_THREAD_OPS as ENABLE_SHARED_ARRAY_BUFFER.
Attachment #9024866 -
Flags: review?(lhansen) → review+
![]() |
Assignee | |
Comment 3•2 years ago
|
||
... or in that follow-up patch we could remove ENABLE_SHARED_ARRAY_BUFFER. Any reason not to do that?
![]() |
Assignee | |
Comment 4•2 years ago
|
||
(Confirmed dependency on shared memory being present (as required by actual spec semantics) in atomic op validation.)
Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6adb19445d38 Baldr: build atomic ops by default (still disabled by pref) (r=lth)
Comment 6•2 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #3) > ... or in that follow-up patch we could remove ENABLE_SHARED_ARRAY_BUFFER. > Any reason not to do that? Yeah, I did think about this too... By the above argument the gating is sufficient (although the code in wasm::DeserializeModule makes me nervous, even if it's probably OK). If there's a risk to the user (social engineering, bugs in the gating) it's there now, so we add none. And if we need to do a hard disable we should be able to make a point fix in getSharedMemoryAndAtomicsEnabled(), we don't need the ifdef for that. rs=me I guess.
Comment 7•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6adb19445d38
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
![]() |
Assignee | |
Comment 8•2 years ago
|
||
Follow-up removing the #defines altogether as discussed.
Attachment #9025786 -
Flags: review?(lhansen)
Comment 9•2 years ago
|
||
Comment on attachment 9025786 [details] [diff] [review] rm-ifdefs Review of attachment 9025786 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TestingFunctions.cpp @@ -668,2 @@ > bool isSupported = wasm::HasSupport(cx); > # ifdef ENABLE_WASM_CRANELIFT Undent this line and the matching endif. ::: js/src/shell/js.cpp @@ -11137,1 @@ > "(default: off, on to enable)" Remove this line, or the help message will be very confusing :)
Attachment #9025786 -
Flags: review?(lhansen) → review+
Comment 10•2 years ago
|
||
Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b541b90438b Baldr: remove shared memory and wasm thread ops #ifdefs (keeping default-off pref) (r=lth)
Comment 11•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4b541b90438b
You need to log in
before you can comment on or make changes to this bug.
Description
•