Bug 184745 – [WebAssembly][Modules] Import memory to/from wasm modules

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[WebAssembly][Modules] Import memory to/from wasm modules

[Yusuke Suzuki]

Created attachment 338231 [details]
Patch

WIP!

[EWS Watchlist]

Comment on attachment 338231 [details]
Patch

Attachment 338231 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7360662

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-5.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-7.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-error-2.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-3.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-4.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-6.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html

[EWS Watchlist]

Created attachment 338243 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 338231 [details]
Patch

Attachment 338231 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7360793

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-5.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-7.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-error-2.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-3.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-4.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-6.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html

[EWS Watchlist]

Created attachment 338245 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 338231 [details]
Patch

Attachment 338231 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7361036

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-5.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-7.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-error-2.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-3.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-4.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-6.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html

[EWS Watchlist]

Created attachment 338250 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 338231 [details]
Patch

Attachment 338231 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7361209

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-5.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-7.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-error-2.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-3.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-4.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-6.html
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html

[EWS Watchlist]

Created attachment 338255 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Asumu Takikawa]

Created attachment 446098 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 446098 [details]
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.

[Asumu Takikawa]

Created attachment 446251 [details]
Patch

[Asumu Takikawa]

Thanks for the review! The latest patch should address the feedback. I agree that the memory import initialization could be unified for module loader and non-module loader cases. I'd be happy to submit a follow-up patch in a new bug.

[Yusuke Suzuki]

Comment on attachment 446251 [details]
Patch

r=me

[EWS]

Committed r286703 (244983@main): <https://commits.webkit.org/244983@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446251 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86224393>