Bug 234604 - Implement Change Array by copy proposal
Summary: Implement Change Array by copy proposal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aditi Singh
URL: https://tc39.es/proposal-change-array...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-22 07:59 PST by Aditi Singh
Modified: 2022-04-12 10:56 PDT (History)
13 users (show)

See Also:


Attachments
Patch (25.74 KB, patch)
2022-01-22 02:00 PST, Aditi Singh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (25.80 KB, patch)
2022-01-22 02:20 PST, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (30.20 KB, patch)
2022-02-01 10:04 PST, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (31.48 KB, patch)
2022-04-06 06:47 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (31.49 KB, patch)
2022-04-09 02:31 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff
Patch (31.83 KB, patch)
2022-04-12 02:31 PDT, Aditi Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditi Singh 2021-12-22 07:59:49 PST
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
Comment 1 Radar WebKit Bug Importer 2021-12-29 08:00:21 PST
<rdar://problem/86984042>
Comment 2 Aditi Singh 2022-01-22 02:00:05 PST
Created attachment 449727 [details]
Patch
Comment 3 Aditi Singh 2022-01-22 02:20:54 PST
Created attachment 449729 [details]
Patch
Comment 4 Alexey Shvayka 2022-01-22 07:38:24 PST
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"?
Comment 5 Yusuke Suzuki 2022-01-22 16:27:30 PST
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 {}.
Comment 6 Aditi Singh 2022-02-01 10:04:09 PST
Created attachment 450543 [details]
Patch
Comment 7 Alexey Shvayka 2022-02-01 10:21:21 PST
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)`.
Comment 8 Yusuke Suzuki 2022-02-01 10:27:03 PST
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.
Comment 9 Joseph Pecoraro 2022-02-01 13:07:36 PST
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`.
Comment 10 Darin Adler 2022-02-03 04:02:44 PST
Comment on attachment 450543 [details]
Patch

Many failing tests. Need another try.
Comment 11 Aditi Singh 2022-02-07 10:34:34 PST
> 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.
Comment 12 Yusuke Suzuki 2022-02-07 10:40:12 PST
(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.
Comment 13 Aditi Singh 2022-02-16 10:21:00 PST
(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`
Comment 14 Yusuke Suzuki 2022-02-16 11:59:56 PST
(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.
Comment 15 Aditi Singh 2022-04-06 06:47:37 PDT
Created attachment 456816 [details]
Patch
Comment 16 Aditi Singh 2022-04-06 06:47:52 PDT
(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?
Comment 17 Alexey Shvayka 2022-04-06 10:58:38 PDT
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.
Comment 18 Aditi Singh 2022-04-07 01:53:45 PDT
(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.

👍
Comment 19 Aditi Singh 2022-04-09 02:31:02 PDT
Created attachment 457159 [details]
Patch
Comment 20 Alexey Shvayka 2022-04-11 05:37:53 PDT
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?
Comment 21 Alexey Shvayka 2022-04-11 05:46:04 PDT
(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.
Comment 22 Aditi Singh 2022-04-12 02:31:19 PDT
Created attachment 457317 [details]
Patch
Comment 23 EWS 2022-04-12 10:56:09 PDT
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].