Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: Add resizable ArrayBuffer and growable SharedArrayBuffer #3116

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

syg
Copy link
Contributor

@syg syg commented Jul 7, 2023

Major editorial differences from the spec draft:

  • The idempotent byte length getter machinery is replaced with Integer-Indexed Object With Buffer Witness Record and DataView With Buffer Witness Record values instead. These records cache the byte length ahead of time. Because shared memory events are observable, there should only be a single load event of growable SAB byte length until the next point where user code may be called.

  • IsResizableArrayBuffer is negated and renamed to IsFixedLengthArrayBuffer, because it covers both resizable ABs and growable SABs.

cc @anba

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
webkit-commit-queue pushed a commit to Constellation/WebKit that referenced this pull request Jul 11, 2023
https://bugs.webkit.org/show_bug.cgi?id=259076
rdar://112041092

Reviewed by Mark Lam.

Change the order of detach check according to the latest spec[1],
but this only changes the type of error (from RangeError to TypeError).

[1]: tc39/ecma262#3116

* JSTests/stress/arraybuffer-resizable-resize-update.js: Added.
(shouldThrow):
* Source/JavaScriptCore/runtime/JSArrayBufferPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/265935@main
@anba
Copy link
Contributor

anba commented Jul 11, 2023

IsArrayBufferViewOutOfBounds is no longer present in this PR. Will it be added back for structured cloning or does the HTML spec need to change?

@syg
Copy link
Contributor Author

syg commented Jul 11, 2023

IsArrayBufferViewOutOfBounds is no longer present in this PR. Will it be added back for structured cloning or does the HTML spec need to change?

Oops, omission is my mistake. I'll add it back.

Edit: This is now added back.

mnutt pushed a commit to movableink/webkit that referenced this pull request Jul 18, 2023
https://bugs.webkit.org/show_bug.cgi?id=259076
rdar://112041092

Reviewed by Mark Lam.

Change the order of detach check according to the latest spec[1],
but this only changes the type of error (from RangeError to TypeError).

[1]: tc39/ecma262#3116

* JSTests/stress/arraybuffer-resizable-resize-update.js: Added.
(shouldThrow):
* Source/JavaScriptCore/runtime/JSArrayBufferPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/265935@main
@bathos
Copy link
Contributor

bathos commented Jul 18, 2023

Is there still a link to the rendered HTML spec available?

(The proposal repo points to the old rendered spec link, which points to this PR).

@syg
Copy link
Contributor Author

syg commented Jul 18, 2023

Is there still a link to the rendered HTML spec available?

You can scroll to the bottom of the checks box and click the Details link next to "Begin.com build preview".

@bathos
Copy link
Contributor

bathos commented Jul 18, 2023

You can scroll to the bottom of the checks box and click the Details link next to "Begin.com build preview".

Thanks! Just to confirm I found the right thing: under “checks” I saw “build PR preview” and clicking that took me to a view with a downloadable “out” artifact. That’s what you’re referring to, right?

screenshot of checks → build PR preview view

@michaelficarra
Copy link
Member

@bathos No you've clicked the wrong one. Scroll down. You're looking for
image

@bathos
Copy link
Contributor

bathos commented Jul 18, 2023

No you've clicked the wrong one. Scroll down. You're looking for

Ohhhh “the checks box” is right there on this page directly above the box I’m typing in, oops. Sorry for conscripting y’all for basic tech support work, I see it now!

@anba
Copy link
Contributor

anba commented Jul 19, 2023

If you're only interested in the changes, you can also use https://arai-a.github.io/ecma262-compare/?pr=3116.

1. Let _O_ be the *this* value.
1. Perform ? RequireInternalSlot(_O_, [[ArrayBufferMaxByteLength]]).
1. If IsSharedArrayBuffer(_O_) is *false*, throw a *TypeError* exception.
1. Let _newByteLength_ be ? ToIntegerOrInfinity(_newLength_).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Let _newByteLength_ be ? ToIntegerOrInfinity(_newLength_).
1. Let _newByteLength_ be ? ToIndex(_newLength_).

Same ToIndex suggestion applies here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, ToIndex is better here. I'm going to say this is editorial. I suppose it's technically observable from the perspective of the host hook, but since the hook already says it must throw if the new length is < current length, and a negative length will always be < current length, it's a non-normative change from the perspective of programs.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
Copy link
Contributor

@anba anba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%TypedArray%.prototype.toLocaleString needs to be updated to call IntegerIndexedObjectLength.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
@@ -39920,8 +40074,8 @@ <h1>Properties of the %TypedArray% Prototype Object</h1>
<h1>%TypedArray%.prototype.at ( _index_ )</h1>
<emu-alg>
1. Let _O_ be the *this* value.
1. Perform ? ValidateTypedArray(_O_).
1. Let _len_ be _O_.[[ArrayLength]].
1. Let _iieoRecord_ be ? ValidateTypedArray(_O_, ~SeqCst~).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may make sense to add a note somewhere to explain that all %TypedArray%.prototype synchronise the length, even when doing only bounds checking.

Given an initially zero length growable SharedArrayBuffer sab when a concurrent thread (atomically) grows the length to 4 bytes and writes 123 at offset zero. The following results should be possible:

Operation Condition Result
new Int32Array(sab)[0] New length not read undefined for out-of-bounds
new Int32Array(sab)[0] New content not read 0
new Int32Array(sab)[0] New length and content read 123
new Int32Array(sab).at(0) New content not read 0
new Int32Array(sab).at(0) New content read 123
Atomics.load(new Int32Array(sab), 0) New length not read RangeError for out-of-bounds
Atomics.load(new Int32Array(sab), 0) New length read 123

When the operations are performed in exactly this order:

Thread 1 Thread 2
ta = new Int32Array(sab) -
- sab.grow(4)
- Atomics.store(new Int32Array(sab), 0, 123)
ta[0] or ta.at(0) or Atomics.load(ta, 0) -

IIUC get %TypedArray%.prototype.length was repurposed as a synchronisation point for reading the (byte-)length. And %TypedArray%.prototype methods generally act as if get %TypedArray%.prototype.length is called and therefore all %TypedArray%.prototype methods now read the length with SeqCst. This also ensures %TypedArray%.prototype methods and user-implemented functions, which call get %TypedArray%.prototype.length, have the same semantics.

This isn't explained anywhere in the actual spec text though, which is likely to cause confusion in the future for people how don't know all these background information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I'll try to capture this in the "for implementers" section. The design principle was "implicit loads of the length are not synchronizing; explicit loads of the length are synchronizing".

And from that we derive the following positions:

  • Explicitly accessing the .length or .byteLength properties are synchronizing.
  • Built-in methods need to check whether TypedArrays are out-of-bounds or detached. This conceptually feels like an explicit access of the length (and may indeed be an explicit access of the length in self-hosted implementations), and so such loads of the length are synchronizing.
  • Bounds checking for element access (i.e. integer-indexed property access, ta[idx]) is not synchronizing, because it feels implicit.

Copy link
Contributor Author

@syg syg Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, the "implicit loads are not synchronizing" thing is desirable because it is desirable to allow the optimization of hoisting bounds checks of TypedArray accesses out of loop bodies. If it were specced that all element access must have an SC load of the length, then that optimization would no longer be allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote up some guidelines in 30a7359, PTAL

@jmdyck
Copy link
Collaborator

jmdyck commented Jul 28, 2023

Rather than suggest a bunch of changes here, I've created a PR against this PR's branch: syg#1

spec.html Outdated Show resolved Hide resolved
@jmdyck
Copy link
Collaborator

jmdyck commented Jul 29, 2023

"Properties of SharedArrayBuffer Instances" says:

SharedArrayBuffer instances each have ... an [[ArrayBufferByteLength]] internal slot.

but that's no longer true.

Some do, but others have [[ArrayBufferByteLengthData]] and [[ArrayBufferMaxByteLength]].

spec.html Outdated
1. Let _byteLengthDelta_ be _newByteLength_ - _currentByteLength_.
1. If it is impossible to create a new Shared Data Block value consisting of _byteLengthDelta_ bytes, throw a *RangeError* exception.
1. NOTE: No new Shared Data Block is constructed and used here. The observable behaviour of growable SharedArrayBuffers is specified by allocating a max-sized Shared Data Block at construction time, and this step is captures the requirement that implementations that run out of memory must throw a *RangeError*.
1. Let _readByteLengthRawBytes_ be AtomicCompareExchangeInSharedBlock(_byteLengthBlock_, 0, 8, _currentByteLengthRawBytes_, _newByteLengthRawBytes_).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that we inline the Element Size of BigUint64 as 8 in a couple places. Should we refer to the table each time or is it okay to duplicate that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to duplicate because I can't see a possible future where BigUint64 would have a different element size.

spec.html Outdated Show resolved Hide resolved
ioannad added a commit to syg/spec that referenced this pull request Aug 23, 2023
@syg syg added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Aug 23, 2023
@michaelficarra
Copy link
Member

The reference to ValidateTypedArray in 23.2.3.31 %TypedArray%.prototype.toLocaleString needs to be updated to specify the order.

@syg
Copy link
Contributor Author

syg commented Sep 15, 2023

Rebased and squashed.

@@ -42851,17 +43243,33 @@ <h1>
AllocateSharedArrayBuffer (
_constructor_: a constructor,
_byteLength_: a non-negative integer,
optional _maxByteLength_: a non-negative integer or ~empty~,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's only one invocation of AllocateSharedArrayBuffer, and it supplies an arg for the _maxByteLength_ parameter, so there's no need for _maxByteLength_ to be optional. (But maybe it's optional to echo AllocateArrayBuffer?)

Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not qualified to review the claims about hardware instructions, memory model, implementability, etc; my review is just for the editorial content.

With that caveat, LGTM other than nits.

spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
<p>The following are guidelines for ECMAScript implementers implementing resizable ArrayBuffer.</p>
<p>Resizable ArrayBuffer can be implemented as copying upon resize, as in-place growth via reserving virtual memory up front, or as a combination of both for different values of the constructor's *"maxByteLength"* option.</p>
<p>If a host is multi-tenanted (i.e. it runs many ECMAScript applications simultaneously), such as a web browser, and its implementations choose to implement in-place growth by reserving virtual memory, we recommend that both 32-bit and 64-bit implementations throw for values of *"maxByteLength"* ≥ 1GiB to 1.5GiB. This is to reduce the likelihood a single application can exhaust the virtual memory address space and to reduce interoperability risk.</p>
<p>If a host does not have virtual memory, such as those running on embedded devices without an MMU, or if a host only implements resizing by copying, it may accept any Number value for the *"maxByteLength"* option. However, we recommend a *RangeError* be thrown if a memory block of the requested size can never be allocated. For example, if the requested size is greater than the maximium amount of usable memory on the device.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Number value for" is a 'd phrase, which causes this to link. So

Suggested change
<p>If a host does not have virtual memory, such as those running on embedded devices without an MMU, or if a host only implements resizing by copying, it may accept any Number value for the *"maxByteLength"* option. However, we recommend a *RangeError* be thrown if a memory block of the requested size can never be allocated. For example, if the requested size is greater than the maximium amount of usable memory on the device.</p>
<p>If a host does not have virtual memory, such as those running on embedded devices without an MMU, or if a host only implements resizing by copying, it may accept any <emu-not-ref>Number value for</emu-not-ref> the *"maxByteLength"* option. However, we recommend a *RangeError* be thrown if a memory block of the requested size can never be allocated. For example, if the requested size is greater than the maximium amount of usable memory on the device.</p>

Alternatively we could change the "Number value for" dfn to include a preceding "the".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an emu-not-ref for now. I don't feel like we need to include the preceding "the" in the existing dfn.

<p>We recommend growable SharedArrayBuffer be implemented as in-place growth via reserving virtual memory up front.</p>
<p>Because grow operations can happen in parallel with memory accesses on a growable SharedArrayBuffer, the constraints of the memory model require that even unordered accesses do not "tear" (bits of their values will not be mixed). In practice, this means the underlying data block of a growable SharedArrayBuffer cannot be grown by being copied without stopping the world. We do not recommend stopping the world as an implementation strategy because it introduces a serialization point and is slow.</p>
<p>Grown memory must appear zeroed from the moment of its creation, including to any racy accesses in parallel. This can be accomplished via zero-filled-on-demand virtual memory pages, or careful synchronization if manually zeroing memory.</p>
<p>Integer-indexed property access on TypedArray views of growable SharedArrayBuffers is intended to be optimizable similarly to access on TypedArray views of non-growable SharedArrayBuffers, because integer-indexed property loads on are not synchronizing on the underlying buffer's length (see programmer guidelines above). For example, bounds checks for property accesses may still be hoisted out of loops.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"integer" is a 'd term, which causes its use in this section to link. I don't think we actually want that, since here these are actually integral Numbers, not mathematical integers.

So "integer" should get emu-not-ref'd, or, alternatively, we could update the existing dfn for "integer-index" to also encompass "integer-indexed", so that this would link there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update the existing dfn for "integer-index" to also encompass "integer-indexed", so that this would link there

I prefer this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syg syg added has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. and removed pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. labels Sep 26, 2023
@syg
Copy link
Contributor Author

syg commented Sep 26, 2023

Rebased.

@bakkot bakkot added the editor call to be discussed in the next editor call label Oct 5, 2023
@bakkot bakkot added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Oct 12, 2023
@bakkot
Copy link
Contributor

bakkot commented Oct 12, 2023

@michaelficarra is going to defer any more thorough review to me, so this is ready to go.

…c39#3116)

Major differences from the spec draft:

- The idempotent byte length getter machinery is replaced with
  Integer-Indexed Object With Buffer Witness Record and DataView With
  BufferWitness Record values instead. These records cache the byte
  length ahead of time. Because shared memory events are observable,
  there should only be a single load event of growable SAB byte length
  until the next point where user code may be called.

- IsResizableArrayBuffer is negated and renamed to
  IsFixedLengthArrayBuffer, because it covers both resizable ABs and
  growable SABs.
@ljharb ljharb merged commit a9ae96e into tc39:main Oct 12, 2023
7 checks passed
zhangenming pushed a commit to zhangenming/ecma262 that referenced this pull request Dec 22, 2023
…c39#3116)

Major differences from the spec draft:

- The idempotent byte length getter machinery is replaced with
  Integer-Indexed Object With Buffer Witness Record and DataView With
  BufferWitness Record values instead. These records cache the byte
  length ahead of time. Because shared memory events are observable,
  there should only be a single load event of growable SAB byte length
  until the next point where user code may be called.

- IsResizableArrayBuffer is negated and renamed to
  IsFixedLengthArrayBuffer, because it covers both resizable ABs and
  growable SABs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants