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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 7 11:26:04 PST 2021

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

Attachment 446098: Patch


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

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>

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 =
> +    ASSERT(initialBlock.get());

Let's get it as

const CodeBlock& initialBlock =

> Source/JavaScriptCore/wasm/WasmModule.cpp:121
> +	   Ref<CodeBlock> newBlock =

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

> 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

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
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.

More information about the webkit-reviews mailing list