Optimize @@species based on a global 'protector' cell

Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Array subclasses will incur the slowness of looking
up @@species, but their use won't slow down invocations of, for example,
Array.prototype.slice on Array base class instances.

Possible future optimizations:
- For the fallback case where the assumptions don't hold, optimize the two
  property lookups.
- For Array.prototype.slice and Array.prototype.splice, even if the full lookup
  of @@species needs to take place, we still could take the rest of the C++
  fastpath. However, to do this correctly requires changing the calling convention
  from C++ to JS to pass the @@species out, so it is not attempted in this patch.

With this patch, microbenchmarks of Array.prototype.slice do not suffer a
noticeable performance regression, unlike their previous 2.5x penalty.

TBR=hpayer@chromium.org

Committed: https://crrev.com/7033ae511f3cca7e5464353e80acbba645e07c44
Cr-Commit-Position: refs/heads/master@{#34199}

[Dan Ehrenberg, 2016-02-17 06:26:43]

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

[commit-bot: I haz the power, 2016-02-17 06:26:46]

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

[commit-bot: I haz the power, 2016-02-17 06:27:36]

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

[commit-bot: I haz the power, 2016-02-17 06:27:37]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/1476)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/1473)

[Dan Ehrenberg, 2016-02-17 23:17:33]

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

[commit-bot: I haz the power, 2016-02-17 23:17:40]

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

[Dan Ehrenberg, 2016-02-17 23:23:46]

Description was changed from

==========
Optimize @@species based on a global 'protector' cell
==========

to

==========
Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Later optimizations are possible in addition to
optimize
the two property lookups for the other case where these assumptions don't hold.
==========

[Dan Ehrenberg, 2016-02-17 23:23:46]

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

[commit-bot: I haz the power, 2016-02-17 23:25:04]

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

[commit-bot: I haz the power, 2016-02-17 23:25:05]

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

[Dan Ehrenberg, 2016-02-17 23:25:54]

Description was changed from

==========
Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Later optimizations are possible in addition to
optimize
the two property lookups for the other case where these assumptions don't hold.
==========

to

==========
Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Later optimizations are possible in addition to
optimize the two property lookups for the other case where these assumptions
don't hold.

With this patch, microbenchmarks of Array.prototype.slice do not suffer a 
noticeable performance regression, unlike their previous 2.5x penalty.
==========

[Dan Ehrenberg, 2016-02-17 23:39:08]

Description was changed from

==========
Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Later optimizations are possible in addition to
optimize the two property lookups for the other case where these assumptions
don't hold.

With this patch, microbenchmarks of Array.prototype.slice do not suffer a 
noticeable performance regression, unlike their previous 2.5x penalty.
==========

to

==========
Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Array subclasses will incur the slowness of looking
up @@species, but their use won't slow down invocations of, for example,
Array.prototype.slice on Array base class instances.

Possible future optimizations:
- For the fallback case where the assumptions don't hold, optimize the two
  property lookups.
- For Array.prototype.slice and Array.prototype.splice, even if the full lookup
  of @@species needs to take place, we still could take the rest of the C++
  fastpath. However, to do this correctly requires changing the calling
convention
  from C++ to JS to pass the @@species out, so it is not attempted in this
patch.

With this patch, microbenchmarks of Array.prototype.slice do not suffer a 
noticeable performance regression, unlike their previous 2.5x penalty.
==========

[Dan Ehrenberg, 2016-02-18 00:46:34]

Camillo, there are a couple small test failures, but I'm wondering if you could
review it anyway for design feedback. If this fixes the benchmark regressions
(both macro and micro; I haven't fully evaluated this yet), would it be enough
to ship species, or do you think more is needed?

[Dan Ehrenberg, 2016-02-18 00:51:27]

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

[commit-bot: I haz the power, 2016-02-18 00:51:36]

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

[commit-bot: I haz the power, 2016-02-18 01:04:16]

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

[commit-bot: I haz the power, 2016-02-18 01:04:17]

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/13924)

[Dan Ehrenberg, 2016-02-18 22:49:36]

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

[commit-bot: I haz the power, 2016-02-18 22:49:58]

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

[commit-bot: I haz the power, 2016-02-18 22:50:59]

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

[commit-bot: I haz the power, 2016-02-18 22:51:00]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/15677)

[Dan Ehrenberg, 2016-02-18 22:59:55]

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

[commit-bot: I haz the power, 2016-02-18 23:00:07]

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

[commit-bot: I haz the power, 2016-02-18 23:32:47]

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

[commit-bot: I haz the power, 2016-02-18 23:32:48]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-02-18 23:44:01]

PTAL, all the issues I know of have been fixed.

[adamk, 2016-02-19 00:01:17]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2016-02-19 00:01:18]

A few stylistic notes, deferring to cbruni for the meat of this review.

https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc#newcode...
src/isolate.cc:2533: CHECK(factory()->species_protector()->value()->IsSmi());
Normally this and the surrounding CHECKs would be DCHECKs. Do you really
need/want to do these outside a debug build?

https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc#newcode159
src/lookup.cc:159: Object* context = heap()->native_contexts_list();
The use of raw pointers instead of handles looks a little funny here; is there a
specific reason this isn't Handlified?

[Dan Ehrenberg, 2016-02-19 00:15:37]

https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc#newcode...
src/isolate.cc:2533: CHECK(factory()->species_protector()->value()->IsSmi());
On 2016/02/19 at 00:01:17, adamk wrote:
> Normally this and the surrounding CHECKs would be DCHECKs. Do you really
need/want to do these outside a debug build?

That would be my intuition, except I was copying what the normal Array protector
does, since I assumed those conventions apply here (except the next checks are
new). Anyway, I don't think this path needs to be extremely fast, since you can
only invalidate the species protector once in the lifetime of the isolate, and
these are not expensive checks.

https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc#newcode159
src/lookup.cc:159: Object* context = heap()->native_contexts_list();
On 2016/02/19 at 00:01:17, adamk wrote:
> The use of raw pointers instead of handles looks a little funny here; is there
a specific reason this isn't Handlified?

I was following the way contexts are used by the array protector code. I guess I
wasn't accounting for the case where contexts would go away while this stuff is
happening. I don't see how that could happen, since we don't allocate or call
into JS, but seems like good practice anyway. Should I change both of these?

[adamk, 2016-02-19 01:06:01]

https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc#newcode...
src/isolate.cc:2533: CHECK(factory()->species_protector()->value()->IsSmi());
On 2016/02/19 00:15:37, Dan Ehrenberg wrote:
> On 2016/02/19 at 00:01:17, adamk wrote:
> > Normally this and the surrounding CHECKs would be DCHECKs. Do you really
> need/want to do these outside a debug build?
> 
> That would be my intuition, except I was copying what the normal Array
protector
> does, since I assumed those conventions apply here (except the next checks are
> new). Anyway, I don't think this path needs to be extremely fast, since you
can
> only invalidate the species protector once in the lifetime of the isolate, and
> these are not expensive checks.

I'll leave this to cbruni or other runtime folks since the ArrayProtector is
there's. Consistency seems reasonable.

https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc#newcode159
src/lookup.cc:159: Object* context = heap()->native_contexts_list();
On 2016/02/19 00:15:37, Dan Ehrenberg wrote:
> On 2016/02/19 at 00:01:17, adamk wrote:
> > The use of raw pointers instead of handles looks a little funny here; is
there
> a specific reason this isn't Handlified?
> 
> I was following the way contexts are used by the array protector code. I guess
I
> wasn't accounting for the case where contexts would go away while this stuff
is
> happening. I don't see how that could happen, since we don't allocate or call
> into JS, but seems like good practice anyway. Should I change both of these?

I agree that I don't see any problematic code here, but if you want to stay
unhandlified the best practice would be to add a DisallowHeapAllocation object
on the stack which will at least CHECK-fail in debug mode if something changes
and starts allocating.

[Dan Ehrenberg, 2016-02-19 02:16:29]

On 2016/02/19 at 01:06:01, adamk wrote:
> https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc#newcode...
> src/isolate.cc:2533: CHECK(factory()->species_protector()->value()->IsSmi());
> On 2016/02/19 00:15:37, Dan Ehrenberg wrote:
> > On 2016/02/19 at 00:01:17, adamk wrote:
> > > Normally this and the surrounding CHECKs would be DCHECKs. Do you really
> > need/want to do these outside a debug build?
> > 
> > That would be my intuition, except I was copying what the normal Array
protector
> > does, since I assumed those conventions apply here (except the next checks
are
> > new). Anyway, I don't think this path needs to be extremely fast, since you
can
> > only invalidate the species protector once in the lifetime of the isolate,
and
> > these are not expensive checks.
> 
> I'll leave this to cbruni or other runtime folks since the ArrayProtector is
there's. Consistency seems reasonable.
> 
> https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc
> File src/lookup.cc (right):
> 
>
https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc#newcode159
> src/lookup.cc:159: Object* context = heap()->native_contexts_list();
> On 2016/02/19 00:15:37, Dan Ehrenberg wrote:
> > On 2016/02/19 at 00:01:17, adamk wrote:
> > > The use of raw pointers instead of handles looks a little funny here; is
there
> > a specific reason this isn't Handlified?
> > 
> > I was following the way contexts are used by the array protector code. I
guess I
> > wasn't accounting for the case where contexts would go away while this stuff
is
> > happening. I don't see how that could happen, since we don't allocate or
call
> > into JS, but seems like good practice anyway. Should I change both of these?
> 
> I agree that I don't see any problematic code here, but if you want to stay
unhandlified the best practice would be to add a DisallowHeapAllocation object
on the stack which will at least CHECK-fail in debug mode if something changes
and starts allocating.

Added that in both cases.

[Dan Ehrenberg, 2016-02-19 02:16:34]

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

[commit-bot: I haz the power, 2016-02-19 02:16:39]

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

[Dan Ehrenberg, 2016-02-19 02:16:41]

https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1689733002/diff/140001/src/isolate.cc#newcode...
src/isolate.cc:2533: CHECK(factory()->species_protector()->value()->IsSmi());
On 2016/02/19 at 01:06:01, adamk wrote:
> On 2016/02/19 00:15:37, Dan Ehrenberg wrote:
> > On 2016/02/19 at 00:01:17, adamk wrote:
> > > Normally this and the surrounding CHECKs would be DCHECKs. Do you really
> > need/want to do these outside a debug build?
> > 
> > That would be my intuition, except I was copying what the normal Array
protector
> > does, since I assumed those conventions apply here (except the next checks
are
> > new). Anyway, I don't think this path needs to be extremely fast, since you
can
> > only invalidate the species protector once in the lifetime of the isolate,
and
> > these are not expensive checks.
> 
> I'll leave this to cbruni or other runtime folks since the ArrayProtector is
there's. Consistency seems reasonable.

Actually, I can't find those CHECKS I thought I was copying. Changed to DCHECK.

[commit-bot: I haz the power, 2016-02-19 02:44:45]

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

[commit-bot: I haz the power, 2016-02-19 02:44:47]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-02-22 12:44:47]

lgtm with nits.

https://codereview.chromium.org/1689733002/diff/180001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1689733002/diff/180001/src/lookup.cc#newcode170
src/lookup.cc:170: }
nit: Could you copy over and adapt the helper ContextForArrayOrObjectPrototype
from https://codereview.chromium.org/1409123003?
At least for me that made the intentions of this boilerplate code clearer.

Same below.

https://codereview.chromium.org/1689733002/diff/180001/src/lookup.cc#newcode179
src/lookup.cc:179: if (current_context->array_function() == *holder_) {
you could check that the value we set is not the array_function (same above).
I could imagine that this would happen in a shim tying to emulate @@species on
an unsupported platform. In our case this would accidentally destroy
performance.

[Camillo Bruni, 2016-02-22 15:29:34]

https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1689733002/diff/140001/src/lookup.cc#newcode159
src/lookup.cc:159: Object* context = heap()->native_contexts_list();
On 2016/02/19 at 01:06:01, adamk wrote:
> On 2016/02/19 00:15:37, Dan Ehrenberg wrote:
> > On 2016/02/19 at 00:01:17, adamk wrote:
> > > The use of raw pointers instead of handles looks a little funny here; is
there
> > a specific reason this isn't Handlified?
> > 
> > I was following the way contexts are used by the array protector code. I
guess I
> > wasn't accounting for the case where contexts would go away while this stuff
is
> > happening. I don't see how that could happen, since we don't allocate or
call
> > into JS, but seems like good practice anyway. Should I change both of these?
> 
> I agree that I don't see any problematic code here, but if you want to stay
unhandlified the best practice would be to add a DisallowHeapAllocation object
on the stack which will at least CHECK-fail in debug mode if something changes
and starts allocating.

right, I think the other code forgot to add DisallowHeapAllocation. Should be
fine in the latest patch.

[Dan Ehrenberg, 2016-02-22 20:01:22]

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

[commit-bot: I haz the power, 2016-02-22 20:01:29]

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

[Dan Ehrenberg, 2016-02-22 20:02:00]

https://codereview.chromium.org/1689733002/diff/180001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1689733002/diff/180001/src/lookup.cc#newcode170
src/lookup.cc:170: }
On 2016/02/22 at 12:44:47, cbruni wrote:
> nit: Could you copy over and adapt the helper ContextForArrayOrObjectPrototype
from https://codereview.chromium.org/1409123003?
> At least for me that made the intentions of this boilerplate code clearer.
> 
> Same below.

Done

https://codereview.chromium.org/1689733002/diff/180001/src/lookup.cc#newcode179
src/lookup.cc:179: if (current_context->array_function() == *holder_) {
On 2016/02/22 at 12:44:47, cbruni wrote:
> you could check that the value we set is not the array_function (same above).
> I could imagine that this would happen in a shim tying to emulate @@species on
an unsupported platform. In our case this would accidentally destroy
performance.

Not really sure how I could do that. Polyfill libraries tend to set @@species
correctly, as a getter with a function, e.g.,
https://github.com/zloirock/core-js/blob/a6035a4b11b3e8522f8981b26c8ee6a20b2e...
. It seems rather complicated to determine if that getter is an identity
function--I don't know a clear way to do that. And it would be incorrect to
execute it just on Array and check if it equals that Array!

I think the strategy in this case would be to optimize the 'constructor' lookup,
and for the @@species lookup, make some sort of IC in the C++ code to speed up
the lookup.

[commit-bot: I haz the power, 2016-02-22 20:04:04]

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

[commit-bot: I haz the power, 2016-02-22 20:04:05]

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

[Camillo Bruni, 2016-02-22 20:05:22]

https://codereview.chromium.org/1689733002/diff/180001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1689733002/diff/180001/src/lookup.cc#newcode179
src/lookup.cc:179: if (current_context->array_function() == *holder_) {
On 2016/02/22 at 20:02:00, Dan Ehrenberg wrote:
> On 2016/02/22 at 12:44:47, cbruni wrote:
> > you could check that the value we set is not the array_function (same
above).
> > I could imagine that this would happen in a shim tying to emulate @@species
on an unsupported platform. In our case this would accidentally destroy
performance.
> 
> Not really sure how I could do that. Polyfill libraries tend to set @@species
correctly, as a getter with a function, e.g.,
https://github.com/zloirock/core-js/blob/a6035a4b11b3e8522f8981b26c8ee6a20b2e...
. It seems rather complicated to determine if that getter is an identity
function--I don't know a clear way to do that. And it would be incorrect to
execute it just on Array and check if it equals that Array!
> 
> I think the strategy in this case would be to optimize the 'constructor'
lookup, and for the @@species lookup, make some sort of IC in the C++ code to
speed up the lookup.

I guess this is fine for now. I thought of checking if you only set the value
directly on array_function and it corresponds to the array_function we simply
don't change the protector.
But indeed, there are other more complicated paths that could be covered, let's
move incrementally.

[Dan Ehrenberg, 2016-02-22 20:36:41]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2016-02-22 20:36:41]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/1689733002/#ps220001
(title: "Remove blank lines")

[commit-bot: I haz the power, 2016-02-22 20:36:48]

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

[commit-bot: I haz the power, 2016-02-22 20:39:31]

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

[commit-bot: I haz the power, 2016-02-22 20:39:32]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11327)

[Dan Ehrenberg, 2016-02-22 20:43:18]

Description was changed from

==========
Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Array subclasses will incur the slowness of looking
up @@species, but their use won't slow down invocations of, for example,
Array.prototype.slice on Array base class instances.

Possible future optimizations:
- For the fallback case where the assumptions don't hold, optimize the two
  property lookups.
- For Array.prototype.slice and Array.prototype.splice, even if the full lookup
  of @@species needs to take place, we still could take the rest of the C++
  fastpath. However, to do this correctly requires changing the calling
convention
  from C++ to JS to pass the @@species out, so it is not attempted in this
patch.

With this patch, microbenchmarks of Array.prototype.slice do not suffer a 
noticeable performance regression, unlike their previous 2.5x penalty.
==========

to

==========
Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Array subclasses will incur the slowness of looking
up @@species, but their use won't slow down invocations of, for example,
Array.prototype.slice on Array base class instances.

Possible future optimizations:
- For the fallback case where the assumptions don't hold, optimize the two
  property lookups.
- For Array.prototype.slice and Array.prototype.splice, even if the full lookup
  of @@species needs to take place, we still could take the rest of the C++
  fastpath. However, to do this correctly requires changing the calling
convention
  from C++ to JS to pass the @@species out, so it is not attempted in this
patch.

With this patch, microbenchmarks of Array.prototype.slice do not suffer a 
noticeable performance regression, unlike their previous 2.5x penalty.

TBR=hpayer@chromium.org
==========

[Dan Ehrenberg, 2016-02-22 20:43:21]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2016-02-22 20:43:30]

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

[commit-bot: I haz the power, 2016-02-22 21:01:36]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-02-22 21:02:57]

Description was changed from

==========
Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Array subclasses will incur the slowness of looking
up @@species, but their use won't slow down invocations of, for example,
Array.prototype.slice on Array base class instances.

Possible future optimizations:
- For the fallback case where the assumptions don't hold, optimize the two
  property lookups.
- For Array.prototype.slice and Array.prototype.splice, even if the full lookup
  of @@species needs to take place, we still could take the rest of the C++
  fastpath. However, to do this correctly requires changing the calling
convention
  from C++ to JS to pass the @@species out, so it is not attempted in this
patch.

With this patch, microbenchmarks of Array.prototype.slice do not suffer a 
noticeable performance regression, unlike their previous 2.5x penalty.

TBR=hpayer@chromium.org
==========

to

==========
Optimize @@species based on a global 'protector' cell

This patch makes ArraySpeciesCreate fast in V8 by avoiding two property reads
when the following conditions are met:
- No Array instance has had its __proto__ reset
- No Array instance has had a constructor property defined
- Array.prototype has not had its constructor changed
- Array[Symbol.species] has not been reset

For subclasses of Array, or for conditions where one of these assumptions is
violated, the full lookup of species is done according to the ArraySpeciesCreate
algorithm. Although this is a "performance cliff", it does not come up in the
expected typical use case of @@species (Array subclassing), so it is hoped that
this can form a good start. Array subclasses will incur the slowness of looking
up @@species, but their use won't slow down invocations of, for example,
Array.prototype.slice on Array base class instances.

Possible future optimizations:
- For the fallback case where the assumptions don't hold, optimize the two
  property lookups.
- For Array.prototype.slice and Array.prototype.splice, even if the full lookup
  of @@species needs to take place, we still could take the rest of the C++
  fastpath. However, to do this correctly requires changing the calling
convention
  from C++ to JS to pass the @@species out, so it is not attempted in this
patch.

With this patch, microbenchmarks of Array.prototype.slice do not suffer a
noticeable performance regression, unlike their previous 2.5x penalty.

TBR=hpayer@chromium.org

Committed: https://crrev.com/7033ae511f3cca7e5464353e80acbba645e07c44
Cr-Commit-Position: refs/heads/master@{#34199}
==========

[commit-bot: I haz the power, 2016-02-22 21:02:58]

Patchset 12 (id:??) landed as
https://crrev.com/7033ae511f3cca7e5464353e80acbba645e07c44
Cr-Commit-Position: refs/heads/master@{#34199}

[Toon Verwaest, 2016-02-29 18:12:23]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2016-02-29 18:12:24]

This doesn't seem to handle redefining .constructor as accessor. Also, this
slows down Object.assign by around 5%.