Bug 219527 - Align %TypedArray% constructor behavior with spec
Summary: Align %TypedArray% constructor behavior with spec
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: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-03 19:25 PST by Ross Kirsling
Modified: 2020-12-08 13:04 PST (History)
8 users (show)

See Also:


Attachments
Patch (16.14 KB, patch)
2020-12-03 19:47 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (17.76 KB, patch)
2020-12-03 23:06 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (21.83 KB, patch)
2020-12-07 15:29 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (32.34 KB, patch)
2020-12-08 12:17 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2020-12-03 19:25:56 PST
Align %TypedArray% constructor behavior with spec
Comment 1 Ross Kirsling 2020-12-03 19:47:56 PST
Created attachment 415379 [details]
Patch
Comment 2 Ross Kirsling 2020-12-03 23:06:08 PST
Created attachment 415391 [details]
Patch
Comment 3 Ross Kirsling 2020-12-03 23:13:20 PST
Comment on attachment 415391 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:21
> +        3. Typed array argument (https://tc39.es/ecma262/#sec-initializetypedarrayfromtypedarray):
> +           We need to support the case where the input typed array uses a custom ArrayBuffer.
> +           This case is *extremely* strange -- we still create the same type of typed array with a normal ArrayBuffer,
> +           but we override the prototype of that ArrayBuffer to input.buffer.constructor[@@species].prototype.

Note: There appears to be interest in removing @@species for ArrayBuffers, so it might be useful to have JSC continue to fail these tests as "web compatibility insurance". In that case, we could split (3) out from the rest.
Comment 4 Ross Kirsling 2020-12-04 13:02:08 PST
Oops, I forgot that the relevant proposal about the @@species removal is https://github.com/tc39/proposal-rm-builtin-subclassing.

But it sounds like folks would like to remove @@species from ArrayBuffers even if web compat issues were discovered for other classes.
Comment 5 Yusuke Suzuki 2020-12-04 13:52:10 PST
Comment on attachment 415391 [details]
Patch

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

Looks good! But I think we should have species optimization at the first patch since new TypedArray(arrayBuffer) is common enough.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:115
> +    JSValue constructor = source->get(globalObject, vm.propertyNames->constructor);

Let's avoid doing property access by having watchpoint like Array / ArrayBuffer are doing since `new TypedArray(arrayBuffer)` is enough critical path.
See speciesConstructArrayBuffer and speciesConstructArray.
Comment 6 Yusuke Suzuki 2020-12-04 13:53:54 PST
Comment on attachment 415391 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:115
>> +    JSValue constructor = source->get(globalObject, vm.propertyNames->constructor);
> 
> Let's avoid doing property access by having watchpoint like Array / ArrayBuffer are doing since `new TypedArray(arrayBuffer)` is enough critical path.
> See speciesConstructArrayBuffer and speciesConstructArray.

Possibly I think modifying speciesConstructArrayBuffer and reusing it would be possible.
Comment 7 Ross Kirsling 2020-12-07 15:29:22 PST
Created attachment 415594 [details]
Patch
Comment 8 Yusuke Suzuki 2020-12-07 21:36:18 PST
Comment on attachment 415594 [details]
Patch

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

Nice, r=me

> Source/JavaScriptCore/runtime/JSArrayBufferPrototype.cpp:68
> +    VM& vm = globalObject->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);
> +
> +    bool isValid = speciesWatchpointIsValid(vm, thisObject, mode);
> +    scope.assertNoException();
> +    if (LIKELY(isValid))
> +        return WTF::nullopt;
> +

Can you make this part inlined when the caller is calling this? (like, definition it in JSArrayBufferPrototypeInlines.h)
I think 99.9999999% of path will go to this "watchpoint is valid" case. So it is worth inlining.
Comment 9 Ross Kirsling 2020-12-08 12:17:09 PST
Created attachment 415661 [details]
Patch for landing
Comment 10 EWS 2020-12-08 13:03:14 PST
Committed r270552: <https://trac.webkit.org/changeset/270552>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415661 [details].
Comment 11 Radar WebKit Bug Importer 2020-12-08 13:04:15 PST
<rdar://problem/72106111>