Bug 234604 – Implement Change Array by copy proposal

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Aditi Singh]

Proposal Change Array by copy provides additional methods on `Array.prototype` and `TypedArray.prototype` to enable changes on the array by returning a new copy of it with the change.

Proposal: https://github.com/tc39/proposal-change-array-by-copy
Spec: https://tc39.es/proposal-change-array-by-copy/#sec-properties-of-the-array-prototype-object

[Radar WebKit Bug Importer]

<rdar://problem/86984042>

[Aditi Singh]

Created attachment 449727 [details]
Patch

[Aditi Singh]

Created attachment 449729 [details]
Patch

[Alexey Shvayka]

Comment on attachment 449729 [details]
Patch

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

Great job, I appreciate the 1:1 precision with the spec. It's a bit uneasy to find matching bytecodes / helpers for all the abstract ops, but you did that perfectly!

I will put a r+ once we sort out the @arraySortHelper.

> Source/JavaScriptCore/ChangeLog:7
> +

nit: Could you please link the proposal in ChangeLog? And possibly mention results of microbenchmarks OR why a LinkTimeConstant is added?

> Source/JavaScriptCore/builtins/ArrayPrototype.js:591
> +    return @arraySortHelper(this, comparator);

While this call will most likely be inlined by DFG / FTL, I'm a bit worried of performance effect for lower tiers.
Could you please either a) run microbenchmarks or rather b) @call Array.prototype.sort in toSorted()?

To a) run the benchmarks:

JSC_useJIT=false Tools/Scripts/run-jsc-benchmarks trunk:../path/to/WebKit/WebKitBuild/Release/jsc patch:../path/to/another/WebKit/WebKitBuild/Release/jsc --benchmarks 'sort' --outer 50
JSC_useDFGJIT=false Tools/Scripts/run-jsc-benchmarks trunk:../path/to/WebKit/WebKitBuild/Release/jsc patch:../path/to/another/WebKit/WebKitBuild/Release/jsc --benchmarks 'sort' --outer 50

Option b) is a bit more tricky: we need to expose Array.prototype.sort as a global private as well, while preferably avoiding creation of a second JSFunction*.
I suggest adding a LinkTimeConstant "arraySort" and initializing it in JSGlobalObject::init() the same way it's done for vm.propertyNames->hasOwnProperty, ending up with `@arraySort.@call(result, comparefn)` in toSorted().

> Source/JavaScriptCore/builtins/ArrayPrototype.js:806
> +    var O = @toObject(this, "Array.prototype.toReversed requires that |this| not be null or undefined");

nit: I believe we don't do single-char variable names (with an exception for iteration vars) even to match the spec. "object" perhaps?

> Source/JavaScriptCore/builtins/ArrayPrototype.js:950
> +        if(k === actualIndex)

nit: missing space

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:489
> +        dc = dc > 0 ? dc : 0;

nit: maybe "tempDeleteCount"?

[Yusuke Suzuki]

Comment on attachment 449729 [details]
Patch

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

> Source/JavaScriptCore/builtins/ArrayPrototype.js:590
> +{

When defining a new function, let's put "use strict" to ensure it is strict code.

>> Source/JavaScriptCore/builtins/ArrayPrototype.js:591
>> +    return @arraySortHelper(this, comparator);
> 
> While this call will most likely be inlined by DFG / FTL, I'm a bit worried of performance effect for lower tiers.
> Could you please either a) run microbenchmarks or rather b) @call Array.prototype.sort in toSorted()?
> 
> To a) run the benchmarks:
> 
> JSC_useJIT=false Tools/Scripts/run-jsc-benchmarks trunk:../path/to/WebKit/WebKitBuild/Release/jsc patch:../path/to/another/WebKit/WebKitBuild/Release/jsc --benchmarks 'sort' --outer 50
> JSC_useDFGJIT=false Tools/Scripts/run-jsc-benchmarks trunk:../path/to/WebKit/WebKitBuild/Release/jsc patch:../path/to/another/WebKit/WebKitBuild/Release/jsc --benchmarks 'sort' --outer 50
> 
> Option b) is a bit more tricky: we need to expose Array.prototype.sort as a global private as well, while preferably avoiding creation of a second JSFunction*.
> I suggest adding a LinkTimeConstant "arraySort" and initializing it in JSGlobalObject::init() the same way it's done for vm.propertyNames->hasOwnProperty, ending up with `@arraySort.@call(result, comparefn)` in toSorted().

Agreed. In (a) and (b), I think (b) is better.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:908
> +    for (var i = 0; i < insertCount; i++, k++) {
> +        @putByValDirect(result, k, arguments[i + 2]);
> +    }

Let's remove {}.

[Aditi Singh]

Created attachment 450543 [details]
Patch

[Alexey Shvayka]

Comment on attachment 450543 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1334
> +        JSValue arraySort = jsCast<JSFunction*>(arrayPrototype()->get(this, vm.propertyNames->builtinNames().sortPublicName()));

vm.propertyNames->builtinNames().sortPublicName() should be the same as vm.propertyNames->sort, and they are both public identifiers, meaning userland code can delete them and break newly added methods (if we implement helpers via initLater()).
A nice example for setting up both public and private identifiers would be arrayProtoFuncShift in ArrayPrototype::finishCreation().

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1340
> +        JSValue typedArrayPrototypeSort = jsCast<JSFunction*>(typedArrayViewPrototype()->get(this, vm.propertyNames->sort));

Doesn't this line initialize TypedArrayPrototype? It's quite large and not very commonly used on the web, so we would prefer avoiding eager init.
Could you please implement @typedArrayPrototypeSort (and @arraySort along with it, purely for consistency) via initLater()?
arrayPrototype() can be accessed from `jsCast<JSGlobalObject*>(init.owner)`.

[Yusuke Suzuki]

Comment on attachment 450543 [details]
Patch

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

This is really nice!

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1334
>> +        JSValue arraySort = jsCast<JSFunction*>(arrayPrototype()->get(this, vm.propertyNames->builtinNames().sortPublicName()));
> 
> vm.propertyNames->builtinNames().sortPublicName() should be the same as vm.propertyNames->sort, and they are both public identifiers, meaning userland code can delete them and break newly added methods (if we implement helpers via initLater()).
> A nice example for setting up both public and private identifiers would be arrayProtoFuncShift in ArrayPrototype::finishCreation().

Let's do the same to the below's my comment.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1340
>> +        JSValue typedArrayPrototypeSort = jsCast<JSFunction*>(typedArrayViewPrototype()->get(this, vm.propertyNames->sort));
> 
> Doesn't this line initialize TypedArrayPrototype? It's quite large and not very commonly used on the web, so we would prefer avoiding eager init.
> Could you please implement @typedArrayPrototypeSort (and @arraySort along with it, purely for consistency) via initLater()?
> arrayPrototype() can be accessed from `jsCast<JSGlobalObject*>(init.owner)`.

Right. I suggest the following.

1. Defining sort function as m_linkTimeConstants' lazy function.
2. Defining `JSGlobalObject::typedArrayProtoSortFunction()` getter which returns it.
3. Use that getter in TypedArrayPrototype's creation.

So, we can set this function in LinkTimeConstant while we avoid earger materialization here.
There are several existing examles, `m_objectProtoToStringFunction` etc.

[Joseph Pecoraro]

Comment on attachment 450543 [details]
Patch

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

Cool!

> Source/JavaScriptCore/builtins/TypedArrayPrototype.js:434
> +        @throwTypeError("TypedArray.prototype.toSorted requires the comparefn argument to be a function or undefined");

The Array version says "the comparator argument" which sounds better than `comparefn argument`.

[Darin Adler]

Comment on attachment 450543 [details]
Patch

Many failing tests. Need another try.

[Aditi Singh]

> 1. Defining sort function as m_linkTimeConstants' lazy function.
> 2. Defining `JSGlobalObject::typedArrayProtoSortFunction()` getter which returns it.
> 3. Use that getter in TypedArrayPrototype's creation.

I followed these steps, and defined a m_linkTimeConstants' lazy function for `TypedArray.prototype.sort()`. But I am curious, why do we need to define a LinkTimeConstant for sort? 
If we expose `TypedArray.prototype.sort()` as a globalPrivate and use it as 
`@sort.@call(array, comparefn)` in `TypedArray.prototype.toSorted()`, the modifications in userland code don't seem to affect the `TypedArray.prototype.toSorted()` function.

[Yusuke Suzuki]

(In reply to Aditi Singh from comment #11)
> > 1. Defining sort function as m_linkTimeConstants' lazy function.
> > 2. Defining `JSGlobalObject::typedArrayProtoSortFunction()` getter which returns it.
> > 3. Use that getter in TypedArrayPrototype's creation.
> 
> I followed these steps, and defined a m_linkTimeConstants' lazy function for
> `TypedArray.prototype.sort()`. But I am curious, why do we need to define a
> LinkTimeConstant for sort? 
> If we expose `TypedArray.prototype.sort()` as a globalPrivate and use it as 
> `@sort.@call(array, comparefn)` in `TypedArray.prototype.toSorted()`, the
> modifications in userland code don't seem to affect the
> `TypedArray.prototype.toSorted()` function.

Because @globalPrivate does not define `sort` prototype function.
You need to have

function sort(...)
{
    return @sort(...)
}

and it is slower than our suggested way.

[Aditi Singh]

(In reply to Yusuke Suzuki from comment #12)
> (In reply to Aditi Singh from comment #11)
> > > 1. Defining sort function as m_linkTimeConstants' lazy function.
> > > 2. Defining `JSGlobalObject::typedArrayProtoSortFunction()` getter which returns it.
> > > 3. Use that getter in TypedArrayPrototype's creation.
> > 
> > I followed these steps, and defined a m_linkTimeConstants' lazy function for
> > `TypedArray.prototype.sort()`. But I am curious, why do we need to define a
> > LinkTimeConstant for sort? 
> > If we expose `TypedArray.prototype.sort()` as a globalPrivate and use it as 
> > `@sort.@call(array, comparefn)` in `TypedArray.prototype.toSorted()`, the
> > modifications in userland code don't seem to affect the
> > `TypedArray.prototype.toSorted()` function.
> 
> Because @globalPrivate does not define `sort` prototype function.
> You need to have
> 
> function sort(...)
> {
>     return @sort(...)
> }
> 
> and it is slower than our suggested way.

Hi! I added the LinkTimeConstant for `%%TypedArray%%.prototype.sort()`, but I am unable to figure out a way to access the LinkTimeConstant in the toSorted() function without exposing `%%TypedArray%%.prototype.sort()` as a @globalPrivate. Without declaring it as a @globalPrivate I keep getting the error:
`ReferenceError: Can't find private variable: PrivateSymbol.sort`

[Yusuke Suzuki]

(In reply to Aditi Singh from comment #13)
> (In reply to Yusuke Suzuki from comment #12)
> > (In reply to Aditi Singh from comment #11)
> > > > 1. Defining sort function as m_linkTimeConstants' lazy function.
> > > > 2. Defining `JSGlobalObject::typedArrayProtoSortFunction()` getter which returns it.
> > > > 3. Use that getter in TypedArrayPrototype's creation.
> > > 
> > > I followed these steps, and defined a m_linkTimeConstants' lazy function for
> > > `TypedArray.prototype.sort()`. But I am curious, why do we need to define a
> > > LinkTimeConstant for sort? 
> > > If we expose `TypedArray.prototype.sort()` as a globalPrivate and use it as 
> > > `@sort.@call(array, comparefn)` in `TypedArray.prototype.toSorted()`, the
> > > modifications in userland code don't seem to affect the
> > > `TypedArray.prototype.toSorted()` function.
> > 
> > Because @globalPrivate does not define `sort` prototype function.
> > You need to have
> > 
> > function sort(...)
> > {
> >     return @sort(...)
> > }
> > 
> > and it is slower than our suggested way.
> 
> Hi! I added the LinkTimeConstant for `%%TypedArray%%.prototype.sort()`, but
> I am unable to figure out a way to access the LinkTimeConstant in the
> toSorted() function without exposing `%%TypedArray%%.prototype.sort()` as a
> @globalPrivate. Without declaring it as a @globalPrivate I keep getting the
> error:
> `ReferenceError: Can't find private variable: PrivateSymbol.sort`

I mean, defining this as @globalPrivate, and putting the function to TypedArray prototype manually instead.
See, m_promiseResolveFunction for example. And let's get it and put it in TypedArray's prototype instead of using lut table.

[Aditi Singh]

Created attachment 456816 [details]
Patch

[Aditi Singh]

(In reply to Aditi Singh from comment #15)
> Created attachment 456816 [details]
> Patch

Hi, apologies for the delay. 
There have been a few changes in change-array-by-copy proposal since my last patch, but none of them were observable. 
As per the previous review comments, I was supposed to define a m_linkTimeConstants' lazy function for `%%TypedArray%%.prototype.sort()`. Is this the correct way to do it?

[Alexey Shvayka]

Comment on attachment 456816 [details]
Patch

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

Great job, lazy function is correct, no worries on delay.

r=me with just 2 nits.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:155
> +        Options::useChangeArrayByCopyMethods() ? &vm.propertyNames->builtinNames().toSplicedPublicName() : nullptr,

"with" appears to be missing from unscopables, failing the tests.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1353
> +        JSValue arraySort = jsCast<JSFunction*>(arrayPrototype()->get(this, vm.propertyNames->builtinNames().sortPublicName()));

I understand it's a copy of what we have for "hasOwnProperty", but can we please do something like

    JSFunction* arrayStore = jsCast<JSFunction*>(arrayPrototype()->getDirect(this, vm.propertyNames->builtinNames().sortPublicName()));

removing the RELEASE_ASSERT and extra cast below.

[Aditi Singh]

(In reply to Alexey Shvayka from comment #17)
> Comment on attachment 456816 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456816&action=review
> 
> Great job, lazy function is correct, no worries on delay.
> 
> r=me with just 2 nits.
> 
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:155
> > +        Options::useChangeArrayByCopyMethods() ? &vm.propertyNames->builtinNames().toSplicedPublicName() : nullptr,
> 
> "with" appears to be missing from unscopables, failing the tests.

Oh, I did not add "with" to the unscopables because it was specified in the spec text: "The reason that "with" is not included in the unscopableList is because it is already a reserved word."

For reference: https://tc39.es/proposal-change-array-by-copy/#sec-array.prototype-@@unscopables

> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1353
> > +        JSValue arraySort = jsCast<JSFunction*>(arrayPrototype()->get(this, vm.propertyNames->builtinNames().sortPublicName()));
> 
> I understand it's a copy of what we have for "hasOwnProperty", but can we
> please do something like
> 
>     JSFunction* arrayStore =
> jsCast<JSFunction*>(arrayPrototype()->getDirect(this,
> vm.propertyNames->builtinNames().sortPublicName()));
> 
> removing the RELEASE_ASSERT and extra cast below.

👍

[Aditi Singh]

Created attachment 457159 [details]
Patch

[Alexey Shvayka]

Comment on attachment 457159 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:7
> +        This patch implements proposal Change Array By Copy. The proposal provides additional 

nit: please add an extra newline before this.

> Source/JavaScriptCore/ChangeLog:10
> +        Since, a call to intrinsic functions: Array.prototype.sort(comparefn) and 

It would be nice if you could expand the ChangeLog regarding why we treat TypedArray's sort() and Array's sort() differently (i.e. not all sites use TypedArray), and why it's important to avoid

    function sort(cmp) { return @arraySort.@call(this, cmp); }

for performance reasons.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1353
> +        JSValue arraySort = jsCast<JSFunction*>(arrayPrototype()->getDirect(vm, vm.propertyNames->builtinNames().sortPublicName()));

nit: JSFunction* arraySort = ...

> JSTests/stress/unscopables.js:1
> +//@ requireOptions("--useAtMethod=1 --useChangeArrayByCopyMethods=1")

Aha, JSC tests fail because expected syntax is:

    //@ requireOptions("--useAtMethod=1", "--useChangeArrayByCopyMethods=1")

> JSTests/stress/unscopables.js:14
> +    test(String(Object.keys(unscopables).sort()), "at,copyWithin,entries,fill,find,findIndex,findLast,findLastIndex,flat,flatMap,includes,keys,toReversed,toSorted,toSpliced,values,with");

Should "with" be removed from here?

[Alexey Shvayka]

(In reply to Aditi Singh from comment #18)
> (In reply to Alexey Shvayka from comment #17)
> > > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:155
> > > +        Options::useChangeArrayByCopyMethods() ? &vm.propertyNames->builtinNames().toSplicedPublicName() : nullptr,
> > 
> > "with" appears to be missing from unscopables, failing the tests.
> 
> Oh, I did not add "with" to the unscopables because it was specified in the
> spec text: "The reason that "with" is not included in the unscopableList is
> because it is already a reserved word."

Oh, my bad, I didn't look beyond test file name and just made an assumption. "reserved word" explanation makes perfect sense.

I've added the absolutely last set of comments that should hopefully fix the JSC EWS.

Please re-upload the patch with --no-review --request-commit: after a r+, no re-review is needed unless the patch approach / idea was completely changed.

[Aditi Singh]

Created attachment 457317 [details]
Patch

[EWS]

Committed r292778 (249562@main): <https://commits.webkit.org/249562@main>

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