[webkit-reviews] review granted: [Bug 184745] [WebAssembly][Modules] Import memory to/from wasm modules : [Attachment 446098] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Asumu Takikawa
<asumu at igalia.com>'s request for review:
Bug 184745: [WebAssembly][Modules] Import memory to/from wasm modules
https://bugs.webkit.org/show_bug.cgi?id=184745

Attachment 446098: Patch

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




--- Comment #11 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 446098
  --> https://bugs.webkit.org/attachment.cgi?id=446098
Patch

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

r=me with one suggestion.

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:41
>      return adoptRef(*result);

Ditto, can you change it too?

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:44
> +Ref<CodeBlock> CodeBlock::createFromExisting(MemoryMode mode, Ref<CodeBlock>
other)

Let's take const CodeBlock&.

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:46
> +    auto* result = new (NotNull, fastMalloc(sizeof(CodeBlock)))
CodeBlock(mode, other);

Not sure why, but it is doing `new (NotNull, fastMalloc(sizeof(CodeBlock)))`. I
don't think we need it.
Can you change them with the following?

return adoptRef(*new CodeBlock(mode, other));

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:50
> +CodeBlock::CodeBlock(MemoryMode mode, Ref<CodeBlock> other)

Let's take const CodeBlock& since this constructor will not need to keep
other's ownership.

> Source/JavaScriptCore/wasm/WasmModule.cpp:117
> +    RefPtr<CodeBlock> initialBlock =
m_codeBlocks[static_cast<uint8_t>(initialMode)].copyRef();
> +    ASSERT(initialBlock.get());

Let's get it as

ASSERT(m_codeBlocks[static_cast<uint8_t>(initialMode)]);
const CodeBlock& initialBlock =
*m_codeBlocks[static_cast<uint8_t>(initialMode)].

> Source/JavaScriptCore/wasm/WasmModule.cpp:121
> +	   Ref<CodeBlock> newBlock =
CodeBlock::createFromExisting(static_cast<MemoryMode>(i),
initialBlock.releaseNonNull());

Can we avoid using `releaseNonNull()` here?
Currently, it is OK since NumberOfMemoryModes is 2. But it relies on this
number, and if it becomes 3, then this is wrong since the second call of this
will get nullptr.
Since we should make the parameter const CodeBlock&, we can just pass
initialBlock.get().

> Source/JavaScriptCore/wasm/WasmModule.cpp:124
> +}

Add one empty line before the namespace closing.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:140
> +    // In the module loader case, we will initialize all memory modes with
the initial LLInt compilation
> +    // results, so that later when memory imports become available, the
appropriate CodeBlock can be used.
> +    // If LLInt is disabled, we instead defer compilation to module
evaluation.

For non-module-loader case, we anyway need to do this kind of handling, is it
correct? (See some failing property-access-order tests on WPT wasm tests.)

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:282
> +	       // For the module loader, we cannot initialize the memory here
so we delay this
> +	       // until WebAssemblyModuleRecord's initialization operation.
> +	       if (creationMode == Wasm::CreationMode::FromModuleLoader)
> +		   break;

Ditto. For non-module-loader case, we anyway need to do this kind of handling,
is it correct? (See some failing property-access-order tests on WPT wasm
tests.)
If so, can we remove here's memory initialization and unify them?
While this is not efficient if we are disabling LLInt, in practice, we are
always enabling LLInt in shipping configuration.
It is not efficient if we disable LLInt in testing, but it does not matter
since it is testing.
But, I'm OK to defering it in a subsequent patch.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:452
> +

Remove this empty line.