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

[js-api] Integrate with the ResizableArrayBuffer proposal #1300

Merged
merged 8 commits into from
Apr 3, 2024

Conversation

syg
Copy link
Contributor

@syg syg commented Mar 30, 2021

Closes #1292

This PR integrates the resizable buffers proposal with js-api.

Example use of the proposed API:

// mem is a WebAssembly.Memory
let ab = mem.buffer;
assert(!ab.resizable);

// Make .buffer resizable, detaching the old one.
let rab = mem.toResizableBuffer();
assert(rab.resizable);
assert(mem.buffer === rab);
assert(%IsDetached(ab));

// We can go back if we choose.
let ab2 = mem.toFixedLengthBuffer();
assert(!ab2.resizable);
assert(mem.buffer === ab2);
assert(ab2 !== ab);
assert(%IsDetached(rab));

// Let's go back to resizable. Memory#grow no longer detaches .buffer when it's resizable.
rab = mem.toResizableBuffer();
let oldLen = rab.byteLength;
mem.grow(8);
assert(!%IsDetached(rab);
assert(rab.byteLength === oldLen + (8 * 65536))

// RAB#resize throws when trying to shrink or grow by non-page multiples for WebAssembly.Memory-vended RABs.
assertThrows(() => rab.resize(rab.byteLength - 65536), RangeError);
assertThrows(() => rab.resize(rab.byteLength + 10), RangeError);

littledan
littledan previously approved these changes Apr 15, 2021
Copy link
Collaborator

@littledan littledan left a comment

Choose a reason for hiding this comment

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

This proposed integration looks great to me. I like the API shape and the semantics of when and how rounding happens.

I'm happy to see proposals move through WebAssembly CG's process in the form of a PR, which seems more clear and lightweight than the whole-repository fork.

Have we heard feedback from SpiderMonkey or JSC about this PR? cc @lars-t-hansen @kmiller68

1. Return |resizableBuffer|.
</div>

{{ResizableArrayBuffer}} objects returned by a {{Memory}} object must use the following definition of [=HostResizeArrayBuffer=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think of host hooks as being uniform across the agent. Maybe it makes more sense to use an internal slot where hosts can write a method, rather than a host hook, if this varies per instance.

Anyway, the idea that the Wasm ResizableArrayBuffers round up to the next Wasm page when you grow them (and invoke the "grow the memory buffer" algorithm) sounds perfect to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting idea for hosts to write a method. It'll be a new editorial convention.

Do I understand the nit correctly that you see this as an implementation hook instead of a host hook, thus the suggestion?

document/js-api/index.bs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@syg syg left a comment

Choose a reason for hiding this comment

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

Thanks for the review.

document/js-api/index.bs Outdated Show resolved Hide resolved
1. Return |resizableBuffer|.
</div>

{{ResizableArrayBuffer}} objects returned by a {{Memory}} object must use the following definition of [=HostResizeArrayBuffer=].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting idea for hosts to write a method. It'll be a new editorial convention.

Do I understand the nit correctly that you see this as an implementation hook instead of a host hook, thus the suggestion?

@rossberg
Copy link
Member

rossberg commented Aug 4, 2022

@syg, this PR is stale, do you still intend to land it?

@syg
Copy link
Contributor Author

syg commented Aug 8, 2022

I do, yes, but it's not clear to me when I should be doing that. This PR is phase... 3, I think? How should integration PRs like this land? My recollection is that we're waiting for the upstream JS feature to get into the JS standard (i.e. stage 4), which hasn't happened yet.

@dtig
Copy link
Member

dtig commented Aug 9, 2022

We discussed this in the 6/22/21 meeting, though the meeting notes aren't very verbose. What I remember matches @syg's comment above that as this is an integration feature, we do need this to be standardized in TC39, before this is merged into the Wasm spec. The plan for this specifically is that once the JS feature lands, this comes back to the Wasm CG for a final poll, and can land with edits if needed.

@dtig
Copy link
Member

dtig commented Aug 26, 2022

I'd like some input on the usage patterns of this API in the tools. One of the motivations of the ResizableArrayBuffer proposal is to expose memory.grow to JS without having to detach the existing ArrayBuffer. If a Wasm instance has a "default" buffer associated with it, to ensure that it doesn't detach on Grow it stands to reason that it should be a ResizableArrayBuffer. The change that this PR makes is that the buffer getter returns a fixed length ArrayBuffer (which makes sense for backwards compatibility). My question here is how would tools use this API? Is the expectation that applications would use toResizableBuffer in place of the buffer getter?

On a side note, this proposal also needs a similar PR to the threads repository to integrate GSABs with shared Wasm memory.

@rossberg
Copy link
Member

Has there been any progress on this?

@syg
Copy link
Contributor Author

syg commented Feb 16, 2023

@rossberg My understanding is there's discussion on whether it's okay for the wasm spec to depend on JS proposals that aren't yet merged into ecma262. The status quo is no, so I've been conservative and letting this sit open until we get resizable buffers to stage 4 in TC39.

Currently the proposal is stage 3 in TC39. Chrome has shipped in M111, and I see Safari enabled it in STP 160. I can probably ask for stage 4 soon, but the practical upshot it's stable and shipped and there shouldn't be too much risk for the wasm spec to depend on it even during stage 3.

Edit: Of course there are mechanical issues with depending on a stage 3 TC39 proposal. The wasm spec document would need to normatively reference spec drafts instead of the official living spec, etc.

@rossberg
Copy link
Member

@syg, makes sense, thanks for the update!

@syg
Copy link
Contributor Author

syg commented Apr 24, 2023

@conrad-watt Could you give me an overview of what needs changing on the threads proposal to account for resizable buffers? (Or, if I'm really feeling lucky, are you up for making the changes?)

@conrad-watt
Copy link
Contributor

I'm happy to take a look at this myself, but I'm currently working through a rebase of the threads repo (with thanks to @ioannad!) which I'd prefer to finish first. Is it ok if I get back to you in ~2 weeks?

Looking at the language in the current draft threads JS API, my knee-jerk feeling is that growable SharedArrayBuffer integration won't be too difficult an extension, so long as I can crib from this PR.

@syg
Copy link
Contributor Author

syg commented Apr 27, 2023

@conrad-watt 2 weeks sgtm! I'm getting ready to ask for Stage 4 for resizable buffers so crossing my t's and dotting my i's.

@conrad-watt
Copy link
Contributor

Ok, I have a fresh version of the threads specification now living in a branch. @syg I'll spend a few days looking at growable SharedArrayBuffer and see what I can do (although I'll probably end up begging for help at some point).

Just as a very surface-level question, is the intention that growable buffers will be produced by toResizableBuffer on a shared memory, or will there be a separate toGrowableBuffer method for shared memories? I have a slight knee-jerk preference for the former.

@syg
Copy link
Contributor Author

syg commented May 17, 2023

Ok, I have a fresh version of the threads specification now living in a branch. @syg I'll spend a few days looking at growable SharedArrayBuffer and see what I can do (although I'll probably end up begging for help at some point).

Excellent! I'd be happy to do work here as well but I'd need handholding around the Wasm spec.

Just as a very surface-level question, is the intention that growable buffers will be produced by toResizableBuffer on a shared memory, or will there be a separate toGrowableBuffer method for shared memories? I have a slight knee-jerk preference for the former.

The intention was for the former IIRC. The latter might have made more sense in the first iteration of the proposal, where ResizableArrayBuffer and GrowableSharedArrayBuffer had different JS types. But the current iteration keeps them as ArrayBuffer and SharedArrayBuffer so one getter makes more sense IMO.

@conrad-watt
Copy link
Contributor

Just a note here that we should probably keep an eye on tc39/ecma262#3116

1. Return |resizableBuffer|.
</div>

{{ArrayBuffer}} objects returned by a {{Memory}} object must use the following definition of [=HostResizeArrayBuffer=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not great - I think we should have a global HostResizeArrayBuffer definition, which checks if its buffer argument is one of these special buffers - I guess we could even use [[ArrayBufferDetachKey]] for that

ioannad added a commit to ioannad/threads that referenced this pull request Aug 31, 2023
…dd-RAB-support

This adds support for ResizableArrayBuffers and GrowableSharedArrayBuffers,
by adding and extending the changes in rab-js-api-integration [1]
to include cases for SharedArrayBuffer.
Also adds a TODO for the steps of the new HostGrowSharedArrayBuffer algorithm.

All this could potentially be refactored to be more concise.

[1] WebAssembly/spec#1300
conrad-watt pushed a commit to WebAssembly/threads that referenced this pull request Sep 12, 2023
…rayBuffers (#208)

* Merge remote-tracking branch 'syg/spec/rab-js-api-integration' into add-RAB-support

This adds support for ResizableArrayBuffers and GrowableSharedArrayBuffers,
by adding and extending the changes in rab-js-api-integration [1]
to include cases for SharedArrayBuffer.
Also adds a TODO for the steps of the new HostGrowSharedArrayBuffer algorithm.

All this could potentially be refactored to be more concise.

[1] WebAssembly/spec#1300

* Add WIP algorithm for HostGrowSharedArrayBuffer and stylistic changes

to match the style in the spec PR adding resizable buffers.

* Fixed HostGrowSharedArrayBuffer algorithm notation, added TODOs to not forget to check these cases.

* Moved TODO item to the correct location.
@bakkot
Copy link

bakkot commented Oct 24, 2023

The JS proposal has landed in the JS specification, with implementations shipping in Chrome and Safari and IIRC underway in Firefox.

@ioannad
Copy link
Contributor

ioannad commented Nov 2, 2023

@Ms2ger does the last commit address your review comment?

Copy link
Collaborator

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

Thanks, just some minor comments.

document/js-api/index.bs Outdated Show resolved Hide resolved
document/js-api/index.bs Outdated Show resolved Hide resolved
document/js-api/index.bs Outdated Show resolved Hide resolved
document/js-api/index.bs Outdated Show resolved Hide resolved
document/js-api/index.bs Outdated Show resolved Hide resolved
@rossberg
Copy link
Member

rossberg commented Apr 2, 2024

@syg, can you address @Ms2ger's remaining comments, so that we can merge this?

@syg
Copy link
Contributor Author

syg commented Apr 2, 2024

I think @ioannad would better handle it, as she's been handling @Ms2ger's previous reviews. Sorry to pass the buck. :)

@Ms2ger Ms2ger merged commit e968229 into WebAssembly:master Apr 3, 2024
@castholm
Copy link

castholm commented Apr 3, 2024

@Ms2ger I'm just an outside observer so apologies if I'm stating the obvious, but I wanted to point out that this was merged into the master branch and not main, so as a result the linked issue was not closed and the updated spec was not deployed to https://webassembly.github.io/spec/.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Apr 3, 2024

Argh. I opened #1742, but of course now I can't approve it, because I opened it. Thanks for flagging!

Ms2ger added a commit that referenced this pull request Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[js-api] Integration with the ResizableArrayBuffer and GrowableSharedArrayBuffer proposal
9 participants