Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
771483002:
Implement ES6 @@isConcatSpreadable (Closed)
|
Created:
6 years ago by caitp (gmail) Modified:
6 years ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Project:
v8 Visibility:
Public. |
DescriptionImplement ES6 @@isConcatSpreadable / Array.prototype.concat
Add support for Symbol.isConcatSpreadable in Array.prototype.concat. This enables spreading non-Array objects with the symbol.
LOG=N
R=dslomov@chromium.org
BUG=
Patch Set 1 #Patch Set 2 : Move implementation to C++ to fix speed issue #
Total comments: 7
Patch Set 3 : Uncomment test cases (should have been done in patch set #2) #Patch Set 4 : Rebase #
Total comments: 2
Patch Set 5 : Add TypedArray tests to ensure they are not spread #Patch Set 6 : Set @@isConcatSpreadable on TypedArrays in tests #
Total comments: 4
Patch Set 7 : Add tests for sloppy arguments + isConcatSpreadable #
Total comments: 4
Patch Set 8 : Add SLOPPY_ARGUMENTS_ELEMENTS handler to IterateElements, additional tests #Patch Set 9 : Fix holey sloppy-arguments case #
Total comments: 1
Patch Set 10 : Remove default case from IterateElements(), add missing Fixed<Type>Array cases #
Total comments: 4
Patch Set 11 : Rename IterateExternalArrayElements, add tests which hit Fixed<Type>Array cases #
Messages
Total messages: 25 (2 generated)
As mentioned in the CL description, I believe the draft's behaviour results in a quite observable difference in behaviour from the current implementations in browsers. I haven't done any research to verify if that difference could break applications or not, but it might be good to answer those questions here. Also as noted, this does not modify the runtime concat routine which appears to be carefully optimized for previously spec'd behaviour. There are still some issues with the subclassed-array behaviour that is mandated. Maybe a good place to figure that out
On 2014/11/30 20:32:08, caitp wrote: > As mentioned in the CL description, I believe the draft's behaviour results in a > quite observable difference in behaviour from the current implementations in > browsers. > > I haven't done any research to verify if that difference could break > applications or not, but it might be good to answer those questions here. > > Also as noted, this does not modify the runtime concat routine which appears to > be carefully optimized for previously spec'd behaviour. > > There are still some issues with the subclassed-array behaviour that is > mandated. Maybe a good place to figure that out Frankly, not sure we want to lead the way for other implementation in this particular case :) Risks seem great for not much benefit. Maybe we should go back to TC39 with this.
On 2014/11/30 21:32:54, Dmitry Lomov (chromium) wrote: > On 2014/11/30 20:32:08, caitp wrote: > > As mentioned in the CL description, I believe the draft's behaviour results in > a > > quite observable difference in behaviour from the current implementations in > > browsers. > > > > I haven't done any research to verify if that difference could break > > applications or not, but it might be good to answer those questions here. > > > > Also as noted, this does not modify the runtime concat routine which appears > to > > be carefully optimized for previously spec'd behaviour. > > > > There are still some issues with the subclassed-array behaviour that is > > mandated. Maybe a good place to figure that out > > Frankly, not sure we want to lead the way for other implementation in this > particular case :) > Risks seem great for not much benefit. Maybe we should go back to TC39 with > this. I was talking about it in #jslang, it looks like this behaviour has actually not changed from ES5 (ES262 5.1 15.4.4.4 step 5.b.iii.3) --- however yes, JSC, SM and V8 are all violating this, it's not clear whether it's fixable or not.
On 2014/11/30 21:41:56, caitp wrote: > On 2014/11/30 21:32:54, Dmitry Lomov (chromium) wrote: > > On 2014/11/30 20:32:08, caitp wrote: > > > As mentioned in the CL description, I believe the draft's behaviour results > in > > a > > > quite observable difference in behaviour from the current implementations in > > > browsers. > > > > > > I haven't done any research to verify if that difference could break > > > applications or not, but it might be good to answer those questions here. > > > > > > Also as noted, this does not modify the runtime concat routine which appears > > to > > > be carefully optimized for previously spec'd behaviour. > > > > > > There are still some issues with the subclassed-array behaviour that is > > > mandated. Maybe a good place to figure that out > > > > Frankly, not sure we want to lead the way for other implementation in this > > particular case :) > > Risks seem great for not much benefit. Maybe we should go back to TC39 with > > this. > > I was talking about it in #jslang, it looks like this behaviour has actually not > changed from ES5 (ES262 5.1 15.4.4.4 step 5.b.iii.3) --- however yes, JSC, SM > and V8 are all violating this, it's not clear whether it's fixable or not. So, I am just reading the spec wrong, jorendorff corrected me here. So there's no wildly web-breaking behaviour.
I've re-implemented a subset of this CL in C++, as the JS implementation was unfortunately too slow and caused test cases with long arrays to hang. The FixedArray backing store for elements seems to make this work much better. This is not quite spec correct, but it does enable spreading of array-like objects. Things remaining to do: - Always treat subclasses of Array as ConcatSpreadable - Return a subclass of Array if `this` is a subclass of Array from the same realm, rather than always an Array. - Improve calculation of `length` for array-like spreadable properties
caitpotter88@gmail.com changed reviewers: + arv@chromium.org, dslomov@chromium.org, rossberg@chromium.org
You don't have to worry about Array subclassing just yet - it does not work in V8 anyway. https://codereview.chromium.org/771483002/diff/20001/src/runtime/runtime-arra... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/20001/src/runtime/runtime-arra... src/runtime/runtime-array.cc:463: Runtime::GetObjectProperty(isolate, receiver, key), false); Here you use whatever length the incoming object gives you, so it does not have "the amazing Array length behavior". This means that the assumptions in remainder of this method do not hold, in particular DCHECKs in lines 477 and 511. https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/arr... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-concat.js:10: assertEquals(1, Array.prototype.concat.length); Add a test that validates that concat works correctly on on objects in fast mode, including Smis and doubles https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-concat.js:185: /* Stray comments
https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/arr... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-concat.js:10: assertEquals(1, Array.prototype.concat.length); On 2014/12/10 10:58:21, Dmitry Lomov (chromium) wrote: > Add a test that validates that concat works correctly on on objects in fast > mode, including Smis and doubles I'm not sure how to create a non-array object which would take the FAST_SMI_ELEMENTS or FAST_DOUBLE_ELEMENTS paths --- I have only managed to get it taking the FAST_ELEMENTS and FAST_HOLEY_ELEMENTS paths, and never FAST_DOUBLE_ELEMENTS. https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-concat.js:185: /* On 2014/12/10 10:58:20, Dmitry Lomov (chromium) wrote: > Stray comments Yeah, I had these commented out because they caused hangs when implemented in JS --- these were removed in patch set 3
Sorry for raising false alarms on FAST_SMI_ELEMENTS and friends, these are only applicable for JSArrays https://codereview.chromium.org/771483002/diff/20001/src/runtime/runtime-arra... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/20001/src/runtime/runtime-arra... src/runtime/runtime-array.cc:463: Runtime::GetObjectProperty(isolate, receiver, key), false); On 2014/12/10 10:58:20, Dmitry Lomov (chromium) wrote: > Here you use whatever length the incoming object gives you, so it does not have > "the amazing Array length behavior". This means that the assumptions in > remainder of this method do not hold, in particular DCHECKs in lines 477 and > 511. Ah ok sorry I jumped the gun here, FAST_(SMI|DOUBLE)_(HOLEY_)ELEMENTS are indeed JSArray only https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/arr... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-concat.js:185: /* On 2014/12/10 13:25:33, caitp wrote: > On 2014/12/10 10:58:20, Dmitry Lomov (chromium) wrote: > > Stray comments > > Yeah, I had these commented out because they caused hangs when implemented in JS > --- these were removed in patch set 3 Oops sorry. https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/arr... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-concat.js:9: (function testArrayConcatArity() { Please add a test that sets isConcatSpreadable on a typed array (small and large)
https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/arr... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-concat.js:9: (function testArrayConcatArity() { On 2014/12/10 14:41:59, Dmitry Lomov (chromium) wrote: > Please add a test that sets isConcatSpreadable on a typed array (small and > large) Acknowledged --- but reading the draft, I am not seeing TypedArrays/ArrayBuffers/DataViews as concat-spreadable, unless I'm missing something. So I think these tests are just asserting that the typed arrays are NOT spread, is that right?
On 2014/12/10 15:29:20, caitp wrote: > https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/arr... > File test/mjsunit/harmony/array-concat.js (right): > > https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/arr... > test/mjsunit/harmony/array-concat.js:9: (function testArrayConcatArity() { > On 2014/12/10 14:41:59, Dmitry Lomov (chromium) wrote: > > Please add a test that sets isConcatSpreadable on a typed array (small and > > large) > > Acknowledged --- but reading the draft, I am not seeing > TypedArrays/ArrayBuffers/DataViews as concat-spreadable, unless I'm missing > something. So I think these tests are just asserting that the typed arrays are > NOT spread, is that right? I meant, just set isConcatSpreadable manually on an instance of typed array and pass it to concat. The goal of the test is to exercise IterateElements (that you have changed to accept more than just JSArray) on all possible ElementKinds.
On 2014/12/10 16:02:38, Dmitry Lomov (chromium) wrote: > On 2014/12/10 15:29:20, caitp wrote: > > > https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/arr... > > File test/mjsunit/harmony/array-concat.js (right): > > > > > https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/arr... > > test/mjsunit/harmony/array-concat.js:9: (function testArrayConcatArity() { > > On 2014/12/10 14:41:59, Dmitry Lomov (chromium) wrote: > > > Please add a test that sets isConcatSpreadable on a typed array (small and > > > large) > > > > Acknowledged --- but reading the draft, I am not seeing > > TypedArrays/ArrayBuffers/DataViews as concat-spreadable, unless I'm missing > > something. So I think these tests are just asserting that the typed arrays are > > NOT spread, is that right? > > I meant, just set isConcatSpreadable manually on an instance of typed array and > pass it to concat. > The goal of the test is to exercise IterateElements (that you have changed to > accept more than just JSArray) on all possible ElementKinds. Oh I see! Of course
lgtm % nit https://codereview.chromium.org/771483002/diff/100001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/100001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-concat.js:129: testConcatTypedArray(ctor, 150, max[i]); Make this smaller, like 2. Default value of FLAG_typed_array_max_size_in_heap is 64 bytes.
We forgot once case. Marking "not lgtm" to prevent accidental landing. https://codereview.chromium.org/771483002/diff/100001/src/runtime/runtime-arr... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/100001/src/runtime/runtime-arr... src/runtime/runtime-array.cc:467: Oops, I forgot one more case - please remove default clause in the switch, line 609, and make sure to handle SLOPPY_ARGUMENTS_ELEMENTS https://codereview.chromium.org/771483002/diff/100001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/100001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-concat.js:5: // Flags: --harmony-arrays --harmony-classes Add a test for sloppy arguments (install isConcatSpreadable on them and pass to concat)
https://codereview.chromium.org/771483002/diff/100001/src/runtime/runtime-arr... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/100001/src/runtime/runtime-arr... src/runtime/runtime-array.cc:467: On 2014/12/10 23:02:47, Dmitry Lomov (chromium) wrote: > Oops, I forgot one more case - please remove default clause in the switch, line > 609, and make sure to handle SLOPPY_ARGUMENTS_ELEMENTS I've removed the top-level "use strict" directive and added two tests for this with strict and sloppy arguments, and both tests pass with no changes. I'll upload it, I'm not sure anything special really needs to be done for this, but I'm probably missing something.
One does not simple create a SLOPPY_ARGUMENTS_ELEMENT_KIND :)) (this is tricky stuff, and we overlook this corner case all the time) https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-concat.js:164: var args = (function() { return arguments; })(1,2,3); Yes, this test does not produce SLOPPY_ARGUMENTS_ELEMENT_KIND. You need a function to have both arguments and parameters to get it. So just replace `function f()` with `function f(x, y, z)` The logic that governs this is here: https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/runtime/run...
https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-concat.js:164: var args = (function() { return arguments; })(1,2,3); On 2014/12/11 11:05:47, Dmitry Lomov (chromium) wrote: > Yes, this test does not produce SLOPPY_ARGUMENTS_ELEMENT_KIND. > You need a function to have both arguments and parameters to get it. > So just replace `function f()` with `function f(x, y, z)` > The logic that governs this is here: > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/runtime/run... Okay --- I've added a handler for SLOPPY_ARGUMENTS_ELEMENTS, but did not remove the default case --- It's a simple thing to remove it, but I think it's good to have missing cases stand out more than just not adding the elements. Also added some tests with different variations of the sloppy arguments object
Some comments. Also I realized a bigger issue with this CL - it never goes up the prototype chain for elements, even though the spec prescribes that. This can be addressed in a separate CL. https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-concat.js:164: var args = (function() { return arguments; })(1,2,3); On 2014/12/11 13:09:35, caitp wrote: > On 2014/12/11 11:05:47, Dmitry Lomov (chromium) wrote: > > Yes, this test does not produce SLOPPY_ARGUMENTS_ELEMENT_KIND. > > You need a function to have both arguments and parameters to get it. > > So just replace `function f()` with `function f(x, y, z)` > > The logic that governs this is here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/runtime/run... > > Okay --- I've added a handler for SLOPPY_ARGUMENTS_ELEMENTS, but did not remove > the default case --- It's a simple thing to remove it, but I think it's good to > have missing cases stand out more than just not adding the elements. No, please remove the default case, so that compiler complains if we add a new ElementsKind and it goes unhandled here. > > Also added some tests with different variations of the sloppy arguments object https://codereview.chromium.org/771483002/diff/150001/src/runtime/runtime-arr... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/150001/src/runtime/runtime-arr... src/runtime/runtime-array.cc:615: if (!element->IsTheHole()) { Use accessor->HasElement(receiver, receiver, index) instead of checking for hole.
https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-concat.js:164: var args = (function() { return arguments; })(1,2,3); On 2014/12/11 14:01:50, Dmitry Lomov (chromium) wrote: > On 2014/12/11 13:09:35, caitp wrote: > > On 2014/12/11 11:05:47, Dmitry Lomov (chromium) wrote: > > > Yes, this test does not produce SLOPPY_ARGUMENTS_ELEMENT_KIND. > > > You need a function to have both arguments and parameters to get it. > > > So just replace `function f()` with `function f(x, y, z)` > > > The logic that governs this is here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/runtime/run... > > > > Okay --- I've added a handler for SLOPPY_ARGUMENTS_ELEMENTS, but did not > remove > > the default case --- It's a simple thing to remove it, but I think it's good > to > > have missing cases stand out more than just not adding the elements. > > No, please remove the default case, so that compiler complains if we add a new > ElementsKind and it goes unhandled here. > > > > > Also added some tests with different variations of the sloppy arguments object > Done --- There were a bunch of missing cases, I'm not sure exactly how to test all of those though. The TypedArray tests don't hit them, as far as I can see
https://codereview.chromium.org/771483002/diff/170001/src/runtime/runtime-arr... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/170001/src/runtime/runtime-arr... src/runtime/runtime-array.cc:583: IterateExternalArrayElements<FixedInt8Array, int8_t>( Please rename IterateExternalArrayElements into IterateTypedArrayElements (this no longer only iterates externals) https://codereview.chromium.org/771483002/diff/170001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/170001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-concat.js:122: var ta = new type(items); The reason why you are not hitting UINT8_ELEMENTS and friends, only EXTERNAL_UINT8_ELEMENTS and friends, is because only new TypedArray(length) call allocates in-heap typed arrays. Remove `items` and initialize `ta` directly - then you should hit the cases you have added.
https://codereview.chromium.org/771483002/diff/170001/src/runtime/runtime-arr... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/170001/src/runtime/runtime-arr... src/runtime/runtime-array.cc:583: IterateExternalArrayElements<FixedInt8Array, int8_t>( On 2014/12/11 15:11:29, Dmitry Lomov (chromium) wrote: > Please rename IterateExternalArrayElements into IterateTypedArrayElements (this > no longer only iterates externals) Done. https://codereview.chromium.org/771483002/diff/170001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/170001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-concat.js:122: var ta = new type(items); On 2014/12/11 15:11:29, Dmitry Lomov (chromium) wrote: > The reason why you are not hitting UINT8_ELEMENTS and friends, only > EXTERNAL_UINT8_ELEMENTS and friends, is because only new TypedArray(length) call > allocates in-heap typed arrays. > > Remove `items` and initialize `ta` directly - then you should hit the cases you > have added. In order to use Fixed<Type>Array, the length needs to be less than FLAG_typed_array_max_size_in_heap with a null JSArrayBuffer, far as I can tell --- so I think I'm only hitting this from the "small" array case, but it works
lgtm
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/771483002/190001
Message was sent while issue was closed.
Committed patchset #11 (id:190001) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
