TypedArray.prototype.copyWithin method

TypedArray.prototype.copyWithin method

This patch adds the copyWithin method to TypedArrays. For the first
pass, the internals of Array.copyWithin are used. Eventually, a more
efficient form based on memcpy could be used instead.

BUG=v8:3578
LOG=Y
R=adamk@chromium.org, arv@chromium.org, caitpotter88@gmail.com

Committed: https://crrev.com/a863c4d3d82f72cc5f7e54eaf66a2a4336f1fdc2
Cr-Commit-Position: refs/heads/master@{#28381}

[dehrenberg, 2015-05-07 20:05:41]

This is based on https://codereview.chromium.org/1126313003/ so it includes that
patch for now, until that one goes in. If needed, the order can be reversed.

[caitp (gmail), 2015-05-07 20:13:20]

From a quick glance it looks good, will look in more detail in a few hours

https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js#n...
src/harmony-typedarray.js:71: this[to] = this[from];
This is a pretty good first implementation.

Might be worth leaving a TODO comment about doing it all with MemMove(), not
sure

https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js#n...
src/harmony-typedarray.js:83: function TypedArrayEvery(f /* thisArg */) {  //
length == 1
Moving the rest of these outside the macro is good, but I'm not sure it's in
scope for this CL. No strong opinion on that though

https://codereview.chromium.org/1131113002/diff/1/test/mjsunit/harmony/typeda...
File test/mjsunit/harmony/typedarray-copywithin.js (right):

https://codereview.chromium.org/1131113002/diff/1/test/mjsunit/harmony/typeda...
test/mjsunit/harmony/typedarray-copywithin.js:147: if (0) {
Leftover code?

[adamk, 2015-05-07 20:28:07]

https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js#n...
src/harmony-typedarray.js:71: this[to] = this[from];
On 2015/05/07 20:13:20, caitp wrote:
> This is a pretty good first implementation.
> 
> Might be worth leaving a TODO comment about doing it all with MemMove(), not
> sure

To expand on this a bit, is there any reason not to re-use the code for
Array.prototype.copyWithin here? This comment likely applies to many of these
methods, at least until optimized versions are written.

[dehrenberg, 2015-05-07 20:42:50]

On 2015/05/07 20:28:07, adamk wrote:
> https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js
> File src/harmony-typedarray.js (right):
> 
>
https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js#n...
> src/harmony-typedarray.js:71: this[to] = this[from];
> On 2015/05/07 20:13:20, caitp wrote:
> > This is a pretty good first implementation.
> > 
> > Might be worth leaving a TODO comment about doing it all with MemMove(), not
> > sure
> 
> To expand on this a bit, is there any reason not to re-use the code for
> Array.prototype.copyWithin here? This comment likely applies to many of these
> methods, at least until optimized versions are written.

There's a couple things that are different:
- The methods are supposed to return a TypeError if they are run with a receiver
which isn't a TypedArray
- The length should be read by reading the internal [[ArrayLength]] slot, rather
than a [[Get]] on length. This is observable in the following session, which
matches my reading of the spec:

d8> y = new Uint8Array([1, 2])
[object Uint8Array]
d8> Object.defineProperty(y, 'length', {value: 1})
[object Uint8Array]
d8> y.length
1
d8> Array.prototype.forEach.call(y, z=>print(z))
1
undefined
d8> y.forEach(z=>print(z))
1
2
undefined

Presumably, the ES6 spec does it this way to allow for more optimizations on
TypedArrays, so they don't have to think about all the silly object stuff.

Those two things happen pretty early up-front. So Array vs TypedArray could have
some different preambles, and then call into the same underlying private
function. That'd let us get rid of the code duplication. However, I am not sure
how I could share those functions between different js files without leaking
them out through some property of a global object. Any suggestions?

[adamk, 2015-05-07 20:46:57]

On 2015/05/07 20:42:50, dehrenberg wrote:
> On 2015/05/07 20:28:07, adamk wrote:
> > https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js
> > File src/harmony-typedarray.js (right):
> > 
> >
>
https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js#n...
> > src/harmony-typedarray.js:71: this[to] = this[from];
> > On 2015/05/07 20:13:20, caitp wrote:
> > > This is a pretty good first implementation.
> > > 
> > > Might be worth leaving a TODO comment about doing it all with MemMove(),
not
> > > sure
> > 
> > To expand on this a bit, is there any reason not to re-use the code for
> > Array.prototype.copyWithin here? This comment likely applies to many of
these
> > methods, at least until optimized versions are written.
> 
> There's a couple things that are different:
> - The methods are supposed to return a TypeError if they are run with a
receiver
> which isn't a TypedArray
> - The length should be read by reading the internal [[ArrayLength]] slot,
rather
> than a [[Get]] on length. This is observable in the following session, which
> matches my reading of the spec:
> 
> d8> y = new Uint8Array([1, 2])
> [object Uint8Array]
> d8> Object.defineProperty(y, 'length', {value: 1})
> [object Uint8Array]
> d8> y.length
> 1
> d8> Array.prototype.forEach.call(y, z=>print(z))
> 1
> undefined
> d8> y.forEach(z=>print(z))
> 1
> 2
> undefined
> 
> Presumably, the ES6 spec does it this way to allow for more optimizations on
> TypedArrays, so they don't have to think about all the silly object stuff.

Yes, all that sounds right to me, and my thought was (as you suggest below)
sharing the logic below the preamble, at least until we add more-optimized
versions that operate directly on the TypedArray backing store (as caitp
suggested in a comment).

> Those two things happen pretty early up-front. So Array vs TypedArray could
have
> some different preambles, and then call into the same underlying private
> function. That'd let us get rid of the code duplication. However, I am not
sure
> how I could share those functions between different js files without leaking
> them out through some property of a global object. Any suggestions?

The natives live in a funny environment where their global is not accessible to
author script. This is how, e.g., the $max and $min functions you're calling get
set up and accessed (see math.js for the implementation). So you could add
$arrayCopyWithinCommon or something in harmony-array.js and access it in
harmony-typedarray.js.

[dehrenberg, 2015-05-12 17:23:08]

On 2015/05/07 20:46:57, adamk wrote:
> On 2015/05/07 20:42:50, dehrenberg wrote:
> > On 2015/05/07 20:28:07, adamk wrote:
> > >
https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js
> > > File src/harmony-typedarray.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1131113002/diff/1/src/harmony-typedarray.js#n...
> > > src/harmony-typedarray.js:71: this[to] = this[from];
> > > On 2015/05/07 20:13:20, caitp wrote:
> > > > This is a pretty good first implementation.
> > > > 
> > > > Might be worth leaving a TODO comment about doing it all with MemMove(),
> not
> > > > sure
> > > 
> > > To expand on this a bit, is there any reason not to re-use the code for
> > > Array.prototype.copyWithin here? This comment likely applies to many of
> these
> > > methods, at least until optimized versions are written.
> > 
> > There's a couple things that are different:
> > - The methods are supposed to return a TypeError if they are run with a
> receiver
> > which isn't a TypedArray
> > - The length should be read by reading the internal [[ArrayLength]] slot,
> rather
> > than a [[Get]] on length. This is observable in the following session, which
> > matches my reading of the spec:
> > 
> > d8> y = new Uint8Array([1, 2])
> > [object Uint8Array]
> > d8> Object.defineProperty(y, 'length', {value: 1})
> > [object Uint8Array]
> > d8> y.length
> > 1
> > d8> Array.prototype.forEach.call(y, z=>print(z))
> > 1
> > undefined
> > d8> y.forEach(z=>print(z))
> > 1
> > 2
> > undefined
> > 
> > Presumably, the ES6 spec does it this way to allow for more optimizations on
> > TypedArrays, so they don't have to think about all the silly object stuff.
> 
> Yes, all that sounds right to me, and my thought was (as you suggest below)
> sharing the logic below the preamble, at least until we add more-optimized
> versions that operate directly on the TypedArray backing store (as caitp
> suggested in a comment).
> 
> > Those two things happen pretty early up-front. So Array vs TypedArray could
> have
> > some different preambles, and then call into the same underlying private
> > function. That'd let us get rid of the code duplication. However, I am not
> sure
> > how I could share those functions between different js files without leaking
> > them out through some property of a global object. Any suggestions?
> 
> The natives live in a funny environment where their global is not accessible
to
> author script. This is how, e.g., the $max and $min functions you're calling
get
> set up and accessed (see math.js for the implementation). So you could add
> $arrayCopyWithinCommon or something in harmony-array.js and access it in
> harmony-typedarray.js.

OK, thanks for the suggestion; the new copyWithin implementation works like this
now.

[adamk, 2015-05-12 17:43:33]

Thanks for the refactor!

Please expand the CL description a little bit (e.g., "Implement the
TypedArray.prototype.copyWithin method"). In general I'd encourage you to follow
the CL description styleguide here:

https://www.chromium.org/developers/contributing-code#TOC-Change-List-Descrip...

https://codereview.chromium.org/1131113002/diff/20001/test/mjsunit/harmony/ty...
File test/mjsunit/harmony/typedarray-copywithin.js (right):

https://codereview.chromium.org/1131113002/diff/20001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarray-copywithin.js:1: // Copyright 2014 the V8
project authors. All rights reserved.
Nit: 2015

https://codereview.chromium.org/1131113002/diff/20001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarray-copywithin.js:144: return Object.freeze(new
constructor([1, 2, 3, 4, 5])).copyWithin(0, 3);
I think these tests are failing for the wrong reason. We throw when trying to
freeze, seal, or preventExtensions on a TypedArray.

It looks like this is not quite right per spec, though. Object.preventExtensions
should succeed. Object.freeze() should fail when trying to set an indexed
property to non-writable, and Object.seal should...succeed? Looks like another
bug to file.

[adamk, 2015-05-12 17:47:36]

https://codereview.chromium.org/1131113002/diff/20001/test/mjsunit/harmony/ty...
File test/mjsunit/harmony/typedarray-copywithin.js (right):

https://codereview.chromium.org/1131113002/diff/20001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarray-copywithin.js:144: return Object.freeze(new
constructor([1, 2, 3, 4, 5])).copyWithin(0, 3);
On 2015/05/12 17:43:33, adamk wrote:
> I think these tests are failing for the wrong reason. We throw when trying to
> freeze, seal, or preventExtensions on a TypedArray.
> 
> It looks like this is not quite right per spec, though.
Object.preventExtensions
> should succeed. Object.freeze() should fail when trying to set an indexed
> property to non-writable, and Object.seal should...succeed? Looks like another
> bug to file.

Small correction: by "failing for the wrong reason" I meant "throwing for the
wrong reason".

[dehrenberg, 2015-05-12 18:37:11]

On 2015/05/12 17:47:36, adamk wrote:
>
https://codereview.chromium.org/1131113002/diff/20001/test/mjsunit/harmony/ty...
> File test/mjsunit/harmony/typedarray-copywithin.js (right):
> 
>
https://codereview.chromium.org/1131113002/diff/20001/test/mjsunit/harmony/ty...
> test/mjsunit/harmony/typedarray-copywithin.js:144: return Object.freeze(new
> constructor([1, 2, 3, 4, 5])).copyWithin(0, 3);
> On 2015/05/12 17:43:33, adamk wrote:
> > I think these tests are failing for the wrong reason. We throw when trying
to
> > freeze, seal, or preventExtensions on a TypedArray.
> > 
> > It looks like this is not quite right per spec, though.
> Object.preventExtensions
> > should succeed. Object.freeze() should fail when trying to set an indexed
> > property to non-writable, and Object.seal should...succeed? Looks like
another
> > bug to file.
> 
> Small correction: by "failing for the wrong reason" I meant "throwing for the
> wrong reason".

Done

[adamk, 2015-05-12 18:51:08]

lgtm

[dehrenberg, 2015-05-12 18:53:01]

The CQ bit was checked by dehrenberg@chromium.org

[commit-bot: I haz the power, 2015-05-12 18:53:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131113002/60001

[commit-bot: I haz the power, 2015-05-12 19:21:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-12 19:21:21]

Patchset 4 (id:??) landed as
https://crrev.com/a863c4d3d82f72cc5f7e54eaf66a2a4336f1fdc2
Cr-Commit-Position: refs/heads/master@{#28381}