Bug 220189

Summary: [JSC] Update WebAssembly instance's exports object
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ashvayka, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch ashvayka: review+

Yusuke Suzuki
Reported 2020-12-28 21:29:56 PST
[JSC] Update WebAssembly instance's exports object
Attachments
Patch (41.63 KB, patch)
2020-12-28 21:37 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (44.32 KB, patch)
2020-12-29 01:47 PST, Yusuke Suzuki 		ashvayka: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2020-12-28 21:37:58 PST
Created attachment 416835 [details] Patch
Yusuke Suzuki
Comment 2 2020-12-29 01:47:42 PST
Created attachment 416836 [details] Patch
Alexey Shvayka
Comment 3 2020-12-30 09:49:07 PST
Comment on attachment 416836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416836&action=review r=me > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:512 > + objectConstructorFreeze(globalObject, exportsObject); Nice: the fast path for final objects will be taken.
Alexey Shvayka
Comment 4 2020-12-30 10:32:19 PST
Comment on attachment 416836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416836&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:513 > + RETURN_IF_EXCEPTION(scope, void()); Is propertyName guaranteed to be non-index? putDirect() has an assert for that. If it's never an index, we can just do `exportsObject->freeze(vm)` and remove this exception check. Otherwise, we should use putDirectMaybeIndex() and scope.assertNoException() since SetIntegrityLevel can't throw per spec, nor it can return `false`.
Yusuke Suzuki
Comment 5 2020-12-30 15:52:28 PST
Comment on attachment 416836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416836&action=review Thanks! >> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:513 >> + RETURN_IF_EXCEPTION(scope, void()); > > Is propertyName guaranteed to be non-index? putDirect() has an assert for that. > If it's never an index, we can just do `exportsObject->freeze(vm)` and remove this exception check. > Otherwise, we should use putDirectMaybeIndex() and scope.assertNoException() since SetIntegrityLevel can't throw per spec, nor it can return `false`. Oops, nice catch. I don't think this is guaranteed. I'll add some tests & use putDirectIndex if it is index.
Yusuke Suzuki
Comment 6 2020-12-30 16:05:25 PST
Committed r271112: <https://trac.webkit.org/changeset/271112>
Radar WebKit Bug Importer
Comment 7 2020-12-30 16:06:15 PST
<rdar://problem/72744953>
Saam Barati
Comment 8 2021-01-05 12:13:54 PST
This seems to have made richards-wasm 70% slower in JS2
Saam Barati
Comment 9 2021-01-05 12:14:13 PST
(In reply to Saam Barati from comment #8) > This seems to have made richards-wasm 70% slower in JS2 The "runtime" score got 150% worse. Compile times are not effected
Saam Barati
Comment 10 2021-01-05 12:18:17 PST
My guess is this "breaks" the JS -> Wasm call fast path
Yusuke Suzuki
Comment 11 2021-01-05 12:21:29 PST
Looking.
Yusuke Suzuki
Comment 12 2021-01-05 14:19:34 PST
(In reply to Yusuke Suzuki from comment #11) > Looking. Found a fun issue. Will fix it soon in https://bugs.webkit.org/show_bug.cgi?id=220339
Note You need to log in before you can comment on or make changes to this bug.