Add Array support for @@species and subclassing

Add Array support for @@species and subclassing

This patch implements @@species, guarded behind the --harmony-species
flag, on Arrays. Methods which return an Array will instead return
the appropriate instance based on the ArraySpeciesCreate algorithm.
The algorithm is implemented in C++ to get access to realm information
and to implement some Array methods in C++, but it is also accessed
from JavaScript through a new runtime function. A couple interactive
Octane runs show no performance regression with the flag turned off,
but turning --harmony-species on will surely have a significant
regression, as Array methods now heavily use ObjectDefineProperty.

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

Committed: https://crrev.com/6e96223750a748a7030386cb12904c1163e61d9d
Cr-Commit-Position: refs/heads/master@{#33144}

[Dan Ehrenberg, 2016-01-05 22:28:51]

Description was changed from

==========
Array aspects of @@species

BUG=
==========

to

==========
Add Array support for @@species and subclassing

This patch implements @@species, guarded behind the --harmony-species
flag, on Arrays. Methods which return an Array will instead return
the appropriate instance based on the ArraySpeciesCreate algorithm.
The algorithm is implemented in C++ to get access to realm information
and to implement some Array methods in C++, but it is also accessed
from JavaScript through a new runtime function. A couple interactive
Octane runs show no performance regression with the flag turned off,
but turning --harmony-species on will surely have a significant
regression, as Array methods now heavily use ObjectDefineProperty.

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

[Dan Ehrenberg, 2016-01-05 22:28:51]

littledan@chromium.org changed reviewers:
+ adamk@chromium.org, cbruni@chromium.org

[adamk, 2016-01-06 00:10:12]

https://codereview.chromium.org/1560763002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1560763002/diff/20001/src/builtins.cc#newcode461
src/builtins.cc:461: // TODO(littledan): Look up @@species only once, not once
here and
Please add a test that exercises this; that'll make sure that it's tested when
the TODO is fixed.

https://codereview.chromium.org/1560763002/diff/20001/src/builtins.cc#newcode554
src/builtins.cc:554: // TODO(littledan): Look up @@species only once, not once
here and
Same note as the TODO above.

https://codereview.chromium.org/1560763002/diff/20001/src/builtins.cc#newcode630
src/builtins.cc:630: ArrayConcatVisitor(Isolate* isolate, Handle<Object>
storage,
I think you should consider doing the concat stuff in a separate patch, as
cbruni should really review it (or I guess we can review this together as one).

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

https://codereview.chromium.org/1560763002/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:32: var InnerArrayMap;
You can now remove this import.

https://codereview.chromium.org/1560763002/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:505: for (var n = 0; n < captured; n++) {
Can you use 'i' instead of 'n' for the name here?

https://codereview.chromium.org/1560763002/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:506: output[n] = result[n];
Nit: indentation (need another space)

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1609
src/objects.cc:1609: Isolate* isolate, Handle<Object> originalArray) {
original_array

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1610
src/objects.cc:1610: Context* native_context =
isolate->context()->native_context();
Please stuff this in a handle immediately; raw pointers in a handlified function
are Bad News.

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1614
src/objects.cc:1614: Handle<Object>
constructor(isolate->heap()->undefined_value(), isolate);
isolate->factory()->undefined_value() will hand you back a handle in one fewer
step.

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1620
src/objects.cc:1620: Object::GetProperty(isolate, originalArray, "constructor",
STRICT),
You can make this slightly faster by using a different GetProperty overload:

Object::GetProperty(original_array, isolate->factory()->constructor_string())

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1623
src/objects.cc:1623: Context* constructor_context =
Again, please store in a handle.

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1628
src/objects.cc:1628: Handle<Object>(isolate->heap()->undefined_value(),
isolate);
isolate->factory()->undefined_value()

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1635
src/objects.cc:1635: Handle<Name>(isolate->heap()->species_symbol())),
isolate->factory()->species_symbol()

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1639
src/objects.cc:1639: Handle<Object>(isolate->heap()->undefined_value(),
isolate);
factory version

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1647
src/objects.cc:1647: isolate->Throw(*isolate->factory()->NewTypeError(
I think you want to return what Throw() returns. Also you should be calling the
version that takes a Handle.

https://codereview.chromium.org/1560763002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1560763002/diff/20001/src/objects.h#newcode1351
src/objects.h:1351: Isolate* isolate, Handle<Object> originalArray);
Nit: original_array

https://codereview.chromium.org/1560763002/diff/20001/src/runtime/runtime-arr...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/1560763002/diff/20001/src/runtime/runtime-arr...
src/runtime/runtime-array.cc:494: 
No blank line here.

https://codereview.chromium.org/1560763002/diff/20001/src/runtime/runtime-arr...
src/runtime/runtime-array.cc:496: CONVERT_ARG_HANDLE_CHECKED(Object,
originalArray, 0);
original_array

https://codereview.chromium.org/1560763002/diff/20001/test/mjsunit/harmony/ar...
File test/mjsunit/harmony/array-species.js (right):

https://codereview.chromium.org/1560763002/diff/20001/test/mjsunit/harmony/ar...
test/mjsunit/harmony/array-species.js:7: // Test the ES2015 @@species feature
More tests I'd like to see:

- Test that the length is passed to the constructor
- As noted elsewhere, that we erroneously fetch @@species twice

https://codereview.chromium.org/1560763002/diff/20001/test/mjsunit/harmony/ar...
test/mjsunit/harmony/array-species.js:41: assertEquals(MyObject,
Array.prototype.map.call(new MyNonArray(), ()=>{}).constructor);
Please wrap at 80 chars here and below (dunno why we don't have a lint check for
js).

[Dan Ehrenberg, 2016-01-07 00:42:58]

Thanks for the detailed review; PTAL

https://codereview.chromium.org/1560763002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1560763002/diff/20001/src/builtins.cc#newcode461
src/builtins.cc:461: // TODO(littledan): Look up @@species only once, not once
here and
On 2016/01/06 at 00:10:12, adamk wrote:
> Please add a test that exercises this; that'll make sure that it's tested when
the TODO is fixed.

Done

https://codereview.chromium.org/1560763002/diff/20001/src/builtins.cc#newcode554
src/builtins.cc:554: // TODO(littledan): Look up @@species only once, not once
here and
On 2016/01/06 at 00:10:11, adamk wrote:
> Same note as the TODO above.

Done

https://codereview.chromium.org/1560763002/diff/20001/src/builtins.cc#newcode630
src/builtins.cc:630: ArrayConcatVisitor(Isolate* isolate, Handle<Object>
storage,
On 2016/01/06 at 00:10:11, adamk wrote:
> I think you should consider doing the concat stuff in a separate patch, as
cbruni should really review it (or I guess we can review this together as one).

Good idea; this patch doesn't include the concat stuff anymore.

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

https://codereview.chromium.org/1560763002/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:505: for (var n = 0; n < captured; n++) {
On 2016/01/06 at 00:10:12, adamk wrote:
> Can you use 'i' instead of 'n' for the name here?

Done

https://codereview.chromium.org/1560763002/diff/20001/src/js/typedarray.js#ne...
src/js/typedarray.js:506: output[n] = result[n];
On 2016/01/06 at 00:10:12, adamk wrote:
> Nit: indentation (need another space)

Done

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1609
src/objects.cc:1609: Isolate* isolate, Handle<Object> originalArray) {
On 2016/01/06 at 00:10:12, adamk wrote:
> original_array

Done

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1610
src/objects.cc:1610: Context* native_context =
isolate->context()->native_context();
On 2016/01/06 at 00:10:12, adamk wrote:
> Please stuff this in a handle immediately; raw pointers in a handlified
function are Bad News.

Oops, fixed (turns out the Handle API is more ergonomic anyway. I was sort of
assuming that the native context wouldn't go away while this was executing, but
that is not a reasonable assumption.)

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1614
src/objects.cc:1614: Handle<Object>
constructor(isolate->heap()->undefined_value(), isolate);
On 2016/01/06 at 00:10:12, adamk wrote:
> isolate->factory()->undefined_value() will hand you back a handle in one fewer
step.

Done

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1620
src/objects.cc:1620: Object::GetProperty(isolate, originalArray, "constructor",
STRICT),
On 2016/01/06 at 00:10:12, adamk wrote:
> You can make this slightly faster by using a different GetProperty overload:
> 
> Object::GetProperty(original_array, isolate->factory()->constructor_string())

Done

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1623
src/objects.cc:1623: Context* constructor_context =
On 2016/01/06 at 00:10:12, adamk wrote:
> Again, please store in a handle.

Done

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1628
src/objects.cc:1628: Handle<Object>(isolate->heap()->undefined_value(),
isolate);
On 2016/01/06 at 00:10:12, adamk wrote:
> isolate->factory()->undefined_value()

Done

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1635
src/objects.cc:1635: Handle<Name>(isolate->heap()->species_symbol())),
On 2016/01/06 at 00:10:12, adamk wrote:
> isolate->factory()->species_symbol()

Done

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1639
src/objects.cc:1639: Handle<Object>(isolate->heap()->undefined_value(),
isolate);
On 2016/01/06 at 00:10:12, adamk wrote:
> factory version

Done

https://codereview.chromium.org/1560763002/diff/20001/src/objects.cc#newcode1647
src/objects.cc:1647: isolate->Throw(*isolate->factory()->NewTypeError(
On 2016/01/06 at 00:10:12, adamk wrote:
> I think you want to return what Throw() returns. Also you should be calling
the version that takes a Handle.

Fixed

https://codereview.chromium.org/1560763002/diff/20001/src/runtime/runtime-arr...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/1560763002/diff/20001/src/runtime/runtime-arr...
src/runtime/runtime-array.cc:494: 
On 2016/01/06 at 00:10:12, adamk wrote:
> No blank line here.

Fixed

https://codereview.chromium.org/1560763002/diff/20001/src/runtime/runtime-arr...
src/runtime/runtime-array.cc:496: CONVERT_ARG_HANDLE_CHECKED(Object,
originalArray, 0);
On 2016/01/06 at 00:10:12, adamk wrote:
> original_array

Fixed

https://codereview.chromium.org/1560763002/diff/20001/test/mjsunit/harmony/ar...
File test/mjsunit/harmony/array-species.js (right):

https://codereview.chromium.org/1560763002/diff/20001/test/mjsunit/harmony/ar...
test/mjsunit/harmony/array-species.js:7: // Test the ES2015 @@species feature
On 2016/01/06 at 00:10:12, adamk wrote:
> More tests I'd like to see:
> 
> - Test that the length is passed to the constructor
> - As noted elsewhere, that we erroneously fetch @@species twice

Added those tests at the end

https://codereview.chromium.org/1560763002/diff/20001/test/mjsunit/harmony/ar...
test/mjsunit/harmony/array-species.js:41: assertEquals(MyObject,
Array.prototype.map.call(new MyNonArray(), ()=>{}).constructor);
On 2016/01/06 at 00:10:12, adamk wrote:
> Please wrap at 80 chars here and below (dunno why we don't have a lint check
for js).

Fixed

[Dan Ehrenberg, 2016-01-07 00:43:06]

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

[commit-bot: I haz the power, 2016-01-07 00:43:17]

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

[commit-bot: I haz the power, 2016-01-07 00:59:19]

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

[commit-bot: I haz the power, 2016-01-07 00:59:20]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/12097)

[Dan Ehrenberg, 2016-01-07 01:06:29]

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

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

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

[adamk, 2016-01-07 01:11:04]

Looking good, just a few more bits.

https://codereview.chromium.org/1560763002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1560763002/diff/60001/src/objects.cc#newcode1627
src/objects.cc:1627:
JSReceiver::GetFunctionRealm(Handle<JSReceiver>::cast(constructor)),
Did this need changing to make proxies work? Can you add a test where a Proxy is
shows up here?

https://codereview.chromium.org/1560763002/diff/60001/src/objects.cc#newcode1649
src/objects.cc:1649: return
isolate->Throw<Object>(isolate->factory()->NewTypeError(
Looks like there's a macro for this:

THROW_NEW_ERROR(isolate,
                NewTypeError(MessageTemplate::kSpeciesNotConstructor),
                Object);

I'm not totally sure that's worth a macro, but it's what all the other code in
objects.cc does, so I'd prefer consistency here.

[Dan Ehrenberg, 2016-01-07 01:35:17]

https://codereview.chromium.org/1560763002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1560763002/diff/60001/src/objects.cc#newcode1627
src/objects.cc:1627:
JSReceiver::GetFunctionRealm(Handle<JSReceiver>::cast(constructor)),
On 2016/01/07 at 01:11:04, adamk wrote:
> Did this need changing to make proxies work? Can you add a test where a Proxy
is shows up here?

Yeah, you caught me. I couldn't think of a great test that really exercised
their Proxy-ness, but I added a test that makes sure that things don't crash in
this case.

https://codereview.chromium.org/1560763002/diff/60001/src/objects.cc#newcode1649
src/objects.cc:1649: return
isolate->Throw<Object>(isolate->factory()->NewTypeError(
On 2016/01/07 at 01:11:04, adamk wrote:
> Looks like there's a macro for this:
> 
> THROW_NEW_ERROR(isolate,
>                 NewTypeError(MessageTemplate::kSpeciesNotConstructor),
>                 Object);
> 
> I'm not totally sure that's worth a macro, but it's what all the other code in
objects.cc does, so I'd prefer consistency here.

OK, sure.

[adamk, 2016-01-07 01:38:30]

lgtm

https://codereview.chromium.org/1560763002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1560763002/diff/60001/src/objects.cc#newcode1627
src/objects.cc:1627:
JSReceiver::GetFunctionRealm(Handle<JSReceiver>::cast(constructor)),
On 2016/01/07 01:35:17, Dan Ehrenberg wrote:
> On 2016/01/07 at 01:11:04, adamk wrote:
> > Did this need changing to make proxies work? Can you add a test where a
Proxy
> is shows up here?
> 
> Yeah, you caught me. I couldn't think of a great test that really exercised
> their Proxy-ness, but I added a test that makes sure that things don't crash
in
> this case.

Not crashing is sufficient to allay my fears.

https://codereview.chromium.org/1560763002/diff/60001/src/objects.cc#newcode1649
src/objects.cc:1649: return
isolate->Throw<Object>(isolate->factory()->NewTypeError(
On 2016/01/07 01:35:17, Dan Ehrenberg wrote:
> On 2016/01/07 at 01:11:04, adamk wrote:
> > Looks like there's a macro for this:
> > 
> > THROW_NEW_ERROR(isolate,
> >                 NewTypeError(MessageTemplate::kSpeciesNotConstructor),
> >                 Object);
> > 
> > I'm not totally sure that's worth a macro, but it's what all the other code
in
> objects.cc does, so I'd prefer consistency here.
> 
> OK, sure.

Sorry, it's the week of macros!

[Dan Ehrenberg, 2016-01-07 01:44:36]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2016-01-07 01:44:42]

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

[commit-bot: I haz the power, 2016-01-07 02:29:56]

Committed patchset #6 (id:70011)

[commit-bot: I haz the power, 2016-01-07 02:30:10]

Description was changed from

==========
Add Array support for @@species and subclassing

This patch implements @@species, guarded behind the --harmony-species
flag, on Arrays. Methods which return an Array will instead return
the appropriate instance based on the ArraySpeciesCreate algorithm.
The algorithm is implemented in C++ to get access to realm information
and to implement some Array methods in C++, but it is also accessed
from JavaScript through a new runtime function. A couple interactive
Octane runs show no performance regression with the flag turned off,
but turning --harmony-species on will surely have a significant
regression, as Array methods now heavily use ObjectDefineProperty.

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

to

==========
Add Array support for @@species and subclassing

This patch implements @@species, guarded behind the --harmony-species
flag, on Arrays. Methods which return an Array will instead return
the appropriate instance based on the ArraySpeciesCreate algorithm.
The algorithm is implemented in C++ to get access to realm information
and to implement some Array methods in C++, but it is also accessed
from JavaScript through a new runtime function. A couple interactive
Octane runs show no performance regression with the flag turned off,
but turning --harmony-species on will surely have a significant
regression, as Array methods now heavily use ObjectDefineProperty.

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

Committed: https://crrev.com/6e96223750a748a7030386cb12904c1163e61d9d
Cr-Commit-Position: refs/heads/master@{#33144}
==========

[commit-bot: I haz the power, 2016-01-07 02:30:11]

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