Bug 220189 - [JSC] Update WebAssembly instance's exports object
Summary: [JSC] Update WebAssembly instance's exports object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-28 21:29 PST by Yusuke Suzuki
Modified: 2021-01-05 14:19 PST (History)
8 users (show)

See Also:

Attachments
Patch (41.63 KB, patch)
2020-12-28 21:37 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (44.32 KB, patch)
2020-12-29 01:47 PST, Yusuke Suzuki 		ashvayka: review+
Details | Formatted Diff | Diff
Show Obsolete (1) 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 Yusuke Suzuki 2020-12-28 21:29:56 PST 
[JSC] Update WebAssembly instance's exports object
Comment 1 Yusuke Suzuki 2020-12-28 21:37:58 PST 
Created attachment 416835 [details]
Patch
Comment 2 Yusuke Suzuki 2020-12-29 01:47:42 PST 
Created attachment 416836 [details]
Patch
Comment 3 Alexey Shvayka 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.
Comment 4 Alexey Shvayka 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`.
Comment 5 Yusuke Suzuki 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.
Comment 6 Yusuke Suzuki 2020-12-30 16:05:25 PST 
Committed r271112: <https://trac.webkit.org/changeset/271112>
Comment 7 Radar WebKit Bug Importer 2020-12-30 16:06:15 PST 
<rdar://problem/72744953>
Comment 8 Saam Barati 2021-01-05 12:13:54 PST 
This seems to have made richards-wasm 70% slower in JS2
Comment 9 Saam Barati 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
Comment 10 Saam Barati 2021-01-05 12:18:17 PST 
My guess is this "breaks" the JS -> Wasm call fast path
Comment 11 Yusuke Suzuki 2021-01-05 12:21:29 PST 
Looking.
Comment 12 Yusuke Suzuki 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