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
<rdar://problem/86984042>
Created attachment 449727 [details] Patch
Created attachment 449729 [details] Patch
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 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 {}.
Created attachment 450543 [details] Patch
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 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 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 on attachment 450543 [details] Patch Many failing tests. Need another try.
> 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.
(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.
(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`
(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.
Created attachment 456816 [details] Patch
(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 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.
(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. 👍
Created attachment 457159 [details] Patch
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?
(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.
Created attachment 457317 [details] Patch
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].