Bug 218693 – [JSC] Implement WebAssembly.Memory with shared

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Key requirement is that WebAssembly.Memory can be shared, plus, it can be grown by any threads.
Interesting thing is that, how to propagate this updated information to the other threads.

For memory itself, we cannot use reallocated memory since while reallocating, the other thread can access the old one.
These old one's write can be discarded if we cannot copy this content to the new memory.
We should allocate the memory as pages if shared is specified (a.k.a. like, fast memory) and grow it with mprotect.
To make it available, when shared: true is specified, WebAssembly.Memory mandatorily requires "maximum" size. So we can just allocate this pages, and available only initial bytes, and keep the rest unusable.
When growing, we can just call mprotect to make it usable so that we can use grown memory without changing address => perfect.
Since we only allocates maximum pages, we do not need to have much VA. So, even in iOS, we can use it.

The second problem is that memory address and memory size are cached by WebAssembly.Instance. By using the above technique, we can make address frozen.
But size can be changed in the current mechanism, since this size is used for bound checking right now, plus, this is the active memory size which can be changed by mprotect.
There are two potential answers.

1. Let's do some dirty hack and propagate the size information *eventually* to the all threads by using VM trap mechanism.
2. Let's change the size's meaning of WebAssembly.Instance. We put max VA size here instead of currently available size. Since this includes non-active memory region in bound-checking mode, we will hit signal handler invocation. And then, we use signal handler to emit out-of-memory exception even for bound-checking mode too.

I think (2)'s option is viable.

[Yusuke Suzuki]

For (2), there are two ways to throwing OoB memory access error for bound checking.

1. Simply bound checking is failed => throwing an error.
2. Bound checking succeeds, but it invokes signal handler since it touches non-active memory => throwing an error from signal handler.

[Saam Barati]

Does the spec say anything about when the size needs to be updated for other threads?

[Yusuke Suzuki]

Created attachment 414008 [details]
Patch

[Yusuke Suzuki]

Created attachment 414012 [details]
Patch

[Yusuke Suzuki]

Created attachment 414013 [details]
Patch

[Yusuke Suzuki]

Created attachment 414109 [details]
Patch

[Yusuke Suzuki]

Created attachment 414129 [details]
Patch

[Yusuke Suzuki]

Created attachment 414157 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71409442>

[Yusuke Suzuki]

Created attachment 414283 [details]
Patch

[Yusuke Suzuki]

Created attachment 414289 [details]
Patch

[Saam Barati]

Comment on attachment 414283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414283&action=review

Nice!! r=me

> Source/JavaScriptCore/ChangeLog:10
> +        one thread, and immediately, it should be accessible in the other threads.

there's a lot of subtlety/detail in "and immediately" in this sentence.

How is this specified in the spec? Understanding the atomic guarantees would be interesting I think.

> Source/JavaScriptCore/ChangeLog:24
> +        <================================================ maximum ============================><------------ other memory, protected by bounds-checking --...
> +        <======= active ==========><===================== not active yet =====================>
> +        ^                               [ if we access this, fault handler will detect it]    ^
> +        pointer                                                                               bound checking size

nice! This is a clever and good solution

> Source/JavaScriptCore/ChangeLog:26
> +        These "growable bound-checking memory" is now managed by wasm memory-manager. And fault handler is even if fast memory is disabled.

"And fault handler is even if" => "And fault handler is used even if"

> Source/WebCore/ChangeLog:13
> +            only when the WebAssembly.Memory is shared mode. And it is only available when we are using it in postMessage.

I commented in the actual code, but does that also mean window.postMessage?

> Source/JavaScriptCore/runtime/Options.cpp:368
>  #if !ENABLE(WEBASSEMBLY_FAST_MEMORY)

maybe this should be renamed to something like ENABLE_WEBASSEMBLY_SIGNALING_MEMORY?

> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:80
> +            auto doesFaultInWasm = [](void* faultingInstruction) {

doesFaultInWasm => didFaultInWasm

> Source/JavaScriptCore/wasm/WasmMemory.cpp:167
> +    bool isAddressInGrowableMemory(void* address)

same comment on naming, I feel like maybe "isInSignalingMemory" or perhaps "isInGrowableOrFastMemory"?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:234
>      Vector<void*> m_fastMemories;

why not just put these in the StdSet below?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:280
> +MemoryHandle::MemoryHandle(void* memory, size_t size, size_t mappedCapacity, PageCount initial, PageCount maximum, MemorySharingMode sharingMode, MemoryMode mode)

for non shared, non fast memory, can we assert size == mappedCapacity, since our implementation requires that now

> Source/JavaScriptCore/wasm/WasmMemory.cpp:455
> +bool Memory::addressIsInActiveGrowableMemory(void* address)

why not just call these "signaling memories" instead of "growable" memory? Seems like a more descriptive name for what it is

> Source/JavaScriptCore/wasm/WasmMemory.cpp:490
> +        bool allocationSuccess = tryAllocate(

I feel like it might be time for a new function since this doesn't actually allocate anything, and just does bookkeeping

> Source/JavaScriptCore/wasm/WasmMemory.cpp:508
> +        WTF::storeStoreFence(); // Ensure storing "size" happens after mprotect is done.

I'm assuming mprotect does a full on memory fence, but I feel like if it didn't, storeStore isn't sufficient here. We'd want a full memory fence.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1294
> +                if (m_context != SerializationContext::WorkerPostMessage) {

what about window post message?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3358
> +            Ref<Wasm::Memory> memory = Wasm::Memory::create(handle.releaseNonNull(),
> +                [&vm] (Wasm::Memory::NotifyPressure) { vm.heap.collectAsync(CollectionScope::Full); },
> +                [&vm] (Wasm::Memory::SyncTryToReclaim) { vm.heap.collectSync(CollectionScope::Full); },
> +                [&vm, result] (Wasm::Memory::GrowSuccess, Wasm::PageCount oldPageCount, Wasm::PageCount newPageCount) { result->growSuccessCallback(vm, oldPageCount, newPageCount); });

This feels like a weird paradigm if we're just sharing memory. Why would sharing memory ever convince us to GC? Since growing  here is just mprotect.

> JSTests/wasm/stress/shared-wasm-memory-buffer.js:3
> +let pageSize = 64 << 10;

nit: we don't have this hard coded somewhere we can import from?

[Saam Barati]

Comment on attachment 414289 [details]
Patch

sorry, commented on the old patch. r=me still

[Yusuke Suzuki]

Comment on attachment 414283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414283&action=review

>> Source/JavaScriptCore/ChangeLog:10
>> +        one thread, and immediately, it should be accessible in the other threads.
> 
> there's a lot of subtlety/detail in "and immediately" in this sentence.
> 
> How is this specified in the spec? Understanding the atomic guarantees would be interesting I think.

This is defined as global state is modified.

>> Source/JavaScriptCore/ChangeLog:26
>> +        These "growable bound-checking memory" is now managed by wasm memory-manager. And fault handler is even if fast memory is disabled.
> 
> "And fault handler is even if" => "And fault handler is used even if"

Fixed.

>> Source/WebCore/ChangeLog:13
>> +            only when the WebAssembly.Memory is shared mode. And it is only available when we are using it in postMessage.
> 
> I commented in the actual code, but does that also mean window.postMessage?

Currently, it is only enabled for worker.postMessage, which is aligned to SharedArrayBuffer implementation right now.
We should look into whether this should be allowed in window.postMessage but for now, we are just using the same semantics to SharedArrayBuffer.

>> Source/JavaScriptCore/runtime/Options.cpp:368
>>  #if !ENABLE(WEBASSEMBLY_FAST_MEMORY)
> 
> maybe this should be renamed to something like ENABLE_WEBASSEMBLY_SIGNALING_MEMORY?

Sounds good. Changed.

>> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:80
>> +            auto doesFaultInWasm = [](void* faultingInstruction) {
> 
> doesFaultInWasm => didFaultInWasm

Fixed.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:167
>> +    bool isAddressInGrowableMemory(void* address)
> 
> same comment on naming, I feel like maybe "isInSignalingMemory" or perhaps "isInGrowableOrFastMemory"?

isInGrowableOrFastMemory sounds good. Changed.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:234
>>      Vector<void*> m_fastMemories;
> 
> why not just put these in the StdSet below?

Currently, we are tracking # of fast memories with this vector.
I think this is nice refactoring in a subsequent patch.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:280
>> +MemoryHandle::MemoryHandle(void* memory, size_t size, size_t mappedCapacity, PageCount initial, PageCount maximum, MemorySharingMode sharingMode, MemoryMode mode)
> 
> for non shared, non fast memory, can we assert size == mappedCapacity, since our implementation requires that now

Added.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:455
>> +bool Memory::addressIsInActiveGrowableMemory(void* address)
> 
> why not just call these "signaling memories" instead of "growable" memory? Seems like a more descriptive name for what it is

Because we would like to keep "fast" memory as "fast" memory named. Shared wasm memory is also using bounds-checking so it is not fully signaling one.
Changed it to addressIsInGrowableOrFastMemory

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:490
>> +        bool allocationSuccess = tryAllocate(
> 
> I feel like it might be time for a new function since this doesn't actually allocate anything, and just does bookkeeping

For now, we keep this function since it is also used by an actual allocation function (see fast-memory allocation case).
So, I'll just keep it untouched in this patch.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:508
>> +        WTF::storeStoreFence(); // Ensure storing "size" happens after mprotect is done.
> 
> I'm assuming mprotect does a full on memory fence, but I feel like if it didn't, storeStore isn't sufficient here. We'd want a full memory fence.

OK, removed.

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1294
>> +                if (m_context != SerializationContext::WorkerPostMessage) {
> 
> what about window post message?

This is aligned to SharedArrayBuffer for now.

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3358
>> +                [&vm, result] (Wasm::Memory::GrowSuccess, Wasm::PageCount oldPageCount, Wasm::PageCount newPageCount) { result->growSuccessCallback(vm, oldPageCount, newPageCount); });
> 
> This feels like a weird paradigm if we're just sharing memory. Why would sharing memory ever convince us to GC? Since growing  here is just mprotect.

We are tracking physical memory usage and if it gets high, it is possible that collecting something will reduce that memory usage. Shared WasmMemory does not invoke sync-reclaim, but still it can call success-callback which can reduce memory pressure.

>> JSTests/wasm/stress/shared-wasm-memory-buffer.js:3
>> +let pageSize = 64 << 10;
> 
> nit: we don't have this hard coded somewhere we can import from?

I think there is no constants in JS world, so we need to hardcode in this test.
But the value of this is hardcoded in the wasm spec. So we can just use it here. https://github.com/WebAssembly/design/blob/master/Rationale.md#linear-memory-resizing

[Yusuke Suzuki]

Committed r269940: <https://trac.webkit.org/changeset/269940>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 219076

[Yusuke Suzuki]

Relanded in https://trac.webkit.org/changeset/269974/webkit

[Sam Sneddon [:gsnedders]]

*** Bug 202737 has been marked as a duplicate of this bug. ***