236268 – [WebAssembly][Modules] Support Wasm module loading in WebCore

Bug 236268 - [WebAssembly][Modules] Support Wasm module loading in WebCore

Summary: [WebAssembly][Modules] Support Wasm module loading in WebCore

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebCore JavaScript (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Nobody

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2022-02-07 15:56 PST by Asumu Takikawa
Modified:	2024-02-28 14:22 PST (History)
CC List:	16 users (show)

See Also:

Attachments
Patch (73.61 KB, patch) 2022-02-07 16:38 PST, Asumu Takikawa	no flags	Details \| Formatted Diff \| Diff
Patch (84.81 KB, patch) 2022-02-08 09:15 PST, Asumu Takikawa	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch (84.95 KB, patch) 2022-02-08 09:50 PST, Asumu Takikawa	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch (85.29 KB, patch) 2022-02-08 11:28 PST, Asumu Takikawa	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch (85.24 KB, patch) 2022-02-08 12:00 PST, Asumu Takikawa	no flags	Details \| Formatted Diff \| Diff
Patch (99.11 KB, patch) 2022-02-16 12:27 PST, Asumu Takikawa	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch (99.26 KB, patch) 2022-02-16 12:49 PST, Asumu Takikawa	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch (99.20 KB, patch) 2022-02-16 13:27 PST, Asumu Takikawa	no flags	Details \| Formatted Diff \| Diff
Patch (99.85 KB, patch) 2022-02-16 17:51 PST, Asumu Takikawa	no flags	Details \| Formatted Diff \| Diff
Patch (99.52 KB, patch) 2022-02-17 17:08 PST, Asumu Takikawa	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch (99.54 KB, patch) 2022-02-17 17:36 PST, Asumu Takikawa	no flags	Details \| Formatted Diff \| Diff
Patch (99.65 KB, patch) 2022-02-17 23:16 PST, Asumu Takikawa	no flags	Details \| Formatted Diff \| Diff
Patch (100.52 KB, patch) 2022-02-21 17:14 PST, Asumu Takikawa	no flags	Details \| Formatted Diff \| Diff
Patch (98.97 KB, patch) 2022-02-21 17:24 PST, Asumu Takikawa	no flags	Details \| Formatted Diff \| Diff
Patch (101.33 KB, patch) 2022-02-21 19:02 PST, Asumu Takikawa	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (14) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Asumu Takikawa 2022-02-07 15:56:19 PST

Wasm modules as ES modules has been implemented in JSC for a while: https://bugs.webkit.org/show_bug.cgi?id=184600 but loading from WebCore is not yet supported.

There is a draft PR to the HTML spec that would specify the loading behavior: https://github.com/whatwg/html/pull/4372 (to be updated soon)

It could make sense to implement this as an experimental feature behind a flag until the HTML spec language is more stable.

Comment 1 Asumu Takikawa 2022-02-07 16:38:38 PST

Created attachment 451171 [details]
Patch

Comment 2 EWS Watchlist 2022-02-07 16:40:16 PST

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Comment 3 Sam Weinig 2022-02-07 16:43:57 PST

Comment on attachment 451171 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/wasm/webapi/esm-integration/invalid-bytecode.tentative.html:1
> +<!DOCTYPE html><!-- webkit-test-runner [ WebAssemblyESMIntegrationEnabled=true ] -->

You should not need these header comment commands since experimental features are enabled by default in the test runner.

Comment 4 Asumu Takikawa 2022-02-08 09:15:28 PST

Created attachment 451259 [details]
Patch

Comment 5 Asumu Takikawa 2022-02-08 09:17:16 PST

(In reply to Sam Weinig from comment #3)
> You should not need these header comment commands since experimental
> features are enabled by default in the test runner.

Thanks for the info, I've removed these now.

I think I've fixed the MacOS build issues too, but I'll see what the CI does.

Comment 6 Asumu Takikawa 2022-02-08 09:50:26 PST

Created attachment 451265 [details]
Patch

Comment 7 Asumu Takikawa 2022-02-08 11:28:42 PST

Created attachment 451282 [details]
Patch

Comment 8 Asumu Takikawa 2022-02-08 12:00:54 PST

Created attachment 451287 [details]
Patch

Comment 9 Yusuke Suzuki 2022-02-12 23:44:31 PST

Comment on attachment 451287 [details]
Patch

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

The direction looks good. I think we need to avoid copying data, but if it is done, the patch looks good to me

> Source/WebCore/bindings/js/ScriptController.cpp:247
> +    const bool isWasmModule = jsDynamicCast<WebAssemblyModuleRecord*>(vm, &moduleRecord);

Use `moduleRecord.inherits<WebAssemblyModuleRecord>(vm)`.

> Source/WebCore/bindings/js/WebAssemblyScriptSourceCode.h:49
> +        : m_provider(JSC::WebAssemblySourceProvider::create(cachedScript->resourceBuffer()->copyData(), JSC::SourceOrigin { cachedScript->response().url(), WTFMove(scriptFetcher) }, cachedScript->response().url().string()))

Can we avoid copying data here? We should do similarly to CachedScriptSourceProvider.
Without that, we will use much more memory.

> Source/WebCore/bindings/js/WebAssemblyScriptSourceCode.h:56
> +        : m_provider(JSC::WebAssemblySourceProvider::create(source->copy()->extractData(), JSC::SourceOrigin { url, WTFMove(scriptFetcher) }, url.string()))

Can we avoid extracting data? I think we should do similarly to ScriptBufferSourceProvider.

> Source/WebCore/platform/MIMETypeRegistry.cpp:500
> +bool MIMETypeRegistry::isSupportedWebAssemblyMIMEType(const String& mimeType)
> +{
> +    return equalLettersIgnoringASCIICase(mimeType, "application/wasm");
> +}

This looks good. We should replace FetchResponse::hasWasmMIMEType's implementation with the one using this function.

Comment 10 Yusuke Suzuki 2022-02-13 22:35:55 PST

One thing I would like to comment is that, WebCore can replace underlying values of CachedResource to shrink memory footprint. (NetworkProcess can decide to put the data on disk, allocate disk-backed mmap-ed data, notify it to WebContent process, and replace the existing one with this disk-backed one).

I think,
1. We should ensure that wasm's CachedResource is categorilized as mayTryReplaceEncodedData() => true
2. Then, when reading data, let's hold underlying Ref<SharedBuffer> while touching (but not holding it forever), since it can be replaced with new mmap-ed one.

Comment 11 Radar WebKit Bug Importer 2022-02-14 15:57:19 PST

<rdar://problem/88933504>

Comment 12 Asumu Takikawa 2022-02-16 12:27:27 PST

Created attachment 452228 [details]
Patch

Comment 13 Asumu Takikawa 2022-02-16 12:49:40 PST

Created attachment 452232 [details]
Patch

Comment 14 Asumu Takikawa 2022-02-16 13:02:18 PST

Thanks for the review! The new patch should address the feedback.

In particular, I added `WebAssemblyCachedScriptSourceProvider.cpp` that should avoid the buffer copying. In the Worker case (where `extractData` was being called), I've made it an error case instead because this patch doesn't fully support workers yet anyway. I will address that in follow-up patches.

Also added an assert of `mayTryReplaceEncodedData()` and the new source provider should also hold onto the underlying ScriptResource buffer via a `RefPtr`.

To accommodate this kind of source provider, I made a `BaseWebAssemblySourceProvider` with a more general pointer & size interface that can accommodate the CachedScript case. The normal `WebAssemblySourceProvider` is now a subclass of it, and still uses a `Vector` representation (only exposed for `CachedTypes.cpp` use).

Added two worker+wasm WPT tests as well (these don't pass yet, but make sure it doesn't crash).

Comment 15 Asumu Takikawa 2022-02-16 13:27:15 PST

Created attachment 452242 [details]
Patch

Comment 16 Asumu Takikawa 2022-02-16 13:28:47 PST

Quick note: asserting `mayTryReplaceEncodedData()` doesn't actually work as it's private, but it should be fine as the Wasm source provider takes a `CachedScript` which has the field set to true and is final.

Comment 17 Asumu Takikawa 2022-02-16 17:51:43 PST

Created attachment 452280 [details]
Patch

Comment 18 Asumu Takikawa 2022-02-17 17:08:41 PST

Created attachment 452449 [details]
Patch

Comment 19 Asumu Takikawa 2022-02-17 17:36:52 PST

Created attachment 452454 [details]
Patch

Comment 20 Asumu Takikawa 2022-02-17 23:16:06 PST

Created attachment 452489 [details]
Patch

Comment 21 Yusuke Suzuki 2022-02-17 23:23:34 PST

Comment on attachment 452489 [details]
Patch

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

> Source/WebCore/bindings/js/WebAssemblyCachedScriptSourceProvider.h:66
> +    const uint8_t* data() override
> +    {
> +        if (!m_buffer)
> +            return nullptr;
> +
> +        if (!m_buffer->isContiguous())
> +            m_buffer = m_buffer->makeContiguous();
> +
> +        return downcast<SharedBuffer>(*m_buffer).data();
> +    }

I have a question. We will load a value from non main thread (see EntryPlan::parseAndValidateModule.). So then, it can happen.

1. We are loading bytes from non main thread via EntryPlan::parseAndValidateModule
2. The main thread will replace underlying data with file-backed resource.

So,

1. How is it protected? Is it OK because m_buffer is keeping the original data alive while parsing?
2. If it is true, when will we release m_buffer's data? Otherwise, we will keep two copy of data.

Comment 22 Yusuke Suzuki 2022-02-17 23:31:34 PST

Comment on attachment 452489 [details]
Patch

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

>> Source/WebCore/bindings/js/WebAssemblyCachedScriptSourceProvider.h:66
>> +    }
> 
> I have a question. We will load a value from non main thread (see EntryPlan::parseAndValidateModule.). So then, it can happen.
> 
> 1. We are loading bytes from non main thread via EntryPlan::parseAndValidateModule
> 2. The main thread will replace underlying data with file-backed resource.
> 
> So,
> 
> 1. How is it protected? Is it OK because m_buffer is keeping the original data alive while parsing?
> 2. If it is true, when will we release m_buffer's data? Otherwise, we will keep two copy of data.

My idea is that, while accessing bytes in wasm compiler, we should retain/lock the underlying `FragmentedSharedBuffer`. So, even while the main thread replaces CachedScript's data, then still the read bytes are valid.
And after reading it, let's release it. So then, it will be destroyed if it is not retained by the other client.

Comment 23 Asumu Takikawa 2022-02-21 17:14:25 PST

Created attachment 452802 [details]
Patch

Comment 24 Asumu Takikawa 2022-02-21 17:24:08 PST

Created attachment 452803 [details]
Patch

Comment 25 Yusuke Suzuki 2022-02-21 17:52:15 PST

Comment on attachment 452803 [details]
Patch

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

r=me

> Source/JavaScriptCore/parser/SourceProvider.h:140
> +        virtual void initBuffer() { }
> +        virtual void cleanupBuffer() { }

Maybe, lockUnderlyingBuffer / unlockUnderlyingBuffer would be better names.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h:66
>      auto throwScope = DECLARE_THROW_SCOPE(vm);

Let's take const WebAssemblySourceProviderBufferGuard& parameter to ensure that this is called after ensuring that. (Like `const AbstractLocker&` parameter)

> Source/WebCore/bindings/js/WebAssemblyCachedScriptSourceProvider.h:40
> +class WebAssemblyCachedScriptSourceProvider : public JSC::BaseWebAssemblySourceProvider, public CachedResourceClient {

Let's put `final`'

> Source/WebCore/bindings/js/WebAssemblyCachedScriptSourceProvider.h:69
> +    {

Let's add `ASSERT(!m_buffer)`

> Source/WebCore/bindings/js/WebAssemblyCachedScriptSourceProvider.h:74
> +    {

Let's add `ASSERT(m_buffer)`

Comment 26 Asumu Takikawa 2022-02-21 19:02:43 PST

Created attachment 452810 [details]
Patch

Comment 27 EWS 2022-02-22 05:22:11 PST

Committed r290300 (247623@main): <https://commits.webkit.org/247623@main>

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