1630706 - Allow Atomics methods on ArrayBuffers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Yulia Startsev [:yulia]]

See: https://github.com/tc39/ecma262/pull/1908

This is to align with wasm allowing atomics to work on non-shared memory.

Most methods work in the obvious deterministic way on ArrayBuffers, with the exceptions of:

• Atomics.wait still throws on ArrayBuffers.
• Atomics.notify always returns 0 on ArrayBuffers.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1630706 - Part 1: Allow non-shared ArrayBuffers for Atomics operations. r=jorendorff!

The TypedArray data extraction had to be moved from AtomicAccess into the
actual operation to account for the case when a TypedArray using inline data
is moved by the GC due to side-effects triggered by type conversions within
the operation. Part 4 contains tests to cover this case.

Because the TypedArray data is no longer passed to the operation, we can't use
it anymore to determine the data type from the SharedMem<T> parameter. Instead
pass an ArrayOps<T> instance to the operation, so we can determine T within
the lambda function. (Templated lambda functions are only available starting
with C++20!) The compiler should be able to optimise away the stack allocation
for this instance, so this won't incur any performance costs.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1630706 - Part 2: Remove addSharedTypedArrayGuard when inlining Atomics operations. r=jandem!

We no longer need to guard against non-shared TypedArrays when inlining
Atomics functions.

The MemoryBarrierRequirement changes for M(Load|Store)UnboxedScalar were
made to keep at least some users for TypedArraySharedness. In practice this
code won't be used anyway, because TemporaryTypeSet::getTypedArraySharedness
always returns UnknownSharedness. Also see bug 1225025.

Depends on D76319

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1630706 - Part 3: Remove no longer used MGuardSharedTypedArray. r=jandem!

After part 2 this MIR node is no longer used.

Depends on D76320

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1630706 - Part 4: Add tests and update existing ones. r=jorendorff!

Update existing tests to run with SharedArrayBuffer and ArrayBuffer.

Also add new tests for cases which weren't previously possible (detached
buffers and TypedArrays using inline storage).

Depends on D76321

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd1ec565b69a
Part 1: Allow non-shared ArrayBuffers for Atomics operations. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/b1ef8096ac14
Part 2: Remove addSharedTypedArrayGuard when inlining Atomics operations. r=jandem
https://hg.mozilla.org/integration/autoland/rev/0f2230cbef64
Part 3: Remove no longer used MGuardSharedTypedArray. r=jandem
https://hg.mozilla.org/integration/autoland/rev/43f7da60f47e
Part 4: Add tests and update existing ones. r=jorendorff

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/dd1ec565b69a
https://hg.mozilla.org/mozilla-central/rev/b1ef8096ac14
https://hg.mozilla.org/mozilla-central/rev/0f2230cbef64
https://hg.mozilla.org/mozilla-central/rev/43f7da60f47e

Comment 7

[Florian Scholz (Open Web Docs)]

Is this enabled by default and ships with Firefox 79?

Comment 8

[André Bargull [:anba]]

Yes, that one is enabled by default and will ship in 79.

Comment 9

[Florian Scholz (Open Web Docs)]

Announced here:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/79#JavaScript

Reference docs updated:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics (and sub pages)

• Previously, we said that Atomic methods would throw with non-shared memory. I removed these statements.
• For notify, it now states that 0 is returned instead of an exception when using non-shared ArrayBuffers.
• For wait, I left the docs as is. It still throws with non-shared memory.
• As a drive-by edit, I also updated to say that Atomics work with BigInts (except for wait and notify)

Please let me know if all this makes sense.

Comment 10

[André Bargull [:anba]]

(In reply to Florian Scholz [:fscholz] (MDN) from comment #9)

Please let me know if all this makes sense.

LGTM, too!