[webkit-reviews] review denied: [Bug 177305] WebAssembly: cache memory address / size on instance : [Attachment 329610] Patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has denied GSkachkov <gskachkov at gmail.com>'s
request for review:
Bug 177305: WebAssembly: cache memory address / size on instance
https://bugs.webkit.org/show_bug.cgi?id=177305

Attachment 329610: Patch

https://bugs.webkit.org/attachment.cgi?id=329610&action=review




--- Comment #8 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 329610
  --> https://bugs.webkit.org/attachment.cgi?id=329610
Patch

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

Very cool! That's the right direction, but I have a few comments.

Also for testing you might want to look at the memory fuzz test I wrote in
JSTests/wasm/fuzz

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:-1284
> -	       ASSERT(sizeRegs[0].sizeRegister != baseMemory);

I think we should keep this assert.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1281
>	       ASSERT(!sizeRegs[0].sizeOffset);

Can you also assert that sizeRegs.size() == 1 ? The code below assumes it is,
and would be wrong if that weren't the case. You can like to FIXME
https://bugs.webkit.org/show_bug.cgi?id=162952

> Source/JavaScriptCore/wasm/WasmBinding.cpp:-68
> -    ASSERT(!sizeRegs[0].sizeOffset); // The following code assumes we start
at 0, and calculates subsequent size registers relative to 0.

Same here on keeping the assert, and adding one for
https://bugs.webkit.org/show_bug.cgi?id=162952

> Source/JavaScriptCore/wasm/WasmInstance.h:77
> +	   updateChechedMemory();

Typo "cached".

> Source/JavaScriptCore/wasm/WasmInstance.h:79
> +    void updateChechedMemory()

Typo.

> Source/JavaScriptCore/wasm/WasmInstance.h:147
> +    WeakPtrFactory<Instance> m_weakPtrFactory;

I would try to organize members so that the ones accessed by the JIT are
together, at the start. That way we need only small immediates, and they'll be
in the same cacheline when accessed.

So the cached values would go after m_context, and the weak ptr factory after
the call frame callback.

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:193
> +	       jit.loadWasmContextInstance(baseMemory);

This now happens twice if the memory wasn't signaling and we use fast TLS.

It might be easier to have 4 code paths without any sharing, for all
combinations of signal/no + fast TLS/no.

> JSTests/wasm/function-tests/memory-reuse.js:4
> +{

This scope doesn't seem useful?

> JSTests/wasm/function-tests/memory-reuse.js:8
> +    function createWasmModule(memory) {

This returns instances, so I'd call it "createInstance".

> JSTests/wasm/function-tests/memory-reuse.js:41
> +	   for (let i = 0; i < buf.length/4; ++i) {

Spaces between divide. Same thing for other binary operators below.

> JSTests/wasm/function-tests/memory-reuse.js:56
> +	   const pageSize = 64 * 1024;

Hoist this to the top of the file, and remove the other one above.

> JSTests/wasm/function-tests/memory-reuse.js:58
> +	   const modules = [];

As above, I'd rename to "instances".

> JSTests/wasm/function-tests/memory-reuse.js:68
> +    }

I think the start of the test is good, but you then need to grow the memory.
There are two ways to do so: as the grow_memory opcode directly in WebAssembly
IR, or through the JS API on WebAssembly.Memory. You should then re-run
doCheck().

The difficulty in doing this test is that you want them memory's base void* to
have moved (hard!), and the size to change (easy!). You're not really testing
size because signaling memory does that implicitly. I think you want a VM
primitive for testing that allows you to allocate a WebAssembly.Memory that is
never signaling. That way to can test size by going out-of-bounds, catching the
trap, then growing, and same out-of-bounds should not trap anymore. Now with a
non-signaling memory you know that enough growth will cause us to re-allocate
and memmove then void* memory too!

After doing so, the tests should kill instances and force-invoke the GC to try
to make sure they die, and then re-run the checks again.