JS API: clarify refreshing of buffers

[jakobkummerow]

Comparing our implementation of this proposal with the spec text, I was surprised to find that the spec for growing a shared memory (https://webassembly.github.io/threads/js-api/index.html#grow-the-memory-buffer) does not at all mention the existence of multiple agents. Since that's a situation that's somewhat tricky to get right, it would be nice for the spec text to be very explicit. I think the text as currently written could be read as "when thread A grows a shared memory, thread B may never realize that this happened", which I guess isn't what was intended.

To illustrate with a concrete example: IIUC, V8 currently refreshes too many buffers. When thread A grows a shared memory, it notifies all other threads referencing the same shared memory that some memory growth has happened (the notification includes no details), and when those threads become aware of that notification, they refresh the array buffers for all shared memories that they know of. Due to the freezing of these buffers, that's unlikely to break any userspace code, but technically it is observable via object identity.
So for both efficiency and spec compliance, I think that implementation should probably be changed to refresh only the buffers associated with the memory that was grown. It is, however, not trivial to identify them by their own length mismatching that of the underlying memory block, because the memory.grow instruction can grow a memory by zero pages. So the obvious fix I have in mind would end up refreshing too few buffers.
To figure out a viable implementation design, it would be nice to have confidence that it actually matches the spec (and hence other implementations), so I would appreciate it if the spec didn't leave as many details to my imagination.

[eqrion]

The implementation in SpiderMonkey works by having the .buffer accessor lazily refresh the shared array buffer. It checks if the memory is shared and the underlying memory's length has increased relative to the wrapper SAB to know if a new SAB needs to be created.

I thought this was how it was specified, but it looks like the threads proposal requires the buffer accessor to unconditionally refresh the SAB.

I think it'd be nice to only refresh the SAB if the length has changed, but we can also do it every time if need be.

[conrad-watt]

In the non-shared case, even grow(0) detaches the current ArrayBuffer (and I think this is well-documented). So the most consistent choice would be for a zero-page grow to refresh all of the associated SharedArrayBuffers. My first instinct from reading @jakobkummerow's comment though is that implementing this seems annoying.

I think it'd be nice to only refresh the SAB if the length has changed, but we can also do it every time if need be.

Can you briefly walk through how this would be implemented in SpiderMonkey without spuriously refreshing other buffers like V8 currently does?

[eqrion]

@conrad-watt

Non-shared buffers are refreshed through detaching immediately on memory.grow.

For shared memory, we lazily refresh the buffer when the WebAssembly.Memory.prototype.buffer accessor is invoked. This works by checking if the length of the current SAB is different from the underlying memory's length. If they are the same, we return the same SAB, otherwise we create a new SAB that points at the wasm memory with a snapshot of the memory's length.

This not spec compliant if we need to return a new SAB object for 0-length grows.

I think we could support that by adding a 'grow generation' field to the wasm memory and wasm SAB objects that is incremented even on 0-length grows, and then compare that and the length to see if a new SAB needs to be created. Some handwaving about generation overflow if there are more than UINT64_MAX zero length grows.

Or another way to do it would be to have underlying shared wasm memory contain a mutex and have all unshared WebAssembly.Memory objects register themselves their to be notified when the memory grows. And the notification would set a bool that the next time the .buffer accessor is invoked you must refresh it.