Bug 236268

Summary:

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

Product:

WebKit

Reporter:

Asumu Takikawa <asumu>

Component:

WebCore JavaScript

Assignee:

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:

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

Asumu Takikawa

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

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 attachment proposed patch, testcase, etc.

Asumu Takikawa

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

Created attachment 451171 [details] Patch

EWS Watchlist

Comment 2 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

Sam Weinig

Comment 3 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> You should not need these header comment commands since experimental features are enabled by default in the test runner.

Asumu Takikawa

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

Created attachment 451259 [details] Patch

Asumu Takikawa

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

Asumu Takikawa

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

Created attachment 451265 [details] Patch

Asumu Takikawa

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

Created attachment 451282 [details] Patch

Asumu Takikawa

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

Created attachment 451287 [details] Patch

Yusuke Suzuki

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

Yusuke Suzuki

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

Radar WebKit Bug Importer

Comment 11 2022-02-14 15:57:19 PST

<rdar://problem/88933504>

Asumu Takikawa

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

Created attachment 452228 [details] Patch

Asumu Takikawa

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

Created attachment 452232 [details] Patch

Asumu Takikawa

Comment 14 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).

Asumu Takikawa

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

Created attachment 452242 [details] Patch

Asumu Takikawa

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

Asumu Takikawa

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

Created attachment 452280 [details] Patch

Asumu Takikawa

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

Created attachment 452449 [details] Patch

Asumu Takikawa

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

Created attachment 452454 [details] Patch

Asumu Takikawa

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

Created attachment 452489 [details] Patch

Yusuke Suzuki

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

Yusuke Suzuki

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

Asumu Takikawa

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

Created attachment 452802 [details] Patch

Asumu Takikawa

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

Created attachment 452803 [details] Patch

Yusuke Suzuki

Comment 25 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)`

Asumu Takikawa

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

Created attachment 452810 [details] Patch

EWS

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

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