Bug 219527 – Align %TypedArray% constructor behavior with spec

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

Align %TypedArray% constructor behavior with spec

[Ross Kirsling]

Created attachment 415379 [details]
Patch

[Ross Kirsling]

Created attachment 415391 [details]
Patch

[Ross Kirsling]

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.

[Ross Kirsling]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Ross Kirsling]

Created attachment 415594 [details]
Patch

[Yusuke Suzuki]

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.

[Ross Kirsling]

Created attachment 415661 [details]
Patch for landing

[EWS]

Committed r270552: <https://trac.webkit.org/changeset/270552>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415661 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72106111>