Normative: Allow Atomics methods on ArrayBuffers

[syg]

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.

[ljharb]

this part is normative, but not part of the rename; can it be separated into its own commit?

[syg]

Preferably not because the rename would be misleading without changing this line. The only reason it's renamed is because the AO now applies to all TypedArrays, not just SAB-backed ones.

[ljharb]

Sure, but the new name isn't wrong even if it only applies to shared array buffers.

[syg]

I don't understand why I would land a renaming PR that is less clear, separately.

Like I said, I wouldn't rename this at all without the normative change. And since there's no mechanical advantage to having a separate commit here I would not split it.

[ljharb]

right, but it makes the commit separable, even if it's in the same PR. iow, this commit is normative, not editorial, as it's currently labelled.

(iow, you do have a separate commit here, labelled editorial, so if there's no mechanical advantage, why would you have the commit be separate?)

[syg]

Are you saying you'd prefer that if this were a separate commit at all, if that separate commit must be normative, it should just be folded into the other one? That's fine with me. It ended up being a separate commit because it was a logical chunk of work.

[ljharb]

mainly just that i'd prefer individual commits be atomic and labelled properly; if the intention is to fold them all up, then it doesn't matter.

[syg]

Addressed review and rebased.

[rwaldron]

@syg I'm going to add this to the end of the priority testing queue

[syg]

@erights raised the following good question: "Are different optimization allowed between a single-agent program that uses Atomics on a non-shared SAB and a single-agent program that uses Atomics on an AB?"

If one takes different optimizations to mean different observable behavior, then the answer is no. On paper, since Atomics on ABs do not introduce SeqCst events (or, indeed, any events), an implementation is allowed to do things to Atomics on ABs that it's not allowed to do to Atomics on SABs, like coalesce writes, reorder them, etc. But it's only allowed to do that if it can prove that such changes are non-observable inside that single agent. SABs need extra guidance because observability is extended to the agent cluster -- proving single agent (non-)observability is insufficient.

However, if one takes different optimizations to mean can different codegen be observed by something outside of the spec's normative control like devtools, then the answer is yes. This is something the spec does not legislate anyhow.

In practice, I reckon the answer to both interpretations above is no. I see no reason why compiler authors would go the length of writing optimizations for Atomics used on ABs instead of reusing the same optimizations as Atomics used on SABs.

Cc @tlively and @lars-t-hansen for thoughts from the wasm side.

[tlively]

I would expect different (semantics-preserving) optimizations to be allowed for the reasons you mention. I assume doing going out of the way to do extra optimizations of unshared atomic operations would be low priority for implementors since that is a rather niche situation. On the other hand, it might not be difficult to arrange for a compiler to notice that the atomics are not shared early in the pipeline and lower them to nonatomic operations that can subsequently be optimized using the same code paths as normal reads and writes.

[erights]

To be clear, I was asking, not advocating. If the observable behavior is already equivalent, then we're done.

I do not think that the spec for AB should be loosened in any way, to enable optimizations that we're not currently doing for AB. Such equivalence of allowed behavior is not attractive to me, even if everyone else were happy with it.

Let me ask a different question: If the allowed behavior is not equivalent: Is the allowed observable behavior on AB a subset of the allowed observable behavior on SAB? If not, I'd like to understand the difference. Until I do understand the difference, I'm still not yet advocating that anything be changed.

[syg]

Let me ask a different question: If the allowed behavior is not equivalent: Is the allowed observable behavior on AB a subset of the allowed observable behavior on SAB? If not, I'd like to understand the difference.

The allowed observable single-thread behavior of AB and the allowed observable behavior of non-shared SAB are the same. What differs is what codegen is allowed, but since there are no concurrent ways to observe the SAB since it's non-shared, the differently generated code have the same observable behavior.

The only way you might observe a difference is a non-normative host-implemented observation utility like a debugger.

tl;dr The observable behavior is already equivalent given how people currently understand how to optimize single-threaded JS, so I think we're done.

[lars-t-hansen]

I would expect different (semantics-preserving) optimizations to be allowed for the reasons you mention. I assume doing going out of the way to do extra optimizations of unshared atomic operations would be low priority for implementors since that is a rather niche situation. On the other hand, it might not be difficult to arrange for a compiler to notice that the atomics are not shared early in the pipeline and lower them to nonatomic operations that can subsequently be optimized using the same code paths as normal reads and writes.

I agree.

[anba]

This seems to be missing detached array buffer checks.

[anba]

The Set part of GetModifySetValueInBuffer for non-shared buffers seems to be missing, too. And as currently written, it returns the result after applying op for non-shared buffers, whereas for shared buffers, the original read value is returned.

[syg]

Yeah this PR is very wrong. Thanks @anba, will fix and rebase.

[syg]

Rebased and fixed the issues @anba pointed out.

[ljharb]

it seems weird for me (both before and after this change) that an abstract operation called "validate" returns something. is there a better name for this abstract op that conveys that?

[syg]

I'd consider that outside the scope of this PR.

One option is to tack on another verb, like ValidateIntegerTypedArrayAndGetBuffer and ValidateAtomicAccessAndGetIndex, that's pretty verbose.

Another option is to ToValidIntegerTypedArrayBuffer, but it might be hard to understand the intent of the AO without reading what it does.

In any case, we should do those as a follow-on PR.

[ljharb]

Maybe GetSharedIntegerTypedArrayBuffer?

I'm not sure it's outside the scope; you're renaming the op in this PR, so any concerns about the name seem thus in scope of the PR.

[syg]

It no longer applies to shared buffers only, which is why this renames that op to delete the word "shared". It doesn't follow to me that a complete rename is also in scope.

Also GetIntegerTypedArrayBuffer doesn't capture most of what the AO does: validating the kind of TypedArray the argument is depending on the parameters.

[anba]

We need to have additional detached buffer checks:

• AtomicReadModifyWrite, after ToBigInt/ToInteger.
• Atomics.compareExchange, after ToBigInt/ToInteger.
• Atomics.store, after ToBigInt/ToInteger.
• AtomicLoad, after ValidateAtomicAccess.
• (I'm not sure if we also want to check for detached buffers in Atomics.notify, because we're returning 0 anyway.)

ValidateAtomicAccess also needs to a slight modification: Step 3 ("Let length be typedArray.[[ArrayLength]]") should be moved before step 2 ("Let accessIndex be ? ToIndex(requestIndex)."), because we don't want to read [[ArrayLength]] on a typed array with a detached buffer. I'm not sure that rule is documented anywhere, but IIRC Allen made sure this never happens when he wrote the ES6 spec. And it also makes things a bit easier for implementors, because some implementations return 0 for [[ArrayLength]] with a detached buffer, but per spec the original length should be returned.

[syg]

• AtomicReadModifyWrite, after ToBigInt/ToInteger.
• Atomics.compareExchange, after ToBigInt/ToInteger.
• Atomics.store, after ToBigInt/ToInteger.
• AtomicLoad, after ValidateAtomicAccess.

For these, I'll delay the detached check to be as late as possible. I don't see a good reason to check detachment early, do you, @anba?

• (I'm not sure if we also want to check for detached buffers in Atomics.notify, because we're returning 0 anyway.)

I think, out of consistency of errors, yes. Trying to access a detached buffer is an egregious enough behavior that we shouldn't hide it even if we could return an answer for Atomics.notify.

ValidateAtomicAccess also needs to a slight modification: Step 3 ("Let length be typedArray.[[ArrayLength]]") should be moved before step 2 ("Let accessIndex be ? ToIndex(requestIndex)."), because we don't want to read [[ArrayLength]] on a typed array with a detached buffer. I'm not sure that rule is documented anywhere, but IIRC Allen made sure this never happens when he wrote the ES6 spec. And it also makes things a bit easier for implementors, because some implementations return 0 for [[ArrayLength]] with a detached buffer, but per spec the original length should be returned.

It looks like this invariant is already broken in the following place. IIEO [[DefineProperty]] calls IsValidIntegerIndex, which gets [[ArrayLength]] without checking if the underlying buffer is detached. It's not until step 3.b.vi.2, when IntegerIndexedElementSet is called, that the detachment check is done. I think the property you said holds everywhere else.

Noted about some implementations returning 0 for [[ArrayLength]] for views with detached buffers. (V8 is incidentally buggy for [[DefineProperty]]: we throw a RangeError instead of a TypeError.)

[syg]

I'm convinced by the consistency argument that most AB methods already do multiple detach checks. I've added a second detach check before [[ArrayLength]] where appropriate. Notably, AtomicLoad, Atomics.notify, and Atomics.wait only have a single check.

[anba]

I'd prefer to have the first detached array buffer check in ValidateIntegerTypedArray instead of ValidateAtomicAccess. Similar to how ValidateTypedArray works.

---

We could even call ValidateTypeArray here, if we want to reduce some duplicate steps:

1. If waitable is not present, set waitable to false.
2. Let buffer be ? ValidateTypedArray(typedArray).
3. Let typeName be typedArray.[[TypedArrayName]].
4. Let type be the Element Type value in Table 61 for typeName.
5. If waitable is true, then
   1. If typeName is not "Int32Array" or "BigInt64Array", throw a TypeError exception.
6. Else,
   1. If ! IsUnclampedIntegerElementType(type) is false and ! IsBigIntElementType(type) is false, throw a TypeError exception.
7. Return buffer.

[syg]

I thought about that, but then we would have 2-3 detach checks for some Atomics on ABs:

1. In ValidateIntegerTypedArray
2. In ValidateAtomicAccess, after ? ToIndex, to preserve the property you want of not getting [[ArrayLength]] on detached TAs.
3. Before doing the actual operation, depending on the op.

Edit: In #1908 (comment), my being convinced of having multiple detach checks doesn't take away from minimizing the number of checks.

[anba]

2\. In ValidateAtomicAccess, after ? ToIndex, to preserve the property you want of not getting [[ArrayLength]] on detached TAs.

That one can be changed without a detached check by simply moving the access to [[ArrayLength]] before calling ToIndex. Basically what implementations like SpiderMonkey or V8 already have to do.

[syg]

That one can be changed without a detached check by simply moving the access to [[ArrayLength]] before calling ToIndex. Basically what implementations like SpiderMonkey or V8 already have to do.

That seems fine, I'll do that.

I think misunderstood your original implementation-freedom point: you were saying even though [[ArrayLength]] isn't zeroed on buffer detaching, implementations might want to do that. I interpreted that to mean you wanted ValidateAtomicAccess to throw a TypeError if the buffer were detached. While what you were saying was it's fine to throw a RangeError on detached buffer, as long as it's explicit in the spec algorithm that the value of [[ArrayLength]] is manually cached in a local before possible detachment.

[anba]

Yes, exactly that. Sorry if I couldn't make it clear enough!