Bug 236268

Summary: [WebAssembly][Modules] Support Wasm module loading in WebCore
Product: WebKit Reporter: Asumu Takikawa <asumu>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bashorov, clopez, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, msaboff, ryuan.choi, saam, sam, sergio, tzagallo, webkit-bug-importer, youennf, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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