Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763473002:
[typedarrays] Move %TypedArray%.prototype.slice to C++ (Closed)
|
Created:
3 years, 9 months ago by Choongwoo Han Modified:
3 years, 8 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[typedarrays] Move %TypedArray%.prototype.slice to C++
- Implement %TypedArray%.prototype.slice to C++ builtins
- Remove TypedArraySlice in src/js/typedarray.js
- Implement TypedArraySpeciesCreate in builtins-typedarray.cc
- Implement TypedArrayCreate in builtins-typedarray.cc
BUG=v8:5929
Review-Url: https://codereview.chromium.org/2763473002
Cr-Commit-Position: refs/heads/master@{#44322}
Committed: https://chromium.googlesource.com/v8/v8/+/c5c0765ad918d3606d7711d9dc5e727981ec8dcf
Patch Set 1 #Patch Set 2 : Add tests #
Total comments: 6
Patch Set 3 : Add tests, and move to elements accessor #Patch Set 4 : generic typedarraycreate #Patch Set 5 : type match #Patch Set 6 : debug check #
Total comments: 7
Patch Set 7 : NewNumberFromInt64 #Patch Set 8 : rebase #
Total comments: 6
Patch Set 9 : nits #
Total comments: 5
Patch Set 10 : minor #Patch Set 11 : rebase #
Messages
Total messages: 47 (27 generated)
Description was changed from ========== [typedarrays] Move %TypedArray%.prototype.slice to C++ - Implement %TypedArray%.prototype.slice to C++ builtins - Remove TypedArraySlice in src/js/typedarray.js - Implement TypedArraySpeciesCreate in builtins-typedarray.cc - Implement TypedArrayCreate in builtins-typedarray.cc BUG=v8:5929 ========== to ========== [typedarrays] Move %TypedArray%.prototype.slice to C++ - Implement %TypedArray%.prototype.slice to C++ builtins - Remove TypedArraySlice in src/js/typedarray.js - Implement TypedArraySpeciesCreate in builtins-typedarray.cc - Implement TypedArrayCreate in builtins-typedarray.cc BUG=v8:5929 ==========
cwhan.tunz@gmail.com changed reviewers: + bmeurer@chromium.org, caitp@igalia.com, cbruni@chromium.org
PTAL? I implemented all into builtins. I tried to put it into ElementsAccessor, but it seems that it conflicts with the other SliceImpl for Arrays. we can just change the current return type of Slice to Object instead of JSArray, but I'm not sure that it's the best option since it's still not general (e.g. Slice of typedarray should not be called in A.prototype.slice).
I'd prefer having this in elements.cc. Feel free to change the return type to JSObject
https://codereview.chromium.org/2763473002/diff/20001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2763473002/diff/20001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:331: FixedTypedArrayBase::cast(result_array->elements())); nit: No need for handles here. Add "DisallowHeapAllocation no_gc" https://codereview.chromium.org/2763473002/diff/20001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:344: result_accessor->Set(result_array, k, *elem); If you push this loop down into the elements accessor you only have to do one virtual call for each iteration step (See CopyElementsImpl). Not sure up to which extend this has to be optimized: - boxing floats/doubles to HeapNumbers is going to be very expensive - boxing 64bit integers to HeapNumbers on x86 is going to be very expensive https://codereview.chromium.org/2763473002/diff/20001/test/mjsunit/es6/typeda... File test/mjsunit/es6/typedarray-slice.js (right): https://codereview.chromium.org/2763473002/diff/20001/test/mjsunit/es6/typeda... test/mjsunit/es6/typedarray-slice.js:72: // Check that the result array is properly created by checking species Could you extend this and add specific tests with a subclass of each TypedArray? I'd like to have coverage when and if you add all the special casing in ElementsAccessor :).
On 2017/03/20 13:21:42, Camillo Bruni wrote: > I'd prefer having this in elements.cc. > Feel free to change the return type to JSObject then maybe i need to move TypedArrayCreate and TypedArraySpeciesCreate into ElementsAccessor, or move them into objects.cc. is this okey?
On 2017/03/20 at 13:47:30, cwhan.tunz wrote: > On 2017/03/20 13:21:42, Camillo Bruni wrote: > > I'd prefer having this in elements.cc. > > Feel free to change the return type to JSObject > > then maybe i need to move TypedArrayCreate and TypedArraySpeciesCreate into ElementsAccessor, or move them into objects.cc. is this okey? I would keep them outside of ElementsAccessor and keep them in builtins-typedarray.cc (so far noboday else is accessing them.) Otherwise the next-best place would be on Object, like the ArraySpeciesConstructor.
I moved all to elements accessor. I made an additional Slice method to get a result array as an argument to keep TypedArrayCreate in builtins. https://codereview.chromium.org/2763473002/diff/20001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2763473002/diff/20001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:331: FixedTypedArrayBase::cast(result_array->elements())); On 2017/03/20 13:37:25, Camillo Bruni wrote: > nit: No need for handles here. Add "DisallowHeapAllocation no_gc" Done. https://codereview.chromium.org/2763473002/diff/20001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:344: result_accessor->Set(result_array, k, *elem); On 2017/03/20 13:37:25, Camillo Bruni wrote: > If you push this loop down into the elements accessor you only have to do one > virtual call for each iteration step (See CopyElementsImpl). > Not sure up to which extend this has to be optimized: > - boxing floats/doubles to HeapNumbers is going to be very expensive > - boxing 64bit integers to HeapNumbers on x86 is going to be very expensive What is the meaning of one virtual call? also, I'm not sure that this case (different species type) is a very common case to optimize it. https://codereview.chromium.org/2763473002/diff/20001/test/mjsunit/es6/typeda... File test/mjsunit/es6/typedarray-slice.js (right): https://codereview.chromium.org/2763473002/diff/20001/test/mjsunit/es6/typeda... test/mjsunit/es6/typedarray-slice.js:72: // Check that the result array is properly created by checking species On 2017/03/20 13:37:25, Camillo Bruni wrote: > Could you extend this and add specific tests with a subclass of each TypedArray? > I'd like to have coverage when and if you add all the special casing in > ElementsAccessor :). Done.
Sorry for the late response... I had a slightly different approach in mind, see my comments. https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:45: MaybeHandle<JSTypedArray> TypedArrayCreate(Isolate* isolate, See my comments further down, I don't think we need the complete implementation (yet). NewJSTypedArray is probably good enough for now. https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:116: argv[0] = isolate->factory()->NewNumber(length); argv[0] = isolate->factory()->NewNumberFromInt64(length); https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:315: Handle<JSTypedArray> result_array; Push everything below here into the ElementsAccessor, so you don't need a separate entry point for TypedArrays. Also you can use NewJSTypedArray directly (https://cs.chromium.org/chromium/src/v8/src/factory.cc?type=cs&q=NewJSTy&l=2208). As a result the TypedArray implementation is going to be quite similar to the default Array one. (let me know if this is more or less clear?) Side node: Execute::New calls into JavaScript, which in this case is not necessary :).
https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:315: Handle<JSTypedArray> result_array; On 2017/03/22 22:08:25, Camillo Bruni wrote: > Push everything below here into the ElementsAccessor, so you don't need a > separate entry point for TypedArrays. > Also you can use NewJSTypedArray directly > (https://cs.chromium.org/chromium/src/v8/src/factory.cc?type=cs&q=NewJSTy&l=2208). > As a result the TypedArray implementation is going to be quite similar to the > default Array one. (let me know if this is more or less clear?) > > Side node: Execute::New calls into JavaScript, which in this case is not > necessary :). But, I guess we still need Execution::New call. species construction can be any javascript code that returns a typed array. So, if there is no species constructor, then we can use NewJSTypedArray for the case, but if not, we have to take the same step like this patch.
https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:45: MaybeHandle<JSTypedArray> TypedArrayCreate(Isolate* isolate, On 2017/03/22 at 22:08:25, Camillo Bruni wrote: > See my comments further down, I don't think we need the complete implementation (yet). > NewJSTypedArray is probably good enough for now. ignore this for now :) https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:315: Handle<JSTypedArray> result_array; On 2017/03/22 at 22:29:55, Choongwoo Han wrote: > On 2017/03/22 22:08:25, Camillo Bruni wrote: > > Push everything below here into the ElementsAccessor, so you don't need a > > separate entry point for TypedArrays. > > Also you can use NewJSTypedArray directly > > (https://cs.chromium.org/chromium/src/v8/src/factory.cc?type=cs&q=NewJSTy&l=2208). > > As a result the TypedArray implementation is going to be quite similar to the > > default Array one. (let me know if this is more or less clear?) > > > > Side node: Execute::New calls into JavaScript, which in this case is not > > necessary :). > > But, I guess we still need Execution::New call. species construction can be any javascript code that returns a typed array. So, if there is no species constructor, then we can use NewJSTypedArray for the case, but if not, we have to take the same step like this patch. You are right... it was too late yesterday night and I forgot about the species case. I think though we can use the SpeciesProtectorCell the same way as in the Array slice case (https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-array.cc?type=c...). This cell gets invalidated if anybody uses the @@species symbol or modifies the default constructor (see https://cs.chromium.org/chromium/src/v8/src/lookup.cc?type=cs&l=170), and as such we can avoid the rather expensive species lookup. Let's go with the current implementation first and land an optimization with a second CL.
The CQ bit was checked by cbruni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2763473002/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:116: argv[0] = isolate->factory()->NewNumber(length); On 2017/03/22 22:08:25, Camillo Bruni wrote: > argv[0] = isolate->factory()->NewNumberFromInt64(length); Done.
The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
cbruni@chromium.org changed reviewers: + petermarshall@chromium.org
Peter, can you have a look at the instantiation.
last nits. https://codereview.chromium.org/2763473002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2763473002/diff/140001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:62: DCHECK(argc > 1 || argv[0]->IsSmi()); DCHECK_IMPLIES(argc == 1, argv[0]->IsSmi()); https://codereview.chromium.org/2763473002/diff/140001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2763473002/diff/140001/src/elements.cc#newcod... src/elements.cc:722: return Subclass::SliceImpl(receiver, start, end); This should be UNREACHABLE(); since it is not possible to perform the raw slice operation of a non-typed array receiver and result (22.2.4.6 TypedArrayCreate prevents this). https://codereview.chromium.org/2763473002/diff/140001/src/elements.cc#newcod... src/elements.cc:3102: for (uint32_t k = start; k < end; k++) { nit: please use "i" as iteration index.
https://codereview.chromium.org/2763473002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2763473002/diff/140001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:62: DCHECK(argc > 1 || argv[0]->IsSmi()); On 2017/03/27 08:59:01, Camillo Bruni wrote: > DCHECK_IMPLIES(argc == 1, argv[0]->IsSmi()); Done. https://codereview.chromium.org/2763473002/diff/140001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2763473002/diff/140001/src/elements.cc#newcod... src/elements.cc:722: return Subclass::SliceImpl(receiver, start, end); On 2017/03/27 08:59:01, Camillo Bruni wrote: > This should be UNREACHABLE(); since it is not possible to perform the raw slice > operation of a non-typed array receiver and result (22.2.4.6 TypedArrayCreate > prevents this). Done. https://codereview.chromium.org/2763473002/diff/140001/src/elements.cc#newcod... src/elements.cc:3102: for (uint32_t k = start; k < end; k++) { On 2017/03/27 08:59:01, Camillo Bruni wrote: > nit: please use "i" as iteration index. Done.
LGTM, please wait for petermarshall and bmeurer.
The CQ bit was checked by cbruni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with comments, thanks https://codereview.chromium.org/2763473002/diff/160001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2763473002/diff/160001/src/elements.cc#newcod... src/elements.cc:3084: int64_t count = std::max<int64_t>(end - start, 0); We have already done DCHECK_LE(start, end) above, so won't end - start always be >= 0 here? https://codereview.chromium.org/2763473002/diff/160001/src/elements.cc#newcod... src/elements.cc:3101: Handle<BackingStore> from(BackingStore::cast(array->elements()), isolate); It is a bit strange that above you do BackingStore* src_elements = BackingStore::cast(receiver->elements()); to achieve essentially the same thing, except with a raw pointer instead of a handle. Could you use Handle<BackingStore>::cast(receiver->elements()) instead, to make it a bit more consistent?
https://codereview.chromium.org/2763473002/diff/160001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2763473002/diff/160001/src/elements.cc#newcod... src/elements.cc:3084: int64_t count = std::max<int64_t>(end - start, 0); On 2017/03/29 14:18:21, petermarshall wrote: > We have already done DCHECK_LE(start, end) above, so won't end - start always be > >= 0 here? Done. https://codereview.chromium.org/2763473002/diff/160001/src/elements.cc#newcod... src/elements.cc:3101: Handle<BackingStore> from(BackingStore::cast(array->elements()), isolate); On 2017/03/29 14:18:21, petermarshall wrote: > It is a bit strange that above you do > BackingStore* src_elements = BackingStore::cast(receiver->elements()); > > to achieve essentially the same thing, except with a raw pointer instead of a > handle. > Could you use Handle<BackingStore>::cast(receiver->elements()) instead, to make > it a bit more consistent? static const Handle<T> cast(Handle<S> that); Handle<T>::cast takes a Handle, so we need one more line to make array->elements() to a Handle for the expression.
Thanks! https://codereview.chromium.org/2763473002/diff/160001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2763473002/diff/160001/src/elements.cc#newcod... src/elements.cc:3101: Handle<BackingStore> from(BackingStore::cast(array->elements()), isolate); On 2017/03/29 at 14:42:02, Choongwoo Han wrote: > On 2017/03/29 14:18:21, petermarshall wrote: > > It is a bit strange that above you do > > BackingStore* src_elements = BackingStore::cast(receiver->elements()); > > > > to achieve essentially the same thing, except with a raw pointer instead of a > > handle. > > Could you use Handle<BackingStore>::cast(receiver->elements()) instead, to make > > it a bit more consistent? > > static const Handle<T> cast(Handle<S> that); > Handle<T>::cast takes a Handle, so we need one more line to make array->elements() to a Handle for the expression. Ahh ok, thanks.
LGTM (rubber-stamped) Note: You don't need to wait for my LGTM on these changes when you already have an LGTM from cbruni@.
The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/20207) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/38224)
The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cwhan.tunz@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from petermarshall@chromium.org, cbruni@chromium.org, bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2763473002/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491065082803080,
"parent_rev": "f22979bca094cbb3e75e131b459c2171b5acc834", "commit_rev":
"c5c0765ad918d3606d7711d9dc5e727981ec8dcf"}
Message was sent while issue was closed.
Description was changed from ========== [typedarrays] Move %TypedArray%.prototype.slice to C++ - Implement %TypedArray%.prototype.slice to C++ builtins - Remove TypedArraySlice in src/js/typedarray.js - Implement TypedArraySpeciesCreate in builtins-typedarray.cc - Implement TypedArrayCreate in builtins-typedarray.cc BUG=v8:5929 ========== to ========== [typedarrays] Move %TypedArray%.prototype.slice to C++ - Implement %TypedArray%.prototype.slice to C++ builtins - Remove TypedArraySlice in src/js/typedarray.js - Implement TypedArraySpeciesCreate in builtins-typedarray.cc - Implement TypedArrayCreate in builtins-typedarray.cc BUG=v8:5929 Review-Url: https://codereview.chromium.org/2763473002 Cr-Commit-Position: refs/heads/master@{#44322} Committed: https://chromium.googlesource.com/v8/v8/+/c5c0765ad918d3606d7711d9dc5e727981e... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/v8/v8/+/c5c0765ad918d3606d7711d9dc5e727981e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
