Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697593002:
Reland [typedarrays] move %TypedArray%.prototype.copyWithin to C++ (Closed)
DescriptionReland [typedarrays] move %TypedArray%.prototype.copyWithin to C++
- Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js
- Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which
relies on std::memmove rather than accessing individual eleements.
- Fixes the case where copyWithin is invoked on a TypedArray with a
detached buffer.
- Add tests to ensure that +/-Infinity (for all 3 parameters) is handled
correctly by the
algorithm
The C++ version gets through the benchmark more than 25000 times as
quickly as the JS implementation.
BUG=v8:5925, v8:5929, v8:4648
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org
Review-Url: https://codereview.chromium.org/2697593002
Cr-Commit-Position: refs/heads/master@{#43213}
Committed: https://chromium.googlesource.com/v8/v8/+/dc302c74be4183db1b84347afab893825c887add
Patch Set 1 #Patch Set 2 : with a fix #Patch Set 3 : make compiler happy #
Total comments: 1
Patch Set 4 : version that throws instead of early return + fixes a typo #Patch Set 5 : Back to an early return to match web reality #
Total comments: 6
Patch Set 6 : don't throw if detached in ValidateTypedArray() #Patch Set 7 : don't change test262.status since it still doesn't throw #
Total comments: 8
Patch Set 8 : point to the bug where requested, + extra assertion... #
Total comments: 2
Patch Set 9 : last of it #
Total comments: 5
Patch Set 10 : Add bounds-checking DCHECKs #
Total comments: 3
Patch Set 11 : bigger fatter DCHECKs #
Total comments: 2
Patch Set 12 : update comment #
Messages
Total messages: 77 (44 generated)
The CQ bit was checked by caitp@igalia.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...
PTAL, here's a fix for the copyWithin() form of this bug. Another one for A.p.includes will be up shortly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/17325)
The CQ bit was checked by caitp@igalia.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...
https://codereview.chromium.org/2697593002/diff/40001/test/mjsunit/es6/regres... File test/mjsunit/es6/regress/regress-5929-1.js (right): https://codereview.chromium.org/2697593002/diff/40001/test/mjsunit/es6/regres... test/mjsunit/es6/regress/regress-5929-1.js:14: arr.copyWithin(tmp); So, the question is if this should throw an exception or not. I don't see a real issue with not throwing, but I suppose you could argue that it needs to act as though it actually did [[Get]] each element.
The CQ bit was checked by caitp@igalia.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.
bmeurer@chromium.org changed reviewers: + bmeurer@chromium.org
bmeurer@chromium.org changed required reviewers: + cbruni@chromium.org
LGTM from my side
Code seems fine, but semantically, could we start throwing on detached TypedArrays in a more organized way (both for the ValidateTypedArray at the beginning, and in the detached-due-to-ToInteger edge case)? It seems a bit strange to just throw for detached here, but not for the other TypedArray methods.
On 2017/02/13 17:49:04, Dan Ehrenberg wrote: > Code seems fine, but semantically, could we start throwing on detached > TypedArrays in a more organized way (both for the ValidateTypedArray at the > beginning, and in the detached-due-to-ToInteger edge case)? It seems a bit > strange to just throw for detached here, but not for the other TypedArray > methods. The behavior if we wanted to preserve "V8 legacy semantics" would be straightforward--just return without doing anything if we find that the buffer is detached (for both checks).
On 2017/02/13 17:49:47, Dan Ehrenberg wrote: > On 2017/02/13 17:49:04, Dan Ehrenberg wrote: > > Code seems fine, but semantically, could we start throwing on detached > > TypedArrays in a more organized way (both for the ValidateTypedArray at the > > beginning, and in the detached-due-to-ToInteger edge case)? It seems a bit > > strange to just throw for detached here, but not for the other TypedArray > > methods. > > The behavior if we wanted to preserve "V8 legacy semantics" would be > straightforward--just return without doing anything if we find that the buffer > is detached (for both checks). The legacy semantic is to throw, since it would have called the element getter previously, which would throw after detach
On 2017/02/13 17:57:15, caitp wrote: > On 2017/02/13 17:49:47, Dan Ehrenberg wrote: > > On 2017/02/13 17:49:04, Dan Ehrenberg wrote: > > > Code seems fine, but semantically, could we start throwing on detached > > > TypedArrays in a more organized way (both for the ValidateTypedArray at the > > > beginning, and in the detached-due-to-ToInteger edge case)? It seems a bit > > > strange to just throw for detached here, but not for the other TypedArray > > > methods. > > > > The behavior if we wanted to preserve "V8 legacy semantics" would be > > straightforward--just return without doing anything if we find that the buffer > > is detached (for both checks). > > The legacy semantic is to throw, since it would have called the element getter > previously, which would throw after detach Why would that happen? Element accesses would return undefined. I tested this on ToT, and copyWithin doesn't throw when applied to a detached TypedArray. I'm not opposed to changing the methods to throw; I just think that if there are any web-compat issues, it's better if we get it all in one go than force users to deal with a stream of small changes over multiple Chrome versions to TypedArray semantics. That way, they can update their code once and be done with the issue.
On 2017/02/13 18:21:51, Dan Ehrenberg wrote: > On 2017/02/13 17:57:15, caitp wrote: > > On 2017/02/13 17:49:47, Dan Ehrenberg wrote: > > > On 2017/02/13 17:49:04, Dan Ehrenberg wrote: > > > > Code seems fine, but semantically, could we start throwing on detached > > > > TypedArrays in a more organized way (both for the ValidateTypedArray at > the > > > > beginning, and in the detached-due-to-ToInteger edge case)? It seems a bit > > > > strange to just throw for detached here, but not for the other TypedArray > > > > methods. > > > > > > The behavior if we wanted to preserve "V8 legacy semantics" would be > > > straightforward--just return without doing anything if we find that the > buffer > > > is detached (for both checks). > > > > The legacy semantic is to throw, since it would have called the element getter > > previously, which would throw after detach > > Why would that happen? Element accesses would return undefined. I tested this on > ToT, and copyWithin doesn't throw when applied to a detached TypedArray. > > I'm not opposed to changing the methods to throw; I just think that if there are > any web-compat issues, it's better if we get it all in one go than force users > to deal with a stream of small changes over multiple Chrome versions to > TypedArray semantics. That way, they can update their code once and be done with > the issue. Doesn't the set throw? Or does that silently fail?
On 2017/02/13 18:24:07, caitp wrote:
> On 2017/02/13 18:21:51, Dan Ehrenberg wrote:
> > On 2017/02/13 17:57:15, caitp wrote:
> > > On 2017/02/13 17:49:47, Dan Ehrenberg wrote:
> > > > On 2017/02/13 17:49:04, Dan Ehrenberg wrote:
> > > > > Code seems fine, but semantically, could we start throwing on detached
> > > > > TypedArrays in a more organized way (both for the ValidateTypedArray
at
> > the
> > > > > beginning, and in the detached-due-to-ToInteger edge case)? It seems a
> bit
> > > > > strange to just throw for detached here, but not for the other
> TypedArray
> > > > > methods.
> > > >
> > > > The behavior if we wanted to preserve "V8 legacy semantics" would be
> > > > straightforward--just return without doing anything if we find that the
> > buffer
> > > > is detached (for both checks).
> > >
> > > The legacy semantic is to throw, since it would have called the element
> getter
> > > previously, which would throw after detach
> >
> > Why would that happen? Element accesses would return undefined. I tested
this
> on
> > ToT, and copyWithin doesn't throw when applied to a detached TypedArray.
> >
> > I'm not opposed to changing the methods to throw; I just think that if there
> are
> > any web-compat issues, it's better if we get it all in one go than force
users
> > to deal with a stream of small changes over multiple Chrome versions to
> > TypedArray semantics. That way, they can update their code once and be done
> with
> > the issue.
>
> Doesn't the set throw? Or does that silently fail?
It silently does nothing, which you could call failing. Here's a d8 session to
illustrate the point:
d8> let x = new Uint8Array(5)
undefined
d8> x[0]
0
d8> x[0] = 1
1
d8> x[0]
1
d8> %ArrayBufferNeuter(x.buffer)
undefined
d8> x
{}
d8> x[0]
undefined
d8> x[0] = 5
5
d8> x[0]
undefined
On 2017/02/13 18:35:18, Dan Ehrenberg wrote:
> On 2017/02/13 18:24:07, caitp wrote:
> > On 2017/02/13 18:21:51, Dan Ehrenberg wrote:
> > > On 2017/02/13 17:57:15, caitp wrote:
> > > > On 2017/02/13 17:49:47, Dan Ehrenberg wrote:
> > > > > On 2017/02/13 17:49:04, Dan Ehrenberg wrote:
> > > > > > Code seems fine, but semantically, could we start throwing on
detached
> > > > > > TypedArrays in a more organized way (both for the ValidateTypedArray
> at
> > > the
> > > > > > beginning, and in the detached-due-to-ToInteger edge case)? It seems
a
> > bit
> > > > > > strange to just throw for detached here, but not for the other
> > TypedArray
> > > > > > methods.
> > > > >
> > > > > The behavior if we wanted to preserve "V8 legacy semantics" would be
> > > > > straightforward--just return without doing anything if we find that
the
> > > buffer
> > > > > is detached (for both checks).
> > > >
> > > > The legacy semantic is to throw, since it would have called the element
> > getter
> > > > previously, which would throw after detach
> > >
> > > Why would that happen? Element accesses would return undefined. I tested
> this
> > on
> > > ToT, and copyWithin doesn't throw when applied to a detached TypedArray.
> > >
> > > I'm not opposed to changing the methods to throw; I just think that if
there
> > are
> > > any web-compat issues, it's better if we get it all in one go than force
> users
> > > to deal with a stream of small changes over multiple Chrome versions to
> > > TypedArray semantics. That way, they can update their code once and be
done
> > with
> > > the issue.
> >
> > Doesn't the set throw? Or does that silently fail?
>
> It silently does nothing, which you could call failing. Here's a d8 session to
> illustrate the point:
>
> d8> let x = new Uint8Array(5)
> undefined
> d8> x[0]
> 0
> d8> x[0] = 1
> 1
> d8> x[0]
> 1
> d8> %ArrayBufferNeuter(x.buffer)
> undefined
> d8> x
> {}
> d8> x[0]
> undefined
> d8> x[0] = 5
> 5
> d8> x[0]
> undefined
Ok, early return sounds good then, still fix up both of these in a sec
The CQ bit was checked by caitp@igalia.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.
https://codereview.chromium.org/2697593002/diff/80001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/80001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:185: THROW_NEW_ERROR(isolate, NewTypeError(message, operation), JSTypedArray); This is also a change vs current behavior. https://codereview.chromium.org/2697593002/diff/80001/test/mjsunit/es6/regres... File test/mjsunit/es6/regress/regress-5929-1.js (right): https://codereview.chromium.org/2697593002/diff/80001/test/mjsunit/es6/regres... test/mjsunit/es6/regress/regress-5929-1.js:14: arr.copyWithin(tmp); Could you test that this leaves the TypedArray in a detached-looking state? Also, apparently we're missing a test that copyWithin on a detached TypedArray does nothing; it'd be good to add it (this is actually my fault, but you're rewriting code which hits that path, so it'd be good to have a test to confirm).
On 2017/02/13 19:56:44, Dan Ehrenberg wrote: > https://codereview.chromium.org/2697593002/diff/80001/src/builtins/builtins-t... > File src/builtins/builtins-typedarray.cc (right): > > https://codereview.chromium.org/2697593002/diff/80001/src/builtins/builtins-t... > src/builtins/builtins-typedarray.cc:185: THROW_NEW_ERROR(isolate, > NewTypeError(message, operation), JSTypedArray); > This is also a change vs current behavior. > > https://codereview.chromium.org/2697593002/diff/80001/test/mjsunit/es6/regres... > File test/mjsunit/es6/regress/regress-5929-1.js (right): > > https://codereview.chromium.org/2697593002/diff/80001/test/mjsunit/es6/regres... > test/mjsunit/es6/regress/regress-5929-1.js:14: arr.copyWithin(tmp); > Could you test that this leaves the TypedArray in a detached-looking state? > > Also, apparently we're missing a test that copyWithin on a detached TypedArray > does nothing; it'd be good to add it (this is actually my fault, but you're > rewriting code which hits that path, so it'd be good to have a test to confirm). and by "does nothing", I assume you mean "doesn't process any parameters"?
https://codereview.chromium.org/2697593002/diff/80001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/80001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:185: THROW_NEW_ERROR(isolate, NewTypeError(message, operation), JSTypedArray); On 2017/02/13 at 19:56:44, Dan Ehrenberg wrote: > This is also a change vs current behavior. I fear that we have quite a few other places where we don't throw according to the spec yet. https://codereview.chromium.org/2697593002/diff/80001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:249: if (V8_UNLIKELY(array->WasNeutered())) return *array; I think you have to throw here according to the spec (see comment on the test). https://codereview.chromium.org/2697593002/diff/80001/test/mjsunit/es6/regres... File test/mjsunit/es6/regress/regress-5929-1.js (right): https://codereview.chromium.org/2697593002/diff/80001/test/mjsunit/es6/regres... test/mjsunit/es6/regress/regress-5929-1.js:14: arr.copyWithin(tmp); On 2017/02/13 at 19:56:44, Dan Ehrenberg wrote: > Could you test that this leaves the TypedArray in a detached-looking state? > > Also, apparently we're missing a test that copyWithin on a detached TypedArray does nothing; it'd be good to add it (this is actually my fault, but you're rewriting code which hits that path, so it'd be good to have a test to confirm). This should throw as you will start iterating from 50 to 0x10000, since you call [[HasProperty]] (22.1.3.3 12.c) which will trow if detached (see 9.4.5.2 3.ii).
https://codereview.chromium.org/2697593002/diff/80001/test/mjsunit/es6/regres... File test/mjsunit/es6/regress/regress-5929-1.js (right): https://codereview.chromium.org/2697593002/diff/80001/test/mjsunit/es6/regres... test/mjsunit/es6/regress/regress-5929-1.js:14: arr.copyWithin(tmp); On 2017/02/13 20:18:56, Camillo Bruni wrote: > On 2017/02/13 at 19:56:44, Dan Ehrenberg wrote: > > Could you test that this leaves the TypedArray in a detached-looking state? > > > > Also, apparently we're missing a test that copyWithin on a detached TypedArray > does nothing; it'd be good to add it (this is actually my fault, but you're > rewriting code which hits that path, so it'd be good to have a test to confirm). > > This should throw as you will start iterating from 50 to 0x10000, since you call > [[HasProperty]] (22.1.3.3 12.c) which will trow if detached (see 9.4.5.2 3.ii). It should, but it's been suggested that we make all the TypedArray operations throw at the same time, to prevent giving TC39 an excuse to make a very complicated set of rules about when to throw and when not to. So, coming in the next patch set, both the detached-before-call and detached-during-ToInteger cases will just cause an early exit
Sorry, I didn't follow the complete conversation :). Given that I implemented throwing in another security case (see https://chromium-review.googlesource.com/c/441964/) we might as well start doing it here according to the spec? (or is that still debatable whether we throw on detached state or not?)
ok, SGTM, will update my CL then accordingly.
The CQ bit was checked by caitp@igalia.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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20939) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/17382) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
The CQ bit was checked by caitp@igalia.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.
Ok, Adam does this work for you? I think Dan is happy with it
Only looked at the bits regarding not-throwing. https://codereview.chromium.org/2697593002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/120001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:180: // TODO(caitp): throw if array.[[ViewedArrayBuffer]] is neutered. Rather than making this a TODO, can you point at the bug about how v8 doesn't throw as often as the spec requires? That'll make it easier for folks reading this in the future to understand why we don't throw here. https://codereview.chromium.org/2697593002/diff/120001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:212: // TODO(caitp): throw if array.[[ViewedArrayBuffer]] is neutered. Same here. https://codereview.chromium.org/2697593002/diff/120001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-copywithin.js (right): https://codereview.chromium.org/2697593002/diff/120001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-copywithin.js:243: // TODO(caitp): this should throw due to being invoked on a TypedArray with a Same thing here as the other TODOs, I'd prefer that this point at a bug. https://codereview.chromium.org/2697593002/diff/120001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-copywithin.js:245: array.copyWithin(tmp, tmp, tmp); Is there an assertion you can make after the copyWithin call? Maybe just assertEquals(0, arr.length)?
The CQ bit was checked by caitp@igalia.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...
https://codereview.chromium.org/2697593002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/120001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:180: // TODO(caitp): throw if array.[[ViewedArrayBuffer]] is neutered. On 2017/02/13 22:44:23, adamk wrote: > Rather than making this a TODO, can you point at the bug about how v8 doesn't > throw as often as the spec requires? That'll make it easier for folks reading > this in the future to understand why we don't throw here. Done as discussed (I think!) https://codereview.chromium.org/2697593002/diff/120001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:212: // TODO(caitp): throw if array.[[ViewedArrayBuffer]] is neutered. On 2017/02/13 22:44:23, adamk wrote: > Same here. Done. https://codereview.chromium.org/2697593002/diff/120001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-copywithin.js (right): https://codereview.chromium.org/2697593002/diff/120001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-copywithin.js:243: // TODO(caitp): this should throw due to being invoked on a TypedArray with a On 2017/02/13 22:44:23, adamk wrote: > Same thing here as the other TODOs, I'd prefer that this point at a bug. Done. https://codereview.chromium.org/2697593002/diff/120001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-copywithin.js:245: array.copyWithin(tmp, tmp, tmp); On 2017/02/13 22:44:23, adamk wrote: > Is there an assertion you can make after the copyWithin call? Maybe just > assertEquals(0, arr.length)? Done
lgtm with this TODO fixes, assuming that bmeurer read the actual copyWithin implementation for his lgtm. https://codereview.chromium.org/2697593002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/140001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:180: // TODO(caitp): throw if array.[[ViewedArrayBuffer]] is neutered. Looks like this one got forgotten?
https://codereview.chromium.org/2697593002/diff/140001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/140001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:180: // TODO(caitp): throw if array.[[ViewedArrayBuffer]] is neutered. On 2017/02/14 00:57:18, adamk wrote: > Looks like this one got forgotten? Oop, the (per v8:...) from the caller of ValidateTypedArray belongs up here. Just moved the comment since it makes more sense up here
The CQ bit was checked by caitp@igalia.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 code looks good to me, AFAICT this fixes the original issue and doesn't change observable semantics. https://codereview.chromium.org/2697593002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/160001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:212: if (V8_UNLIKELY(array->WasNeutered())) return *array; Do you have a test which hits this condition?
https://codereview.chromium.org/2697593002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/160001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:212: if (V8_UNLIKELY(array->WasNeutered())) return *array; On 2017/02/14 18:11:45, Dan Ehrenberg wrote: > Do you have a test which hits this condition? The regression test hits this
https://codereview.chromium.org/2697593002/diff/160001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/160001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:212: if (V8_UNLIKELY(array->WasNeutered())) return *array; On 2017/02/14 18:12:43, caitp wrote: > On 2017/02/14 18:11:45, Dan Ehrenberg wrote: > > Do you have a test which hits this condition? > > The regression test hits this Wait, I thought this was the other early return case. AFAIK there are tests that hit this, but let me check...
https://codereview.chromium.org/2697593002/diff/160001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-copywithin.js (right): https://codereview.chromium.org/2697593002/diff/160001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-copywithin.js:244: // detached buffer (per v8:4648). Yeah, this test verifies the first early return, so it's covered.
lgtm https://codereview.chromium.org/2697593002/diff/160001/test/mjsunit/es6/typed... File test/mjsunit/es6/typedarray-copywithin.js (right): https://codereview.chromium.org/2697593002/diff/160001/test/mjsunit/es6/typed... test/mjsunit/es6/typedarray-copywithin.js:235: "array.[[ViewedArrayBuffer]] is neutered."); Oh sorry, I missed this before. This is actually an observable change, but it's fine IMO; this is a very minor edge case of an edge case, and it's bringing us closer to the spec.
The CQ bit was checked by caitp@igalia.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...
https://codereview.chromium.org/2697593002/diff/180001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/180001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:251: DCHECK_GT(len - count, 0); Shouldn't this be DCHECK_GE, since you could copyWithin a whole TypedArray onto itself? You could add a DCHECK_LE(to + count, len); DCHECK_LE(from + count, len)
https://codereview.chromium.org/2697593002/diff/180001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/180001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:251: DCHECK_GT(len - count, 0); On 2017/02/14 18:33:53, Dan Ehrenberg wrote: > Shouldn't this be DCHECK_GE, since you could copyWithin a whole TypedArray onto > itself? I think that's impossible because of the `if (count <= 0) return *array;` above, but I guess it doesn't matter much > You could add a DCHECK_LE(to + count, len); DCHECK_LE(from + count, len) Seemed like it would slow things down, but if that's preferred, sure
https://codereview.chromium.org/2697593002/diff/180001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2697593002/diff/180001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:251: DCHECK_GT(len - count, 0); On 2017/02/14 18:36:14, caitp wrote: > On 2017/02/14 18:33:53, Dan Ehrenberg wrote: > > Shouldn't this be DCHECK_GE, since you could copyWithin a whole TypedArray > onto > > itself? > > I think that's impossible because of the `if (count <= 0) return *array;` above, > but I guess it doesn't matter much > > > You could add a DCHECK_LE(to + count, len); DCHECK_LE(from + count, len) > > Seemed like it would slow things down, but if that's preferred, sure eh nvm, yeah that doesn't work, will fix that
The CQ bit was checked by caitp@igalia.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.
lgtm https://codereview.chromium.org/2697593002/diff/200001/src/js/array.js File src/js/array.js (right): https://codereview.chromium.org/2697593002/diff/200001/src/js/array.js#newcod... src/js/array.js:1273: // ES6 draft 03-17-15, section 22.1.3.3 nit: please use sec reference instead of the (possibly changing) section numbers
The CQ bit was checked by caitp@igalia.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...
Thanks, gonna reland shortly https://codereview.chromium.org/2697593002/diff/200001/src/js/array.js File src/js/array.js (right): https://codereview.chromium.org/2697593002/diff/200001/src/js/array.js#newcod... src/js/array.js:1273: // ES6 draft 03-17-15, section 22.1.3.3 On 2017/02/15 13:36:28, Camillo Bruni wrote: > nit: please use sec reference instead of the (possibly changing) section numbers Done.
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 caitp@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, adamk@chromium.org, littledan@chromium.org, cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2697593002/#ps220001 (title: "update comment")
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": 220001, "attempt_start_ts": 1487168279386150,
"parent_rev": "70791d0cc3350885c48e6c7fa8b61d959602eab9", "commit_rev":
"dc302c74be4183db1b84347afab893825c887add"}
Message was sent while issue was closed.
Description was changed from ========== Reland [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the algorithm The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ========== to ========== Reland [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the algorithm The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org Review-Url: https://codereview.chromium.org/2697593002 Cr-Commit-Position: refs/heads/master@{#43213} Committed: https://chromium.googlesource.com/v8/v8/+/dc302c74be4183db1b84347afab893825c8... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/v8/v8/+/dc302c74be4183db1b84347afab893825c8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
