Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

More details on how this interacts with WebAssembly.Memory #5

Closed
binji opened this issue Jul 14, 2020 · 16 comments
Closed

More details on how this interacts with WebAssembly.Memory #5

binji opened this issue Jul 14, 2020 · 16 comments

Comments

@binji
Copy link

binji commented Jul 14, 2020

Thanks for working on this, I like the direction this is going! I have a few questions about how this proposal will integrate with WebAssembly.

  1. What are the expected changes to WebAssembly.Memory?
  2. Is it expected that the buffer accessor will always return the same ResizableArrayBuffer?
  3. Are there cases where a regular ArrayBuffer should be used (perhaps if the maximum memory size isn't provided)?
  4. What is the expected behavior if a WebAssembly.Memory's associated ResizableArrayBuffer is shrunk? Do we disallow that operation?
@syg
Copy link
Collaborator

syg commented Jul 14, 2020

My current thinking, which is still a bit handwavy:

  1. Since .buffer is already returning a regular ArrayBuffer, I was thinking we'd add a new property like .resizableBuffer. Which segues into the next question...

  2. If two properties exist, there're at least two reasonable semantics. A) Make them mutually exclusive, depending on some additional flag, perhaps? B) Make them both point to the underlying backing store in the implementation. A) probably makes the most sense as an opt-in, which segues into the next question...

  3. If maximum size is provided, it makes sense to make .resizableBuffer available if we choose the (A) semantics above. This also has the effect of making providing maximum size a carrot. Is that desirable? It might also be more ergonomic to come up with a separate flag that lets the user signal they want .resizableBuffer, regardless of maximum size provided. In our off-GitHub chat I think we agreed such a relaxation can always come later.

  4. This is interesting. I also think disallowing is the most reasonable behavior for wasm-vended resizable buffers. By disallow, I assume we'd throw a RangeError or something instead of silently doing nothing. To enable this, resize(n) for n < current byte length would have to be specced to be allowed to throw. The question is, is that reasonable? And, probably "yes"? It can already throw on growth, so users are already used to try-catching calls to resize(). To maximize implementation latitude, I can imagine an implementation wanting to do in-place shrinks. And maybe that's not always possible because it uses realloc under the hood.

@binji
Copy link
Author

binji commented Jul 15, 2020

This makes sense to me. I was assuming we'd replace the buffer property, but it probably is better to add a new property. I don't think we'll be able to make them mutually exclusive, since we can create a WebAssembly.Memory like this too:

const module = new WebAssembly.Module(bytes);  // some module that exports memory.
const instance = new WebAssembly.Instance(module);
const memory = instance.exports.memory;

We wouldn't want to add a new flag to the wasm binary bytestream for this. I suppose you could imagine another parameter to WebAssembly.Instance constructor, though that feels pretty clunky.

So if we have both buffer and resizableBuffer properties, then the only thing that makes sense (I think) is to have them reference the same underlying backing store. Since we can't really add a flag, if the maximum size is not provided then the resizableBuffer accessor has to throw, right?

To enable this, resize(n) for n < current byte length would have to be specced to be allowed to throw. The question is, is that reasonable? And, probably "yes"?

Yes, this sounds right to me.

@domenic
Copy link
Member

domenic commented Jul 15, 2020

It'd certainly be nicer if we could replace the property. My impression is that doing so would create minor observable changes, but to detect them you'd have to writing somewhat unusual code. Is that impression accurate?

@binji
Copy link
Author

binji commented Jul 16, 2020

Well the major change is that currently a buffer object would be detached if the memory is grown, which is the issue we're trying to fix in the first place. I think it would be unusual to rely on that, but I could see doing it for debugging reasons maybe.

@conrad-watt
Copy link

Apologies if this has already been discussed elsewhere.

In WebAssembly, memory may only be grown in units of "page size" (multiples of 2^16 bytes). Some implementation schemes rely on this and could not support a byte-granularity growth. For example, this approach by V8 relies on protecting/unprotecting OS pages to simulate memory growth and eliminate explicit bounds checks.

My understanding is that we want to be able to expose Wasm memories as GrowableSharedArrayBuffer in JavaScript. Currently the JS grow instruction is sketched as having byte granularity. Is it intended that the buffer should be grown exactly according to the grow argument, or is it permitted for implementations to grow some implementation-defined amount more?

I could alternatively imagine the GrowableSharedArrayBuffer keeping an explicitly checked byteLength even if the underlying Wasm memory needs to be grown more.

@syg
Copy link
Collaborator

syg commented Jan 25, 2021

Good question. This is the resize() analogue to #18.

The wasm behavior can be implemented in the current spec draft by something like throwing on growth requests from the JS side that would result in a non-page size multiple for wasm-vended RABs. Would this be too cumbersome? The other option is as you say, permit implementations to round resize requests accordingly.

For regular JS-originated RABs, I see some value in not rounding, otherwise some class of applications would need to again track their own length.

The cost to the developer is that they'll need to internalize different throwing or rounding behaviors depending on how the RAB was vended. Given that wasm already has this restriction, this difference needs be internalized regardless.

@conrad-watt
Copy link

The wasm behavior can be implemented in the current spec draft by something like throwing on growth requests from the JS side that would result in a non-page size multiple for wasm-vended RABs. Would this be too cumbersome?

Ah, as an OOM error? Conceptually it seems a little odd to me, but I have near-zero context on whether there would be any real practical implications. It might lead to a de-facto restriction where generic libraries over RABs only ever attempt to grow by "Wasm-blessed" values, just in case they're passed a RAB backed by a Wasm memory.

@syg
Copy link
Collaborator

syg commented Jan 26, 2021

Ah, as an OOM error? Conceptually it seems a little odd to me, but I have near-zero context on whether there would be any real practical implications.

All the specification would say is that it's a TypeError. The message contents and kind of error isn't that granular in JS, so it also seems fine to me for those errors to throw for misalignment.

@Ms2ger
Copy link

Ms2ger commented Feb 1, 2021

I'm wondering if it's actually desirable to support calling the mutating methods on wasm-created RABs. We already disallow detaching for ABs, so transfer should probably throw. For resize, we'd need to

  • disallow shrinking
  • limit to page-size lengths
  • tell wasm about the change somehow

Is it workable to include all that?

@syg
Copy link
Collaborator

syg commented Feb 1, 2021

Is it workable to include all that?

From the spec draft's POV, certainly for resize(), which is allowed to throw on inability to grow or shrink, which would cover the first two bullet points. For the last bullet point, are you saying a wasm-visible handler that the application can hook into is needed?

For transfer(), the spec draft's intention is to also allow for it to throw for cases like this. (Indeed, if transfer() were implemented under the hood as realloc, which is fallible, then it must be able to throw TypeErrors.) An editorial question is whether there should be a separate step in the spec that calls out that implementations may throw non-OOM related TypeErrors.

@littledan
Copy link
Member

I think it's important to get a concrete answer on this question, ideally around the time this proposal goes to Stage 3. We should bring a change to the WebAssembly/JS API formalizing the change to the Wasm CG.

@syg
Copy link
Collaborator

syg commented Mar 2, 2021

I think it's important to get a concrete answer on this question, ideally around the time this proposal goes to Stage 3.

What question in particular? Whether resize() should throw for the scenarios described in #5 (comment)?

As for sequencing, are you requesting that the Wasm/JS API integration changes be gotten consensus for before stage 3, or immediately after ("around the time") of stage 3?

@littledan
Copy link
Member

Arguably #5 (comment) was plenty concrete, but still, I'm a bit disappointed if this topic hasn't been raised to Wasm CG. I raised the interaction as a concern at the previous TC39 meeting, and there have been a couple CG meetings since then (and the WebAssembly/spec repo is always open to file an issue). I don't think we need very strong consensus in Wasm CG before Stage 3 in TC39, but ideally we've discussed it with them and have something of a common idea. This is typically the state that we ask for with HTML integration for JS proposals which have a lot of issues there.

@syg
Copy link
Collaborator

syg commented Mar 2, 2021

Looks like the next Wasm CG meeting is on the 16th. I'll file a bug on the Wasm repo.

@syg
Copy link
Collaborator

syg commented Apr 1, 2021

A note that the integration PR is at WebAssembly/spec#1300 and reached Phase 1 on March 30.

@ljharb
Copy link
Member

ljharb commented Oct 24, 2023

The proposal has reached stage 4; further questions should likely go to the discourse.

@ljharb ljharb closed this as completed Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants