1619196 - Allow atomic operations on non-shared memories

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P2)

Description

[Lars T Hansen [:lth]]

See https://github.com/WebAssembly/threads/pull/147; for background https://github.com/WebAssembly/threads/issues/144; for spec update https://github.com/WebAssembly/threads/pull/150.

Required for spec compliance.

Atomic ops are allowed on unshared memories with fairly obvious semantics: atomic read/write/rmw do their normal thing, notify does nothing; wait traps.

This is a fairly lightweight change, given our current implementation, which I think is agnostic as to the underlying memory. If we should ever end up with an implementation where we rely on the memory being a SAB (eg because we place a lock in the header page because the hardware does not have appropriate locking) then we'll have a harder time. MIPS32 may have some issues like that but I think the lock is elsewhere. We need to check that when we implement this change.

Comment 1

[Lars T Hansen [:lth]]

This is actually slightly tricky. We're going to want to do this soon because tools are currently emitting code that depends on it. At the same time, we may not be shipping threads on aarch64 initially because Cranelift may be enabled soon but will not have threads support initially. However, the way the feature is currently controlled is that threads support is always enabled but is gated (during opcode reading) on the memory being shared; shared memory is in turn gated on the pref (clever, I'm sure). But with this change, the gate on memory being shared must disappear. Thus we must re-plumb the pref setting into the opcode reader. (We probably must do this even if the situation had not been what it is with Cranelift, and keep it so long as we keep the pref.)

Comment 2

[Lars T Hansen [:lth]]

Attachment: Bug 1619196 - allow atomics on non-shared memory part 1: validation and compilation. r=rhunt

Pretty simple change: remove the validation guards on plain atomic
operations, and distinguish between shared and unshared memory for
wait and notify. Per spec, wait checks for sharedness before checking
for alignment or bounds, and notify returns 0 after checking for
alignment and bounds.

Comment 3

[Lars T Hansen [:lth]]

Attachment: Bug 1619196 - allow atomics on non-shared memory part 2: test cases. r=rhunt

Basically generalize almost all the tests to run with both shared and
unshared memory, removing a number of validation failures. Test
run-time failure in the case of wait() on unshared memory, and that
notify() on unshared memory returns 0.

Depends on D81319

Comment 4

[Lars T Hansen [:lth]]

With this change, the thread ops become always supported, ie, we ship them -- their acceptance no longer depends on shared memory being enabled in about:config. Nor will there be a way to disable them (though an easy fix for that inserts a guard in the big switches to short-circuit the decoding of the atomic ops). Of course, if there is no shared memory then they are not interesting in any way, they are just normal (if expensive) memory accesses.

It is a fun fact that FF release has had atomic.fence support for quite some time, since that instruction was never gated on anything.

In a build where shared memory is enabled by default, cranelift will simply be disabled. In a build where shared memory is disabled by default, but enableable by a switch, cranelift will become disabled when the switch is enabled. Both are a consequence of the existing compiler selection logic, and precisely what we want.

In the near future, on an arm64 system with arm64 cranelift, say, if we want to enable threads then the compiler selection logic must change and must take into account that whether cranelift supports threads or not is platform-dependent. This is also precisely what we want.

(Edit: I think it isn't that easy. If the atomic ops are available regardless of whether shared memory is enabled, then cranelift must recognize them and compile them to regular memory accesses at a minimum, until it has proper support for atomics. That's not so great. Pondering this.)

Comment 5

[Lars T Hansen [:lth]]

(Edit: I think it isn't that easy. If the atomic ops are available regardless of whether shared memory is enabled, then cranelift must recognize them and compile them to regular memory accesses at a minimum, until it has proper support for atomics. That's not so great. Pondering this.)

I think the patch on bug 1648755 will take care of the rest of the problem, it introduces the proper run-time guards during instruction decoding and rejects thread operations if the pref is not enabled.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6844af82924
allow atomics on non-shared memory part 1: validation and compilation. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/191dce06f4b4
allow atomics on non-shared memory part 2: test cases. r=rhunt

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/d6844af82924
https://hg.mozilla.org/mozilla-central/rev/191dce06f4b4

Comment 8

[Florian Scholz (Open Web Docs)]

Just so I get this correct for the docs: bug 1630706 is the JS side of things (and that one ships in Fx79).
This bug is the WASM side of things (and ships in Fx80). (you're not planning to uplift it to Fx79?)

Comment 9

[Lars T Hansen [:lth]]

Florian, I believe that's correct. I don't think an uplift is worth it; wasm atomics are used virtually nowhere at the moment, and unless I'm mistaken the change to allow atomics on unshared wasm memories hasn't shipped in Chrome either, so atomics on unshared memories are actually used nowhere. (Plus for gnarly technical reasons I'd probably have to uplift the bug blocking this one and that one wants a little time to bake.)

Comment 10

[Florian Scholz (Open Web Docs)]

Thanks Lars, makes sense. Updated the docs for bug 1630706 for Firefox 79 now and we can maybe mention this change here later in the Fx80 release notes, for the readers interested in the WebAssembly side of things.

Comment 11

[Chris Mills [:cmills]]

I think I've updated the docs satisfactorily for this one — see https://github.com/mdn/sprints/issues/3498#issuecomment-669860884 for the full details.

I am pretty sure the rel note reads ok, but can you check the mention I added to the MDN text format doc? Is this in the correct context?

Thanks.

Comment 12

[Lars T Hansen [:lth]]

I believe that all looks good. Thanks!