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

Status: RESOLVED FIXED

(Core :: Javascript: WebAssembly, enhancement)

Description

[Luke Wagner [:luke]]

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).

Comment 1

[Luke Wagner [:luke]]

Attachment: enable-atomics

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?

Comment 2

[Lars T Hansen [:lth]]

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.

Comment 3

[Luke Wagner [:luke]]

... or in that follow-up patch we could remove ENABLE_SHARED_ARRAY_BUFFER.  Any reason not to do that?

Comment 4

[Luke Wagner [:luke]]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6adb19445d38
Baldr: build atomic ops by default (still disabled by pref) (r=lth)

Comment 6

[Lars T Hansen [: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

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/6adb19445d38

Comment 8

[Luke Wagner [:luke]]

Attachment: rm-ifdefs

Follow-up removing the #defines altogether as discussed.

Comment 9

[Lars T Hansen [:lth]]

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 :)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4b541b90438b