Fastpath some spread-call desugaring.

Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.5x slower than baseline es5 implementation, compared to 19x slower previously.

BUG=v8:5511
Committed: https://crrev.com/a63eeb485a2667e9d8d0660969baeb1f242d0979
Cr-Commit-Position: refs/heads/master@{#40998}

[petermarshall, 2016-11-04 10:19:28]

Description was changed from

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

BUG=v8:5511
==========

to

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

BUG=v8:5511
==========

[petermarshall, 2016-11-04 10:19:42]

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

[commit-bot: I haz the power, 2016-11-04 10:19:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 10:39:02]

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

[commit-bot: I haz the power, 2016-11-04 10:39:03]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11937)
  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...)

[petermarshall, 2016-11-07 15:41:03]

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

[commit-bot: I haz the power, 2016-11-07 15:41:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 15:45:23]

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

[commit-bot: I haz the power, 2016-11-07 15:45:24]

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

[petermarshall, 2016-11-07 16:45:38]

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

[commit-bot: I haz the power, 2016-11-07 16:45:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 16:47:50]

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

[commit-bot: I haz the power, 2016-11-07 16:47:51]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-11-08 08:53:46]

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

[commit-bot: I haz the power, 2016-11-08 08:53:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[petermarshall, 2016-11-08 08:57:52]

Description was changed from

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

BUG=v8:5511
==========

to

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 3x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

[commit-bot: I haz the power, 2016-11-08 09:20:37]

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

[commit-bot: I haz the power, 2016-11-08 09:20:38]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-11-08 10:00:53]

petermarshall@chromium.org changed reviewers:
+ bmeurer@chromium.org, yangguo@chromium.org

[petermarshall, 2016-11-08 10:00:53]

PTAL :). WASM sanitizer failures are being fixed in
https://codereview.chromium.org/2476643003

[Yang, 2016-11-08 14:08:41]

https://codereview.chromium.org/2465253011/diff/60001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2465253011/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:2439: Builtins::kSpreadIterablePrepare, 1, false);
Why do we need this? I think what you want is to call a runtime function. See
src/runtime/runtime.h

https://codereview.chromium.org/2465253011/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:3502: native_context()->set_initial_array_iterator(
Isn't this the same thing as native_context()->array_values_iterator()?

https://codereview.chromium.org/2465253011/diff/60001/src/builtins/builtins-s...
File src/builtins/builtins-spread.cc (right):

https://codereview.chromium.org/2465253011/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-spread.cc:26: if (Object::GetProperty(spread,
it_name).ToHandle(&iterator_func)) {
Does this violate the spec? GetProperty may be observable (through Javascript
accessors for example). So if we figure out that the iterator is not the
original one, we load it once here, and later in spread_iterable_function, we
load it again in the for-of.

We could use GetDataProperty here, but I think we might be able to use the
array_protector cell here (see builtins/builtins-array.cc and look for
array_protector)

And please add test cases for this kind of thing.

[petermarshall, 2016-11-09 10:40:59]

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

[commit-bot: I haz the power, 2016-11-09 10:41:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 10:55:50]

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

[commit-bot: I haz the power, 2016-11-09 10:55:51]

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_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[petermarshall, 2016-11-09 13:39:10]

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

[commit-bot: I haz the power, 2016-11-09 13:39:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 14:07:52]

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

[commit-bot: I haz the power, 2016-11-09 14:07:53]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-11-10 09:35:35]

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

[commit-bot: I haz the power, 2016-11-10 09:35:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[petermarshall, 2016-11-10 09:36:19]

Description was changed from

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 3x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

to

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.25x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

[petermarshall, 2016-11-10 09:40:13]

The CQ bit was unchecked by petermarshall@chromium.org

[petermarshall, 2016-11-10 11:37:54]

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

[commit-bot: I haz the power, 2016-11-10 11:37:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[petermarshall, 2016-11-10 11:41:01]

Description was changed from

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.25x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

to

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.5x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

[commit-bot: I haz the power, 2016-11-10 12:06:07]

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

[commit-bot: I haz the power, 2016-11-10 12:06:08]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-11-10 12:19:20]

Benedikt could you take a look? I added a protector cell for the array iterator,
pretty much the same as Caitlin has in
https://codereview.chromium.org/2484003002.

https://codereview.chromium.org/2465253011/diff/60001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2465253011/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:2439: Builtins::kSpreadIterablePrepare, 1, false);
On 2016/11/08 at 14:08:41, Yang wrote:
> Why do we need this? I think what you want is to call a runtime function. See
src/runtime/runtime.h

Yep I've moved it to a runtime function

https://codereview.chromium.org/2465253011/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:3502: native_context()->set_initial_array_iterator(
On 2016/11/08 at 14:08:41, Yang wrote:
> Isn't this the same thing as native_context()->array_values_iterator()?

Removed this anyway - no longer needed

https://codereview.chromium.org/2465253011/diff/60001/src/builtins/builtins-s...
File src/builtins/builtins-spread.cc (right):

https://codereview.chromium.org/2465253011/diff/60001/src/builtins/builtins-s...
src/builtins/builtins-spread.cc:26: if (Object::GetProperty(spread,
it_name).ToHandle(&iterator_func)) {
On 2016/11/08 at 14:08:41, Yang wrote:
> Does this violate the spec? GetProperty may be observable (through Javascript
accessors for example). So if we figure out that the iterator is not the
original one, we load it once here, and later in spread_iterable_function, we
load it again in the for-of.
> 
> We could use GetDataProperty here, but I think we might be able to use the
array_protector cell here (see builtins/builtins-array.cc and look for
array_protector)
> 
> And please add test cases for this kind of thing.

Yeah it does violate the spec. I added a test case for this and for a few other
things that could potentially go wrong. The array_protector doesn't currently
help us here, but I added a protector cell for the array iterator.

[Benedikt Meurer, 2016-11-10 12:32:58]

bmeurer@chromium.org changed reviewers:
+ caitp@chromium.org

[Benedikt Meurer, 2016-11-10 12:32:59]

Fist comment (as per offline discussion) :-)

We need to either rename this protector or the one introduced by Caitlin.

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

https://codereview.chromium.org/2465253011/diff/140001/src/lookup.cc#newcode202
src/lookup.cc:202: } else if (*name_ == heap()->next_string()) {
As discussed offline, please check against the initial map of the
%ArrayIterator%.prototype instead.

[petermarshall, 2016-11-11 09:14:33]

Description was changed from

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.5x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

to

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.4x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

[petermarshall, 2016-11-11 09:15:28]

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

[commit-bot: I haz the power, 2016-11-11 09:15:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[petermarshall, 2016-11-11 09:18:14]

The CQ bit was unchecked by petermarshall@chromium.org

[petermarshall, 2016-11-11 09:19:32]

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

[commit-bot: I haz the power, 2016-11-11 09:19:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 09:33:07]

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

[commit-bot: I haz the power, 2016-11-11 09:33:08]

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_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[petermarshall, 2016-11-11 10:00:51]

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

[commit-bot: I haz the power, 2016-11-11 10:00:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 10:26:29]

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

[commit-bot: I haz the power, 2016-11-11 10:26:30]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-11-11 10:34:23]

Now checks the map of %ArrayIterator%.prototype against the initial one. I think
we properly handle any observable behaviour now, but possible we still missed
something.

Still need to deal with naming/concept behind array iterator protector - not
sure to what extent this part overlaps with Caitlin's CL, or maybe they are
completely different.
PTAL :)

[caitp, 2016-11-11 16:19:16]

caitp@igalia.com changed reviewers:
+ caitp@igalia.com

[caitp, 2016-11-11 16:19:17]

https://codereview.chromium.org/2465253011/diff/200001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/2465253011/diff/200001/src/lookup.cc#newcode201
src/lookup.cc:201: }
this is a very different case from the version in my CL. We will need to rename
one of them. The ArrayIterator inlining should not be broken by this.

[Yang, 2016-11-14 10:16:56]

On 2016/11/11 16:19:17, caitp wrote:
> https://codereview.chromium.org/2465253011/diff/200001/src/lookup.cc
> File src/lookup.cc (right):
> 
>
https://codereview.chromium.org/2465253011/diff/200001/src/lookup.cc#newcode201
> src/lookup.cc:201: }
> this is a very different case from the version in my CL. We will need to
rename
> one of them. The ArrayIterator inlining should not be broken by this.

As discussed offline, please check whether overwriting the prototype of the
spreadable will be caught by these fastpath checks.

[Yang, 2016-11-14 10:17:20]

https://codereview.chromium.org/2465253011/diff/200001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/2465253011/diff/200001/src/heap/heap.cc#newco...
src/heap/heap.cc:2844: handle(Smi::FromInt(Isolate::kArrayProtectorValid),
isolate()));
Can we name kArrayProtectorValid to kProtectorValid?

https://codereview.chromium.org/2465253011/diff/200001/src/lookup.h
File src/lookup.h (right):

https://codereview.chromium.org/2465253011/diff/200001/src/lookup.h#newcode262
src/lookup.h:262: *name_ == heap()->iterator_symbol()) {
If this is a performance concern (which I doubt), we could compare *name_ to
constructor_string first, and then check for IsSymbol before following up with
these symbol compares.

[petermarshall, 2016-11-14 12:11:42]

Description was changed from

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.4x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

to

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.5x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

[petermarshall, 2016-11-14 12:13:26]

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

[commit-bot: I haz the power, 2016-11-14 12:13:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[petermarshall, 2016-11-14 12:32:49]

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

[commit-bot: I haz the power, 2016-11-14 12:32:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[petermarshall, 2016-11-14 12:52:50]

Ok we now check if the array has the original prototype that we actually spent
so much effort to verify wasn't changed. Did the renaming for
kArrayProtectorValid which added a fair few files with 1 line changes.

PTAL :)

https://codereview.chromium.org/2465253011/diff/200001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/2465253011/diff/200001/src/heap/heap.cc#newco...
src/heap/heap.cc:2844: handle(Smi::FromInt(Isolate::kArrayProtectorValid),
isolate()));
On 2016/11/14 at 10:17:19, Yang wrote:
> Can we name kArrayProtectorValid to kProtectorValid?

Sounds good to me.

https://codereview.chromium.org/2465253011/diff/200001/src/lookup.h
File src/lookup.h (right):

https://codereview.chromium.org/2465253011/diff/200001/src/lookup.h#newcode262
src/lookup.h:262: *name_ == heap()->iterator_symbol()) {
On 2016/11/14 at 10:17:19, Yang wrote:
> If this is a performance concern (which I doubt), we could compare *name_ to
constructor_string first, and then check for IsSymbol before following up with
these symbol compares.

OK good idea - let's leave it and see if we have any performance issues?

[Yang, 2016-11-14 12:59:01]

https://codereview.chromium.org/2465253011/diff/240001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/2465253011/diff/240001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:654: Object* original_proto =
*isolate->initial_array_prototype();
this unnecessarily wraps the context field into a handle, just to dereference
it. Can we just use isolate->is_initial_array_prototype(current_proto) instead?

[Yang, 2016-11-14 12:59:13]

On 2016/11/14 12:59:01, Yang wrote:
>
https://codereview.chromium.org/2465253011/diff/240001/src/runtime/runtime-ar...
> File src/runtime/runtime-array.cc (right):
> 
>
https://codereview.chromium.org/2465253011/diff/240001/src/runtime/runtime-ar...
> src/runtime/runtime-array.cc:654: Object* original_proto =
> *isolate->initial_array_prototype();
> this unnecessarily wraps the context field into a handle, just to dereference
> it. Can we just use isolate->is_initial_array_prototype(current_proto)
instead?

lgtm otherwise.

[commit-bot: I haz the power, 2016-11-14 13:02:18]

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

[commit-bot: I haz the power, 2016-11-14 13:02:19]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-11-14 13:37:19]

Updated for Yang's comment

https://codereview.chromium.org/2465253011/diff/240001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/2465253011/diff/240001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:654: Object* original_proto =
*isolate->initial_array_prototype();
On 2016/11/14 at 12:59:01, Yang wrote:
> this unnecessarily wraps the context field into a handle, just to dereference
it. Can we just use isolate->is_initial_array_prototype(current_proto) instead?

That is much nicer, and we can do it for checking the %ArrayIteratorPrototype%'s
map too.

[Yang, 2016-11-14 13:43:48]

On 2016/11/14 13:37:19, petermarshall wrote:
> Updated for Yang's comment
> 
>
https://codereview.chromium.org/2465253011/diff/240001/src/runtime/runtime-ar...
> File src/runtime/runtime-array.cc (right):
> 
>
https://codereview.chromium.org/2465253011/diff/240001/src/runtime/runtime-ar...
> src/runtime/runtime-array.cc:654: Object* original_proto =
> *isolate->initial_array_prototype();
> On 2016/11/14 at 12:59:01, Yang wrote:
> > this unnecessarily wraps the context field into a handle, just to
dereference
> it. Can we just use isolate->is_initial_array_prototype(current_proto)
instead?
> 
> That is much nicer, and we can do it for checking the
%ArrayIteratorPrototype%'s
> map too.

lgtm!

[petermarshall, 2016-11-14 13:44:13]

Benedikt, you mentioned a few patches ago that there might be an issue with
holey array types - does that still stand?

[Benedikt Meurer, 2016-11-14 18:58:24]

please rebase on caitlins CL and address comment with test case.

https://codereview.chromium.org/2465253011/diff/260001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/2465253011/diff/260001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:649: if (IsFastElementsKind(array_kind) &&
I don't think this is correct for holey arrays. They could actually have a funny
getter on the prototype, that messes with %ArrayIterator%.prototype.next. I
think for holey arrays we need to check the array lookup protector as well.

[petermarshall, 2016-11-15 09:14:41]

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

[commit-bot: I haz the power, 2016-11-15 09:14:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 09:42:58]

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

[commit-bot: I haz the power, 2016-11-15 09:42:59]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-11-15 10:14:33]

I added a test for holey arrays with a getter on the prototype
(testArrayPrototypeHasHoleGetter).

The flow through the code is that SpreadIterablePrepare takes the fast path,
then Builtins::Apply checks the elements type and calls into
CreateListFromArrayLike if it finds anything that isn't FAST_SMI_ELEMENTS or
FAST_ELEMENTS. CreateListFromArrayLike calls GetElement for each element, so the
behaviour here is already correct.

Do you see any other case where holey arrays could break this?

[Benedikt Meurer, 2016-11-15 10:16:17]

The breakage happens when the element access modifies
%ArrayIterator%.prototype.next, which is not covered currently.

[petermarshall, 2016-11-15 10:51:17]

Ahh OK, the GetElement calls in CreateListFromArrayLike are observable but
happen after all of our checks. Updated the test so that it broke the code, and
updated the code to check the regular array protector for holey arrays.

[petermarshall, 2016-11-15 12:19:14]

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

[commit-bot: I haz the power, 2016-11-15 12:19:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 12:20:47]

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

[commit-bot: I haz the power, 2016-11-15 12:20:50]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  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_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/16194)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[petermarshall, 2016-11-15 12:35:47]

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

[commit-bot: I haz the power, 2016-11-15 12:35:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[petermarshall, 2016-11-15 12:36:44]

The CQ bit was unchecked by petermarshall@chromium.org

[petermarshall, 2016-11-15 12:40:59]

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

[commit-bot: I haz the power, 2016-11-15 12:41:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 13:08:10]

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

[commit-bot: I haz the power, 2016-11-15 13:08:12]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-11-15 13:12:03]

lgtm with comment

https://codereview.chromium.org/2465253011/diff/360001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/2465253011/diff/360001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:661: if (array_kind == FAST_SMI_ELEMENTS ||
array_kind == FAST_ELEMENTS) {
Can we also allow FAST_DOUBLE_ELEMENTS? (test case :)
Then we could also use IsFastPackedElementsKind for this check.

https://codereview.chromium.org/2465253011/diff/360001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:665: array_kind == FAST_HOLEY_ELEMENTS) &&
Same here.

[Benedikt Meurer, 2016-11-15 13:13:46]

LGTM once comments addressed.

https://codereview.chromium.org/2465253011/diff/360001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/2465253011/diff/360001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:661: if (array_kind == FAST_SMI_ELEMENTS ||
array_kind == FAST_ELEMENTS) {
Yes, please.

[petermarshall, 2016-11-15 14:12:47]

The CQ bit was checked by petermarshall@chromium.org

[petermarshall, 2016-11-15 14:12:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2465253011/#ps380001
(title: "Handles double arrays too")

[commit-bot: I haz the power, 2016-11-15 14:13:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 14:17:10]

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

[commit-bot: I haz the power, 2016-11-15 14:17:13]

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

[petermarshall, 2016-11-15 14:24:48]

petermarshall@chromium.org changed reviewers:
+ vogelheim@chromium.org

[petermarshall, 2016-11-15 14:24:49]

Daniel could you PTAL as parsing.cc owner

https://codereview.chromium.org/2465253011/diff/360001/src/runtime/runtime-ar...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/2465253011/diff/360001/src/runtime/runtime-ar...
src/runtime/runtime-array.cc:661: if (array_kind == FAST_SMI_ELEMENTS ||
array_kind == FAST_ELEMENTS) {
On 2016/11/15 at 13:13:46, Benedikt Meurer wrote:
> Yes, please.

OK, done

[vogelheim, 2016-11-15 14:26:36]

lgtm for src/parsing/...

[petermarshall, 2016-11-15 14:28:39]

petermarshall@chromium.org changed reviewers:
+ ulan@chromium.org

[petermarshall, 2016-11-15 14:28:41]

Ulan, could you PTAL as src/heap/* owner?

[ulan, 2016-11-15 14:34:01]

heap lgtm

[petermarshall, 2016-11-15 14:34:55]

The CQ bit was checked by petermarshall@chromium.org

[commit-bot: I haz the power, 2016-11-15 14:35:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 14:41:36]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-11-17 22:34:22]

Description was changed from

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.5x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511
==========

to

==========
Fastpath some spread-call desugaring.

Avoid using the iterator for arrays with fast elements where the iterator has
not been modified.

Only deals with the case where there is a single spread argument.

Improves the six-speed "spread" benchmark to 1.5x slower than baseline es5
implementation, compared to 19x slower previously.

BUG=v8:5511

Committed: https://crrev.com/a63eeb485a2667e9d8d0660969baeb1f242d0979
Cr-Commit-Position: refs/heads/master@{#40998}
==========

[commit-bot: I haz the power, 2016-11-17 22:34:24]

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