TypedArray and ArrayBuffer support for @@species

TypedArray and ArrayBuffer support for @@species

This patch improves ArrayBuffer and TypedArray subclassing by adding
support for @@species and constructing outputs to certain methods
by creating an instance of the constructor determined by the
SpeciesConstructor algorithm, rather than fixed to a superclass or
naively the constructor. The new behavior is enabled by the
--harmony-species flag. Care is taken to not significantly change the
observable behavior when the flag is off. Previously, TypedArrays
already supported subclassing by reading the constructor of the
receiver, but ArrayBuffers did not, and this old behavior is
preserved and tested for, to avoid a multi-stage upgrade path and keep
things simple for users.

R=adamk
BUG=v8:4093
LOG=Y

Committed: https://crrev.com/2bd9bdbe62f27af9416bd74670ef96569c8728fe
Cr-Commit-Position: refs/heads/master@{#33223}

[Dan Ehrenberg, 2016-01-11 22:56:43]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-11 22:56:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1574903004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1574903004/1

[commit-bot: I haz the power, 2016-01-11 23:24:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-11 23:24:39]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-01-11 23:40:36]

Description was changed from

==========
TypedArray and ArrayBuffer support for @@species

This patch improves ArrayBuffer and TypedArray subclassing by adding
support for @@species and constructing outputs to certain methods
by creating an instance of the constructor determined by the
SpeciesConstructor algorithm, rather than fixed to a superclass or
naively the constructor. The new behavior is enabled by the
--harmony-species flag, but this patch does change behavior
somewhat when that flag is off. For example, TypedArray methods
like map previously output instances of this.constructor, and now
will output instances of the TypedArray superclass (such as
Uint8Array) rather than the particular subclass, until the full
@@species-based version is turned on.

R=adamk
BUG=v8:4093
LOG=Y
==========

to

==========
TypedArray and ArrayBuffer support for @@species

This patch improves ArrayBuffer and TypedArray subclassing by adding
support for @@species and constructing outputs to certain methods
by creating an instance of the constructor determined by the
SpeciesConstructor algorithm, rather than fixed to a superclass or
naively the constructor. The new behavior is enabled by the
--harmony-species flag. Care is taken to not significantly change the
observable behavior when the flag is off. Previously, TypedArrays
already supported subclassing by reading the constructor of the
receiver, but ArrayBuffers did not, and this old behavior is
preserved and tested for, to avoid a multi-stage upgrade path and keep
things simple for users.


R=adamk
BUG=v8:4093
LOG=Y
==========

[Dan Ehrenberg, 2016-01-11 23:40:41]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-11 23:40:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1574903004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1574903004/20001

[commit-bot: I haz the power, 2016-01-12 00:06:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-12 00:06:10]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-01-12 00:11:00]

https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js
File src/js/arraybuffer.js (right):

https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js#n...
src/js/arraybuffer.js:69: if (!IS_ARRAYBUFFER(result)) {
Is this exercised in some test?

https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js#n...
src/js/arraybuffer.js:73: if (%_ArrayBufferGetByteLength(result) < newLen) {
And this? Same question about most of these error cases.

https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js#n...
src/js/arraybuffer.js:76: if (result === this) {
FWIW this check precedes the byte length check in the spec, might as well match
those checks?

https://codereview.chromium.org/1574903004/diff/20001/src/js/runtime.js
File src/js/runtime.js (right):

https://codereview.chromium.org/1574903004/diff/20001/src/js/runtime.js#newco...
src/js/runtime.js:146: throw MakeTypeError(kSpeciesNotConstructor);
This error message seems wrong here, since @@species is not involved.

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js
File src/js/typedarray.js (right):

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:123: if (!skipCheckLength &&
%_TypedArrayGetLength(newTypedArray) < arg0) {
This skipLengthCheck is pretty strange-looking. Is this a different way of
implementing 22.2.4.6 step 3?

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:755: // in the write order, e.g., the constructor needs to
be called before
s/write/right/

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:758: return TypedArrayCreate(this, array, UNDEFINED,
UNDEFINED, true);
Why does this one skip the length check?

https://codereview.chromium.org/1574903004/diff/20001/test/mjsunit/es6/legacy...
File test/mjsunit/es6/legacy-subclassing.js (right):

https://codereview.chromium.org/1574903004/diff/20001/test/mjsunit/es6/legacy...
test/mjsunit/es6/legacy-subclassing.js:5: // Before Symbol.species was added,
ArrayBuffer subclasses constructed
Might as well add:

// Flags: --noharmony-species

to this? That way you won't have to add it later.

[Dan Ehrenberg, 2016-01-12 01:38:33]

I forgot to git add a couple files of tests
(mjsunit/harmony/{typedarray,arraybuffer}-species.js), and this version also
clears up some code and makes better error messages.

https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js
File src/js/arraybuffer.js (right):

https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js#n...
src/js/arraybuffer.js:69: if (!IS_ARRAYBUFFER(result)) {
On 2016/01/12 at 00:11:00, adamk wrote:
> Is this exercised in some test?

Test262, but that's not active yet so I wrote another test.

https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js#n...
src/js/arraybuffer.js:73: if (%_ArrayBufferGetByteLength(result) < newLen) {
On 2016/01/12 at 00:11:00, adamk wrote:
> And this? Same question about most of these error cases.

Oops, I forgot to add the file test/mjsunit/harmony/arraybuffer-species.js where
I had written a test for it. Now it's there.

https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js#n...
src/js/arraybuffer.js:76: if (result === this) {
On 2016/01/12 at 00:11:00, adamk wrote:
> FWIW this check precedes the byte length check in the spec, might as well
match those checks?

Was in test262, added an mjsunit test for it.

https://codereview.chromium.org/1574903004/diff/20001/src/js/runtime.js
File src/js/runtime.js (right):

https://codereview.chromium.org/1574903004/diff/20001/src/js/runtime.js#newco...
src/js/runtime.js:146: throw MakeTypeError(kSpeciesNotConstructor);
On 2016/01/12 at 00:11:00, adamk wrote:
> This error message seems wrong here, since @@species is not involved.

Added a separate error message.

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js
File src/js/typedarray.js (right):

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:123: if (!skipCheckLength &&
%_TypedArrayGetLength(newTypedArray) < arg0) {
On 2016/01/12 at 00:11:00, adamk wrote:
> This skipLengthCheck is pretty strange-looking. Is this a different way of
implementing 22.2.4.6 step 3?

Yes. Rephrased it to look more like the spec.

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:755: // in the write order, e.g., the constructor needs to
be called before
On 2016/01/12 at 00:11:00, adamk wrote:
> s/write/right/

Done

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:758: return TypedArrayCreate(this, array, UNDEFINED,
UNDEFINED, true);
On 2016/01/12 at 00:11:00, adamk wrote:
> Why does this one skip the length check?

The spec has no length check here.

[adamk, 2016-01-12 01:47:44]

Ah, looks better with tests. Just one point of confusion I hope you can clear
up.

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js
File src/js/typedarray.js (right):

https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:758: return TypedArrayCreate(this, array, UNDEFINED,
UNDEFINED, true);
On 2016/01/12 01:38:33, Dan Ehrenberg wrote:
> On 2016/01/12 at 00:11:00, adamk wrote:
> > Why does this one skip the length check?
> 
> The spec has no length check here.

The spec just calls TypedArrayFrom and passes len as the single item in
argumentList. Seems like that would do a check. See 22.2.2.1.1 steps 7.f & 11.
What am I missing?

[Dan Ehrenberg, 2016-01-12 01:53:19]

On 2016/01/12 at 01:47:44, adamk wrote:
> Ah, looks better with tests. Just one point of confusion I hope you can clear
up.
> 
> https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js
> File src/js/typedarray.js (right):
> 
>
https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js#ne...
> src/js/typedarray.js:758: return TypedArrayCreate(this, array, UNDEFINED,
UNDEFINED, true);
> On 2016/01/12 01:38:33, Dan Ehrenberg wrote:
> > On 2016/01/12 at 00:11:00, adamk wrote:
> > > Why does this one skip the length check?
> > 
> > The spec has no length check here.
> 
> The spec just calls TypedArrayFrom and passes len as the single item in
argumentList. Seems like that would do a check. See 22.2.2.1.1 steps 7.f & 11.
What am I missing?

Oh, right, my mistake. I think TypedArrayFrom will have to be rewritten as a
separate version from the Array.from. There are TODOs in the code that indicate
that changes are needed. I'd rather not add an extra checking path for this
argument combination when this code needs to be changed anyway. The original
motivation for the checking is to allow implementations to hoist bounds checking
out of the loops involving TypedArrays, and that doesn't apply here in this
definition of %TypedArray%.from.

[adamk, 2016-01-12 01:59:02]

On 2016/01/12 01:53:19, Dan Ehrenberg wrote:
> On 2016/01/12 at 01:47:44, adamk wrote:
> > Ah, looks better with tests. Just one point of confusion I hope you can
clear
> up.
> > 
> > https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js
> > File src/js/typedarray.js (right):
> > 
> >
>
https://codereview.chromium.org/1574903004/diff/20001/src/js/typedarray.js#ne...
> > src/js/typedarray.js:758: return TypedArrayCreate(this, array, UNDEFINED,
> UNDEFINED, true);
> > On 2016/01/12 01:38:33, Dan Ehrenberg wrote:
> > > On 2016/01/12 at 00:11:00, adamk wrote:
> > > > Why does this one skip the length check?
> > > 
> > > The spec has no length check here.
> > 
> > The spec just calls TypedArrayFrom and passes len as the single item in
> argumentList. Seems like that would do a check. See 22.2.2.1.1 steps 7.f & 11.
> What am I missing?
> 
> Oh, right, my mistake. I think TypedArrayFrom will have to be rewritten as a
> separate version from the Array.from. There are TODOs in the code that
indicate
> that changes are needed. I'd rather not add an extra checking path for this
> argument combination when this code needs to be changed anyway. The original
> motivation for the checking is to allow implementations to hoist bounds
checking
> out of the loops involving TypedArrays, and that doesn't apply here in this
> definition of %TypedArray%.from.

Sounds reasonable to me: you've already got some TODOs in there to fix how this
is done, just wanted to make sure I understood the spec.

lgtm

[Dan Ehrenberg, 2016-01-12 05:20:12]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2016-01-12 05:20:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1574903004/20002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1574903004/20002

[commit-bot: I haz the power, 2016-01-12 06:07:52]

Committed patchset #3 (id:20002)

[commit-bot: I haz the power, 2016-01-12 06:08:07]

Description was changed from

==========
TypedArray and ArrayBuffer support for @@species

This patch improves ArrayBuffer and TypedArray subclassing by adding
support for @@species and constructing outputs to certain methods
by creating an instance of the constructor determined by the
SpeciesConstructor algorithm, rather than fixed to a superclass or
naively the constructor. The new behavior is enabled by the
--harmony-species flag. Care is taken to not significantly change the
observable behavior when the flag is off. Previously, TypedArrays
already supported subclassing by reading the constructor of the
receiver, but ArrayBuffers did not, and this old behavior is
preserved and tested for, to avoid a multi-stage upgrade path and keep
things simple for users.


R=adamk
BUG=v8:4093
LOG=Y
==========

to

==========
TypedArray and ArrayBuffer support for @@species

This patch improves ArrayBuffer and TypedArray subclassing by adding
support for @@species and constructing outputs to certain methods
by creating an instance of the constructor determined by the
SpeciesConstructor algorithm, rather than fixed to a superclass or
naively the constructor. The new behavior is enabled by the
--harmony-species flag. Care is taken to not significantly change the
observable behavior when the flag is off. Previously, TypedArrays
already supported subclassing by reading the constructor of the
receiver, but ArrayBuffers did not, and this old behavior is
preserved and tested for, to avoid a multi-stage upgrade path and keep
things simple for users.

R=adamk
BUG=v8:4093
LOG=Y

Committed: https://crrev.com/2bd9bdbe62f27af9416bd74670ef96569c8728fe
Cr-Commit-Position: refs/heads/master@{#33223}
==========

[commit-bot: I haz the power, 2016-01-12 06:08:07]

Patchset 3 (id:??) landed as
https://crrev.com/2bd9bdbe62f27af9416bd74670ef96569c8728fe
Cr-Commit-Position: refs/heads/master@{#33223}