Build with wasm thread ops by default (still disabled by default by pref)

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 months ago
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

5 months 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 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

5 months ago
... or in that follow-up patch we could remove ENABLE_SHARED_ARRAY_BUFFER.  Any reason not to do that?
(Assignee)

Comment 4

5 months ago
(Confirmed dependency on shared memory being present (as required by actual spec semantics) in atomic op validation.)

Comment 5

5 months ago
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)
(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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6adb19445d38
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(Assignee)

Comment 8

5 months ago
Posted patch rm-ifdefsSplinter Review
Follow-up removing the #defines altogether as discussed.
Attachment #9025786 - Flags: review?(lhansen)
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

5 months 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)
You need to log in before you can comment on or make changes to this bug.