[builtins] implement JSBuiltinReducer for ArrayIteratorNext()

[builtins] implement JSBuiltinReducer for ArrayIteratorNext()

Adds a protector cell to prevent inlining (which will likely lead to deopt
loops) when a JSArrayIterator's array transitions from a fast JSArray to a
slow JSArray (such as, when the array is touched during iteration in a way
which triggers a map transition).

Also adds TODO comments relating to the spec update proposed by Dan at
https://github.com/tc39/ecma262/pull/724

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
TBR=hpayer@chromium.org, ulan@chromium.org

Committed: https://crrev.com/7f21e67b38a859c5503777a606d68ef3cb449517
Cr-Commit-Position: refs/heads/master@{#40970}

[caitp, 2016-11-07 22:40:47]

Remove whitespace change that snuck in

[caitp, 2016-11-08 00:01:10]

On 2016/11/07 22:40:47, caitp wrote:
> Remove whitespace change that snuck in

This isn't quite ready for review yet. `TheHole` is sneaking into the
JSArrayIterator object (but not during creation) at certain points, and I'm
trying to figure out why that can happen.

[caitp, 2016-11-08 05:26:06]

On 2016/11/08 00:01:10, caitp wrote:
> On 2016/11/07 22:40:47, caitp wrote:
> > Remove whitespace change that snuck in
> 
> This isn't quite ready for review yet. `TheHole` is sneaking into the
> JSArrayIterator object (but not during creation) at certain points, and I'm
> trying to figure out why that can happen.

On 2016/11/08 00:01:10, caitp wrote:
> On 2016/11/07 22:40:47, caitp wrote:
> > Remove whitespace change that snuck in
> 
> This isn't quite ready for review yet. `TheHole` is sneaking into the
> JSArrayIterator object (but not during creation) at certain points, and I'm
> trying to figure out why that can happen.

Pretty sure it's related to this in the schedule:

```
--- BLOCK B9 <- B7 ---
  780: IfTrue(781)
  779: LoadField[tagged base, 40, OtherInternal, kRepTaggedPointer|kTypeAny,
PointerWriteBarrier](35, 785, 40)
  778: CheckMaps[1](785, 779, 785, 40)
  776: StoreField[tagged base, 24, Range(0, 134217726),
kRepTaggedSigned|kTypeInt32, NoWriteBarrier](35, 822, 778, 780)
```

(where #822 is a heap constant of `the hole`), so I guess that gives me a place
to look

[Benedikt Meurer, 2016-11-08 05:29:47]

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

[Benedikt Meurer, 2016-11-08 05:29:49]

Thanks a lot. First round of comments.

https://codereview.chromium.org/2484003002/diff/40001/src/builtins/builtins-a...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2484003002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-array.cc:2290: invalidate_protector(assembler);
The invalidate_protector label is unused.

https://codereview.chromium.org/2484003002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-array.cc:2305: Label if_wasfastarray(assembler),
if_wasnotfastarray(assembler);
The if_wasnotfastarray label is unused.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:568: TypeCache::Get().kPositiveSafeInteger,
Actually it can be any positive integer, not only the safe integer range.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:569: MachineType::TaggedSigned(),
The general type is MachineType::AnyTagged()

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:574: access.write_barrier_kind = kNoWriteBarrier;
Set access.machine_type = MachineType::TaggedSigned() here.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:577: access.write_barrier_kind = kNoWriteBarrier;
Set access.machine_type = MachineType::TaggedSigned() here.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:582: access.type =
TypeCache::Get().kJSTypedArrayLengthType;
Set access.machine_type = MachineType::TaggedSigned() here.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:228: }
Add a break for the default

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:257: value, jsgraph()->SmiConstant(0),
effect, control);
Nit: use jsgraph()->ZeroConstant() here.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:288:
simplified()->LoadField(AccessBuilder::ForJSArrayIteratorObject()),
Move this after the condition below, otherwise you create a dangling LoadField
in the effect chain if the condition doesn't hold and you return NoChange.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:293: if
(!GetMapWitness(array).ToHandle(&array_map)) {
This won't work in general, as there's no guarantee that you visit the array
first, plus there's LoadElimination happening at this point which would fold the
LoadField away and present you with a CheckMaps on the actual object. So for
holey elements you'll always abort here.

Instead if you see holey elements_kind here, you must abort if you the array
protector is invalid, and if it's valid, add a code dependency on it, and just
convert all holes to undefined. Plus during construction of the iterator you
must check that the prototype of fast holey arrays is the Array.prototype, and
use the generic iterator for holey arrays with non-Array.prototype.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:300:
javascript()->StrictEqual(CompareOperationHint::kNone), array,
Use simplified()->ReferenceEqual() instead of StrictEqual here.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:339: iterator, effect, control);
s/effect/etrue1

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:341: effect, control);
s/effect/etrue1/

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:344: Node* next_index =
graph()->NewNode(simplified()->NumberAdd(), index,
You need to truncate the next_index via NumberToUint32 to teach TurboFan that
this cannot overflow (for general JSArray, the input types don't provide this
information, as the index can be any Unsigned32), otherwise TurboFan will be
forced to pick Float64Add here for the general JSArray case.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:361: // the hole to undefined if possible, or
deoptimizing otherwise.
You cannot deoptimize for hole here, as that'll be a deoptimization loop. See
above.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:380: if (CanTreatHoleAsUndefined(array_map))
{
Same as above.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
File src/compiler/js-builtin-reducer.h (right):

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.h:27: class MachineOperatorBuilder;
Remove this unused forward declaration.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.h:110: bool CanTreatHoleAsUndefined(Handle<Map>
receiver_map);
These helpers are unnecessary, see comments in .cc file.

[caitp, 2016-11-08 06:25:24]

Move the [[IteratedObject]] field load after ensuring code can be optimized

[caitp, 2016-11-08 06:27:59]

I think I've addressed all of those first round comments, and fixed the bug that
was causing a crash earlier. This will probably pass the current test suite now,
but I'd like to add some coverage to make sure this does the right thing in a
variety of situations.

Tomorrow I'll add the TypedArray versions of ReduceArrayIterator() and try to
add some good test coverage to make sure all of the bailouts work as expected.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/js-builtin...
src/compiler/js-builtin-reducer.cc:293: if
(!GetMapWitness(array).ToHandle(&array_map)) {
On 2016/11/08 05:29:48, Benedikt Meurer wrote:
> This won't work in general, as there's no guarantee that you visit the array
> first, plus there's LoadElimination happening at this point which would fold
the
> LoadField away and present you with a CheckMaps on the actual object. So for
> holey elements you'll always abort here.
> 
> Instead if you see holey elements_kind here, you must abort if you the array
> protector is invalid, and if it's valid, add a code dependency on it, and just
> convert all holes to undefined. Plus during construction of the iterator you
> must check that the prototype of fast holey arrays is the Array.prototype, and
> use the generic iterator for holey arrays with non-Array.prototype.

Okay, I've updated this to set up a generic iterator if the prototype isn't
something that the protector cell can track (and gotten rid of the
CanTreatHoleAsUndefiend() helpers copied from
js-native-context-specialization.cc)

[caitp, 2016-11-08 06:29:12]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 06:29:18]

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-08 06:33:48]

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

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

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

[caitp, 2016-11-08 06:37:36]

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:568: TypeCache::Get().kPositiveSafeInteger,
On 2016/11/08 05:29:48, Benedikt Meurer wrote:
> Actually it can be any positive integer, not only the safe integer range.

I think `ToLength(<array length>)` should restrict the length to max safe
integer, doesn't it? Since the [[NextIndex]] field should never exceed the
highest possible length (which for generic arrays can be up to 2^53-1)

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:569: MachineType::TaggedSigned(),
On 2016/11/08 05:29:48, Benedikt Meurer wrote:
> The general type is MachineType::AnyTagged()

Acknowledged.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:574: access.write_barrier_kind = kNoWriteBarrier;
On 2016/11/08 05:29:48, Benedikt Meurer wrote:
> Set access.machine_type = MachineType::TaggedSigned() here.

Acknowledged.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:577: access.write_barrier_kind = kNoWriteBarrier;
On 2016/11/08 05:29:48, Benedikt Meurer wrote:
> Set access.machine_type = MachineType::TaggedSigned() here.

Acknowledged.

https://codereview.chromium.org/2484003002/diff/40001/src/compiler/access-bui...
src/compiler/access-builder.cc:582: access.type =
TypeCache::Get().kJSTypedArrayLengthType;
On 2016/11/08 05:29:48, Benedikt Meurer wrote:
> Set access.machine_type = MachineType::TaggedSigned() here.

Acknowledged.

[caitp, 2016-11-08 06:44:42]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 06:44:48]

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-08 06:46:58]

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

[commit-bot: I haz the power, 2016-11-08 06:46:59]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15779)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12073)

[caitp, 2016-11-08 06:50:54]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

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

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-08 07:21:01]

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

[commit-bot: I haz the power, 2016-11-08 07:21:02]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-11-08 08:30:00]

Looking good, thanks. Next round of comments.

https://codereview.chromium.org/2484003002/diff/160001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2484003002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2304: Label if_wasfastarray(assembler),
if_wasnotfastarray(assembler);
Label if_wasnotfastarray still unused.

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/access-bu...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/access-bu...
src/compiler/access-builder.cc:556: Type::Receiver(),
Since you store undefined when done, this type must be ReceiverOrUndefined

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:200: if
(IsFastElementsKind(receiver_map->elements_kind()) &&
For holey arrays you first need to check whether all prototype maps are stable
(i.e. the Array.prototype and the Object.prototype map) and the array protector
is valid.

You to change the baseline version of this as well, so that it checks for fast
holey whether

1. prototype is initial array prototype, and
2. array protector is intact,

and only then creates a fast iterator.

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:439: DCHECK(kind == IterationKind::kValues);
Please use DCHECK_EQ here (and add operator<< overload for the IterationKind).

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:575: jsgraph()->OneConstant());
Here you also need to do the NumberToUint32 truncation, for the same reason as
above.

https://codereview.chromium.org/2484003002/diff/160001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/2484003002/diff/160001/src/heap/heap.h#newcod...
src/heap/heap.h:169: V(PropertyCell, array_iterator_protector,
ArrayIteratorProtector)            \
Since you don't add code dependencies on this protector, a simple Cell is
sufficient.

[caitp, 2016-11-08 17:47:49]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 17:47:51]

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

[caitp, 2016-11-08 17:48:36]

Description was changed from

==========
[builtins] implement JSBuiltinReducer for ArrayIteratorNext()

Adds a protector cell to prevent inlining (which will likely lead to deopt
loops) when a JSArrayIterator's array transitions from a fast JSArray to a
slow JSArray (such as, when the array is touched during iteration in a way
which triggers a map transition).

Also adds TODO comments relating to the spec update proposed by Dan at
https://github.com/tc39/ecma262/pull/724

BUG=v8:5388
R=bmeurer@chromium.org
==========

to

==========
[builtins] implement JSBuiltinReducer for ArrayIteratorNext()

Adds a protector cell to prevent inlining (which will likely lead to deopt
loops) when a JSArrayIterator's array transitions from a fast JSArray to a
slow JSArray (such as, when the array is touched during iteration in a way
which triggers a map transition).

Also adds TODO comments relating to the spec update proposed by Dan at
https://github.com/tc39/ecma262/pull/724

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
TBR=hpayer@chromium.org, ulan@chromium.org
==========

[commit-bot: I haz the power, 2016-11-08 17:52:02]

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

[commit-bot: I haz the power, 2016-11-08 17:52:03]

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

[caitp, 2016-11-08 17:53:07]

next round up, ptal when you get some time (try jobs are failing, so taking a
look at those now)

https://codereview.chromium.org/2484003002/diff/160001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2484003002/diff/160001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2304: Label if_wasfastarray(assembler),
if_wasnotfastarray(assembler);
On 2016/11/08 08:30:00, Benedikt Meurer wrote:
> Label if_wasnotfastarray still unused.

it's toast

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/access-bu...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/access-bu...
src/compiler/access-builder.cc:556: Type::Receiver(),
On 2016/11/08 08:30:00, Benedikt Meurer wrote:
> Since you store undefined when done, this type must be ReceiverOrUndefined

I had originally added a ReceiverOrUndefined definition to types.h, since one
didn't seem to exist yet. I guess I'll add it back

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:200: if
(IsFastElementsKind(receiver_map->elements_kind()) &&
On 2016/11/08 08:30:00, Benedikt Meurer wrote:
> For holey arrays you first need to check whether all prototype maps are stable
> (i.e. the Array.prototype and the Object.prototype map) and the array
protector
> is valid.
> 
> You to change the baseline version of this as well, so that it checks for fast
> holey whether
> 
> 1. prototype is initial array prototype, and
> 2. array protector is intact,
> 
> and only then creates a fast iterator.

I did some creative stuff to have (I think) exactly the same behaviour between
the baseline and optimized versions re: holey fast JSArrays, but it could be
simplified a bit, I guess

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:439: DCHECK(kind == IterationKind::kValues);
On 2016/11/08 08:30:00, Benedikt Meurer wrote:
> Please use DCHECK_EQ here (and add operator<< overload for the IterationKind).

will do that for the next one

https://codereview.chromium.org/2484003002/diff/160001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:575: jsgraph()->OneConstant());
On 2016/11/08 08:30:00, Benedikt Meurer wrote:
> Here you also need to do the NumberToUint32 truncation, for the same reason as
> above.

added truncation for both cases

https://codereview.chromium.org/2484003002/diff/160001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/2484003002/diff/160001/src/heap/heap.h#newcod...
src/heap/heap.h:169: V(PropertyCell, array_iterator_protector,
ArrayIteratorProtector)            \
On 2016/11/08 08:30:00, Benedikt Meurer wrote:
> Since you don't add code dependencies on this protector, a simple Cell is
> sufficient.

done (got rid of the runtime call, since nothing gets deopt-ed when it changes)

[caitp, 2016-11-08 18:05:33]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 18:05:38]

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-08 18:33:28]

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

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

Dry run: This issue passed the CQ dry run.

[caitp, 2016-11-08 19:51:28]

CheckIf() for ArrayBufferWasNeutered() rather than a branch, which hopefully can
be eliminated, and serialize IterationKind for DCHECK_EQs... inline the
%TypedArray%.prototype methods too

[caitp, 2016-11-08 19:51:33]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 19:51:45]

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-08 20:50:03]

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

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

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-11-09 05:17:18]

On 2016/11/08 19:51:28, caitp wrote:
> CheckIf() for ArrayBufferWasNeutered() rather than a branch, which hopefully
can
> be eliminated, and serialize IterationKind for DCHECK_EQs... inline the
> %TypedArray%.prototype methods too

You can only use CheckIf, if there's a way to learn, so you don't constantly
deopt on neutered typed arrays (sounds unlikely I know, but we don't want to
risk going down Crankshafts road again...)

[Benedikt Meurer, 2016-11-09 05:37:04]

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

[Benedikt Meurer, 2016-11-09 05:37:05]

Camillo please take a look at the holey array iteration. Looks good to me, but I
really need a second reviewer for this who knows this shit.

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
src/code-stub-assembler.cc:8729: // Otherwise, check that the prototype is a
stable initial
Maybe it'd be sufficient to check for current contexts Array.prototype and
Object.prototype, but I don't have a strong opinion on this. You don't need the
stability checks on the maps tho, that's only required for optimized code, so
that we can use the code dependency mechanism.

Also can you add an explanation of why we have to do this dance?

https://codereview.chromium.org/2484003002/diff/210001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/210001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:188: if
(IsFastHoleyElementsKind(receiver_map->elements_kind())) return true;
Return true if non-holey.

[caitp, 2016-11-09 06:20:49]

On 2016/11/09 05:17:18, Benedikt Meurer wrote:
> On 2016/11/08 19:51:28, caitp wrote:
> > CheckIf() for ArrayBufferWasNeutered() rather than a branch, which hopefully
> can
> > be eliminated, and serialize IterationKind for DCHECK_EQs... inline the
> > %TypedArray%.prototype methods too
> 
> You can only use CheckIf, if there's a way to learn, so you don't constantly
> deopt on neutered typed arrays (sounds unlikely I know, but we don't want to
> risk going down Crankshafts road again...)

It does sound extremely unlikely to me, which was how I rationalized it, and I
thought the performance implications of the deopt loop might be a flag to
developers to fix broken code.

So, we are sure we want to re-add the branching? Ok

[Benedikt Meurer, 2016-11-09 06:22:46]

On 2016/11/09 06:20:49, caitp wrote:
> On 2016/11/09 05:17:18, Benedikt Meurer wrote:
> > On 2016/11/08 19:51:28, caitp wrote:
> > > CheckIf() for ArrayBufferWasNeutered() rather than a branch, which
hopefully
> > can
> > > be eliminated, and serialize IterationKind for DCHECK_EQs... inline the
> > > %TypedArray%.prototype methods too
> > 
> > You can only use CheckIf, if there's a way to learn, so you don't constantly
> > deopt on neutered typed arrays (sounds unlikely I know, but we don't want to
> > risk going down Crankshafts road again...)
> 
> It does sound extremely unlikely to me, which was how I rationalized it, and I
> thought the performance implications of the deopt loop might be a flag to
> developers to fix broken code.
> 
> So, we are sure we want to re-add the branching? Ok

You are right, it sounds extremely unlikely. Maybe you can at least leave a TODO
there.

[Camillo Bruni, 2016-11-09 12:03:50]

https://codereview.chromium.org/2484003002/diff/210001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2484003002/diff/210001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2322: assembler->WordNotEqual(orig_map,
assembler->UndefinedConstant()),
nit: assembler->IsUndefined(orig_map)

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
src/code-stub-assembler.cc:8717: &if_ispacked, &if_isholey);
Could you add a IsFastPackedElementsKind helper on the CodeStubAssembler with
the corresponding STATIC_ASSERT (same probably for BranchIfFastJSArray)

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
src/code-stub-assembler.cc:8739: prototype =
LoadMapPrototype(LoadMap(prototype));
object_map = LoadMap(prototype); ...

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
src/code-stub-assembler.cc:8742:
DecodeWord32<Map::IsUnstable>(LoadMapBitField3(array_map)),
You probably wanted to check the object_map here.

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
src/code-stub-assembler.cc:8908: Branch(WordEqual(var_context.value(),
UndefinedConstant()), &done, &loop);
nit: IsUndefined(var_context.value())

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
src/code-stub-assembler.cc:8924: Branch(WordEqual(var_context.value(),
UndefinedConstant()), &done, &loop);
nit: IsUndefined(var_context.value())

https://codereview.chromium.org/2484003002/diff/210001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/210001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:643: // Allocate elements for key/value pair
Could you somehow reuse the following snippet, this seems like an awful lot of
duplication :P

https://codereview.chromium.org/2484003002/diff/210001/src/isolate-inl.h
File src/isolate-inl.h (right):

https://codereview.chromium.org/2484003002/diff/210001/src/isolate-inl.h#newc...
src/isolate-inl.h:146: bool Isolate::CanInlineArrayIterator() {
nit: Could you rename this function to be more consistent with the rest?
IsArrayIteratorLookupChainIntact() maybe?

https://codereview.chromium.org/2484003002/diff/210001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2484003002/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2954: void Isolate::InvalidateArrayIteratorProtector() {
You may want to wire this to LookupIterator::InternalUpdateProtector

[caitp, 2016-11-09 17:40:16]

as noted on the design doc, there seems to be a problem at some point in this,
in which after a deopt, it seems to be no longer possible to inline `test`
again. I'm not sure if this is because of the protector cell being flipped at a
point where it shouldn't be (seems possible), or if there's some other factor at
play here and it's nothing to worry about.

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
src/code-stub-assembler.cc:8717: &if_ispacked, &if_isholey);
On 2016/11/09 12:03:49, Camillo Bruni wrote:
> Could you add a IsFastPackedElementsKind helper on the CodeStubAssembler with
> the corresponding STATIC_ASSERT (same probably for BranchIfFastJSArray)

Done, 2 new helpers (CSA::IsFastElementsKind() and
CSA::IsHoleyFastElementsKind())

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
src/code-stub-assembler.cc:8729: // Otherwise, check that the prototype is a
stable initial
On 2016/11/09 05:37:05, Benedikt Meurer wrote:
> Maybe it'd be sufficient to check for current contexts Array.prototype and
> Object.prototype, but I don't have a strong opinion on this. You don't need
the
> stability checks on the maps tho, that's only required for optimized code, so
> that we can use the code dependency mechanism.
> 
> Also can you add an explanation of why we have to do this dance?

Updated to only check the current context, and added a more detailed comment

https://codereview.chromium.org/2484003002/diff/210001/src/code-stub-assemble...
src/code-stub-assembler.cc:8742:
DecodeWord32<Map::IsUnstable>(LoadMapBitField3(array_map)),
On 2016/11/09 12:03:49, Camillo Bruni wrote:
> You probably wanted to check the object_map here.

good catch, but I guess we don't need stability checks for the baseline case

https://codereview.chromium.org/2484003002/diff/210001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/210001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:188: if
(IsFastHoleyElementsKind(receiver_map->elements_kind())) return true;
On 2016/11/09 05:37:05, Benedikt Meurer wrote:
> Return true if non-holey.

oop, good catch.

https://codereview.chromium.org/2484003002/diff/210001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:643: // Allocate elements for key/value pair
On 2016/11/09 12:03:49, Camillo Bruni wrote:
> Could you somehow reuse the following snippet, this seems like an awful lot of
> duplication :P

Ah, I've created an opcode for this to avoid the duplication. Ideally it would
be good to use the existing CreateArray opcode, but I'm not sure I can do that
in a useful way, because the allocation site will always be null and not be
lowered (unless I can work around that somehow), which I think results in it
being changed into a stub call.

In the meantime I've created a new opcode for this, but it would be really nice
to be able to reuse an existing one.

https://codereview.chromium.org/2484003002/diff/210001/src/isolate-inl.h
File src/isolate-inl.h (right):

https://codereview.chromium.org/2484003002/diff/210001/src/isolate-inl.h#newc...
src/isolate-inl.h:146: bool Isolate::CanInlineArrayIterator() {
On 2016/11/09 12:03:49, Camillo Bruni wrote:
> nit: Could you rename this function to be more consistent with the rest?
> IsArrayIteratorLookupChainIntact() maybe?

I'm not sure what a good name for this is, but "LookupChainIntact" seems to be
unrelated to what it's actually tracking. "IsArrayIteratedObjectMapIntact"
maybe?

https://codereview.chromium.org/2484003002/diff/210001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2484003002/diff/210001/src/isolate.cc#newcode...
src/isolate.cc:2954: void Isolate::InvalidateArrayIteratorProtector() {
On 2016/11/09 12:03:49, Camillo Bruni wrote:
> You may want to wire this to LookupIterator::InternalUpdateProtector

this cell isn't changed during lookup, it's changed if a fast array map used by
the array iterator transitions to a slow one --- the iterator implementations
handle it manually. I think it's okay to not wire it up to anything

[caitp, 2016-11-09 18:12:58]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

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

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 18:42:19]

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

[commit-bot: I haz the power, 2016-11-09 18:42:20]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-11-09 20:00:33]

Thanks a lot Caitlin, this is awesome stuff! LGTM from my side once comments
addressed.

Will talk to Dan about the spec change/violation to simplify optimizing the
object handling for the iterated object.

https://codereview.chromium.org/2484003002/diff/230001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2328: // Invalidate array_iterator_protector cell
if needed (Not needed
You can simplify this further and just always write kArrayProtectorInvalid to
the cell. That's probably as cheap as load+check.

https://codereview.chromium.org/2484003002/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2330: assembler->StoreObjectField(
Skip write barrier for this StoreObjectField.

https://codereview.chromium.org/2484003002/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2343: assembler->StoreObjectField(cell,
Cell::kValueOffset, invalid);
Skip write barrier for this StoreObjectField.

https://codereview.chromium.org/2484003002/diff/230001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/code-stub-assemble...
src/code-stub-assembler.cc:683: GotoUnless(IsFastElementsKind(elements_kind),
if_false);
Nice!

https://codereview.chromium.org/2484003002/diff/230001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2484003002/diff/230001/src/code-stub-assemble...
src/code-stub-assembler.h:341: compiler::Node*
IsInAnyNativeContext(compiler::Node* object, uint32_t index);
Drop these dead declarations.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/access-bu...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/access-bu...
src/compiler/access-builder.cc:556: Type::ReceiverOrUndefined(),
Note for later changes/direction: I've been chatting about this with Camillo
offline today, and I think we might want to change this aspect of the spec in
the future (or violate the spec here), as the undefined clearing of the internal
object slot causes a lot of trouble, for example, the load/check elimination
cannot really eliminate the LoadField for the object in each iteration, because
it can also be undefined, and thus we cannot just eliminate the map check
(unless we also do some heavy tracking of information along the control chain).
Plus this part of the spec is only concerned about resource management, and
that's not the business of the language spec.

So nothing to change for now (esp. not for this CL), but some heads up for the
future.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/access-bu...
src/compiler/access-builder.cc:568: TypeCache::Get().kPositiveSafeInteger,
s/kPositiveSafeInteger/kPositiveInteger/

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:230: bool for_typed_array) {
Nit: Can I haz an enum for this plz?

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:236: // For JSTypedArray iterator methods,
deopt if the buffer is neutered
Please add a hint, that this is a potential, but highly unlikely deoptimization
loop.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:237: DCHECK_EQ(receiver_map->instance_type(),
JS_TYPED_ARRAY_TYPE);
Nit: DCHECK_EQ(JS_TYPED_ARRAY_TYPE, receiver_map->instance_type())

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:436: simplified()->CheckFloat64Hole(mode),
value, etrue1, if_true1);
This will most likely deopt on every hole, as it's unlikely that all uses will
be truncated (i.e. at the very least we will probably see an ObjectState use for
the deoptimizer). Can you add a TODO with my name that we need to fix the double
hole thing in TurboFan for real (we have a similar problem already in
JSNativeContextSpecialization, so this is not an issue for the ArrayIterator
only).

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:442: javascript()->CreateKeyValueArray(),
index, value, etrue1);
Nice idea!

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-create...
File src/compiler/js-create-lowering.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-create...
src/compiler/js-create-lowering.cc:740: aa.Allocate(FixedArray::SizeFor(2));
Nit: Use AllocateArray instead.

[caitp, 2016-11-09 20:55:25]

so that's it.. i think this probably still needs better test coverage before I
CQ though, but maybe it's pretty safe.

https://codereview.chromium.org/2484003002/diff/230001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2328: // Invalidate array_iterator_protector cell
if needed (Not needed
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> You can simplify this further and just always write kArrayProtectorInvalid to
> the cell. That's probably as cheap as load+check.

Ah, that's already been removed. Will update the comment though.

https://codereview.chromium.org/2484003002/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2330: assembler->StoreObjectField(
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> Skip write barrier for this StoreObjectField.

Done.

https://codereview.chromium.org/2484003002/diff/230001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2343: assembler->StoreObjectField(cell,
Cell::kValueOffset, invalid);
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> Skip write barrier for this StoreObjectField.

Done.

https://codereview.chromium.org/2484003002/diff/230001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2484003002/diff/230001/src/code-stub-assemble...
src/code-stub-assembler.h:341: compiler::Node*
IsInAnyNativeContext(compiler::Node* object, uint32_t index);
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> Drop these dead declarations.

Done.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/access-bu...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/access-bu...
src/compiler/access-builder.cc:556: Type::ReceiverOrUndefined(),
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> Note for later changes/direction: I've been chatting about this with Camillo
> offline today, and I think we might want to change this aspect of the spec in
> the future (or violate the spec here), as the undefined clearing of the
internal
> object slot causes a lot of trouble, for example, the load/check elimination
> cannot really eliminate the LoadField for the object in each iteration,
because
> it can also be undefined, and thus we cannot just eliminate the map check
> (unless we also do some heavy tracking of information along the control
chain).
> Plus this part of the spec is only concerned about resource management, and
> that's not the business of the language spec.
> 
> So nothing to change for now (esp. not for this CL), but some heads up for the
> future.

Acknowledged.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/access-bu...
src/compiler/access-builder.cc:568: TypeCache::Get().kPositiveSafeInteger,
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> s/kPositiveSafeInteger/kPositiveInteger/

Iteration stops if index >= ToLength(array.[[Get]]("length")), doesn't that cap
this to 2^53-1, since the index is never incremented in that situation?

Done for now, but we could I think it's valid to narrow the range of values a
bit

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:230: bool for_typed_array) {
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> Nit: Can I haz an enum for this plz?

Done.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:236: // For JSTypedArray iterator methods,
deopt if the buffer is neutered
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> Please add a hint, that this is a potential, but highly unlikely
deoptimization
> loop.

Done.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:237: DCHECK_EQ(receiver_map->instance_type(),
JS_TYPED_ARRAY_TYPE);
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> Nit: DCHECK_EQ(JS_TYPED_ARRAY_TYPE, receiver_map->instance_type())

Done.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:436: simplified()->CheckFloat64Hole(mode),
value, etrue1, if_true1);
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> This will most likely deopt on every hole, as it's unlikely that all uses will
> be truncated (i.e. at the very least we will probably see an ObjectState use
for
> the deoptimizer). Can you add a TODO with my name that we need to fix the
double
> hole thing in TurboFan for real (we have a similar problem already in
> JSNativeContextSpecialization, so this is not an issue for the ArrayIterator
> only).

Done. Also removed the "Check if we are allowed..." comment, since it doesn't
seem to be valid now that CanTreatHoleAsUndefined is gone.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-create...
File src/compiler/js-create-lowering.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-create...
src/compiler/js-create-lowering.cc:740: aa.Allocate(FixedArray::SizeFor(2));
On 2016/11/09 20:00:32, Benedikt Meurer wrote:
> Nit: Use AllocateArray instead.

Done.

[Benedikt Meurer, 2016-11-10 04:49:55]

Having more tests would certainly help, especially in feeding our various
fuzzers now. Testing the various bailout paths would also be nice.

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/access-bu...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/access-bu...
src/compiler/access-builder.cc:568: TypeCache::Get().kPositiveSafeInteger,
Good point, you are right, thanks for the explanation. So feel free to change it
back (and maybe add a comment why this is safe).

[caitp, 2016-11-10 06:30:20]

On 2016/11/10 04:49:55, Benedikt Meurer wrote:
> Having more tests would certainly help, especially in feeding our various
> fuzzers now. Testing the various bailout paths would also be nice.

Started writing tests, and found a crash already. It looks like the frame_state
in LowerCheckFloat64Hole() is null, which seems to be related to the comments in
js-native-context-specialization.cc around each point where a Checkpoint op is
added ("The effect/control linearization will not find a FrameState after the
EffectPhi that is generated above.").

[Benedikt Meurer, 2016-11-10 06:33:40]

On 2016/11/10 06:30:20, caitp wrote:
> On 2016/11/10 04:49:55, Benedikt Meurer wrote:
> > Having more tests would certainly help, especially in feeding our various
> > fuzzers now. Testing the various bailout paths would also be nice.
> 
> Started writing tests, and found a crash already. It looks like the
frame_state
> in LowerCheckFloat64Hole() is null, which seems to be related to the comments
in
> js-native-context-specialization.cc around each point where a Checkpoint op is
> added ("The effect/control linearization will not find a FrameState after the
> EffectPhi that is generated above.").

That's easy to fix in your case, just move the StoreField for the index below
the access.

[caitp, 2016-11-10 20:22:59]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 20:23:03]

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

[caitp, 2016-11-10 20:24:41]

On 2016/11/10 06:33:40, Benedikt Meurer wrote:
> On 2016/11/10 06:30:20, caitp wrote:
> > On 2016/11/10 04:49:55, Benedikt Meurer wrote:
> > > Having more tests would certainly help, especially in feeding our various
> > > fuzzers now. Testing the various bailout paths would also be nice.
> > 
> > Started writing tests, and found a crash already. It looks like the
> frame_state
> > in LowerCheckFloat64Hole() is null, which seems to be related to the
comments
> in
> > js-native-context-specialization.cc around each point where a Checkpoint op
is
> > added ("The effect/control linearization will not find a FrameState after
the
> > EffectPhi that is generated above.").
> 
> That's easy to fix in your case, just move the StoreField for the index below
> the access.

I've added some test coverage which found some issues (since been fixed), but
I'm not totally sure how to test specific bailouts conditions (which I'd like to
check).

[caitp, 2016-11-10 20:27:42]

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
File src/compiler/js-builtin-reducer.cc (right):

https://codereview.chromium.org/2484003002/diff/230001/src/compiler/js-builti...
src/compiler/js-builtin-reducer.cc:244: effect =
graph()->NewNode(simplified()->CheckIf(), check, effect, control);
CheckIf does a DeoptimizeUnless(), so this is deoptimizing if the buffer is
_not_ neutered. Replacing with DeoptimizeIf() instead.

https://codereview.chromium.org/2484003002/diff/270001/test/mjsunit/es6/array...
File test/mjsunit/es6/array-iterator-turbo.js (right):

https://codereview.chromium.org/2484003002/diff/270001/test/mjsunit/es6/array...
test/mjsunit/es6/array-iterator-turbo.js:222: assertThrows(() => sum(clone),
TypeError);
for example (of not being able to test bailouts in JS code), this test passes,
but `sum` is still turbofan code. So,this means the baseline path must be taken,
but what I would expect is that the iterator constructor is inlined in
turbofanned `sum`(), which should lead to a bailout.

I expect I'd put tests for these in cctest/compiler/test-inlining.cc, but there
doesn't seem to be a lot of precedent for those sorts of tests there.

[commit-bot: I haz the power, 2016-11-10 20:39:27]

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

[commit-bot: I haz the power, 2016-11-10 20:39:28]

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...)

[caitp, 2016-11-10 20:42:11]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 20:42: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-10 20:55:07]

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

[commit-bot: I haz the power, 2016-11-10 20:55: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...)

[caitp, 2016-11-10 21:03:40]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 21:03:52]

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-10 21:17:41]

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

[commit-bot: I haz the power, 2016-11-10 21:17:43]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[caitp, 2016-11-10 21:20:53]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

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

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-10 21:47:22]

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

[commit-bot: I haz the power, 2016-11-10 21:47:23]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-11-11 06:41:11]

>
https://codereview.chromium.org/2484003002/diff/270001/test/mjsunit/es6/array...
> test/mjsunit/es6/array-iterator-turbo.js:222: assertThrows(() => sum(clone),
> TypeError);
> for example (of not being able to test bailouts in JS code), this test passes,
> but `sum` is still turbofan code. So,this means the baseline path must be
taken,
> but what I would expect is that the iterator constructor is inlined in
> turbofanned `sum`(), which should lead to a bailout.
> 
> I expect I'd put tests for these in cctest/compiler/test-inlining.cc, but
there
> doesn't seem to be a lot of precedent for those sorts of tests there.

In the past those cctest have proven to become a maintenance problem shortly
after they were introduced. 
I'd suggest to land the CL as-is for now, and let our various fuzzers play with
the mjsunit tests.

[Camillo Bruni, 2016-11-11 11:27:56]

LGTM

https://codereview.chromium.org/2484003002/diff/350001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2484003002/diff/350001/src/builtins/builtins-...
src/builtins/builtins-array.cc:2345: invalid);
Update: Mostly ignore my ramblings below ;) and keep on going with this CL!

If I understand this correctly, this means you invalidate the protector whenever
the array you're iterating over changes it's map.
This can easily happen with elements transition:

let a = [1, 2]
for (let v in a) { a[0] = 1.2 }

In this case you would disable all future iterator optimizations.
There are anyway only `number of elements kind` map transitions than can occur
in the normal case. 
The degenerate case, where people start adding properties to the iterated array
would probably still cause deopts even though the properties (unlike elements)
would not be visible in the default iterator.

I don't know if there is a possibility, but you probably only care about the
elements as deoptimization reason.

I suggest adding a runtime function to runtime-debug.cc to query the protector
in the tests, to make sure you don't accidentally invalidate it.

[Benedikt Meurer (Google), 2016-11-11 16:39:16]

On 2016/11/11 11:27:56, Camillo Bruni wrote:
> LGTM
> 
>
https://codereview.chromium.org/2484003002/diff/350001/src/builtins/builtins-...
> File src/builtins/builtins-array.cc (right):
> 
>
https://codereview.chromium.org/2484003002/diff/350001/src/builtins/builtins-...
> src/builtins/builtins-array.cc:2345: invalid);
> Update: Mostly ignore my ramblings below ;) and keep on going with this CL!
> 
> If I understand this correctly, this means you invalidate the protector
whenever
> the array you're iterating over changes it's map.
> This can easily happen with elements transition:
> 
> let a = [1, 2]
> for (let v in a) { a[0] = 1.2 }
> 
> In this case you would disable all future iterator optimizations.
> There are anyway only `number of elements kind` map transitions than can occur
> in the normal case. 
> The degenerate case, where people start adding properties to the iterated
array
> would probably still cause deopts even though the properties (unlike elements)
> would not be visible in the default iterator.
> 
> I don't know if there is a possibility, but you probably only care about the
> elements as deoptimization reason.
> 
> I suggest adding a runtime function to runtime-debug.cc to query the protector
> in the tests, to make sure you don't accidentally invalidate it.

As per offline discussion: yes we can improve, but we need a baseline first.
Plus mutation during iteration is usually a bug.

[Benedikt Meurer, 2016-11-13 18:40:24]

https://codereview.chromium.org/2484003002/diff/350001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/2484003002/diff/350001/src/heap/heap.h#newcod...
src/heap/heap.h:169: V(Cell, array_iterator_protector, ArrayIteratorProtector)  
                 \
Camillo suggested to rename this to something like FastArrayIterationProtector,
and use the name ArrayIteratorProtector to protect the @@iterator property on
the Array.prototype, which would be more consistent with the  other protectors.

[Benedikt Meurer, 2016-11-14 04:47:12]

I'd love to get this in for M56. Can you rename the protector and land the CL?

[caitp, 2016-11-14 15:12:17]

The CQ bit was checked by caitp@igalia.com

[caitp, 2016-11-14 15:12:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2484003002/#ps370001
(title: "rename protector cell for landing")

[commit-bot: I haz the power, 2016-11-14 15:12:28]

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-14 15:28:20]

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

[commit-bot: I haz the power, 2016-11-14 15:28:22]

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...)

[caitp, 2016-11-14 15:35:02]

The CQ bit was checked by caitp@igalia.com

[caitp, 2016-11-14 15:35:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2484003002/#ps390001
(title: "fix tests when ignition is used")

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

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-14 15:58:59]

Description was changed from

==========
[builtins] implement JSBuiltinReducer for ArrayIteratorNext()

Adds a protector cell to prevent inlining (which will likely lead to deopt
loops) when a JSArrayIterator's array transitions from a fast JSArray to a
slow JSArray (such as, when the array is touched during iteration in a way
which triggers a map transition).

Also adds TODO comments relating to the spec update proposed by Dan at
https://github.com/tc39/ecma262/pull/724

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
TBR=hpayer@chromium.org, ulan@chromium.org
==========

to

==========
[builtins] implement JSBuiltinReducer for ArrayIteratorNext()

Adds a protector cell to prevent inlining (which will likely lead to deopt
loops) when a JSArrayIterator's array transitions from a fast JSArray to a
slow JSArray (such as, when the array is touched during iteration in a way
which triggers a map transition).

Also adds TODO comments relating to the spec update proposed by Dan at
https://github.com/tc39/ecma262/pull/724

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
TBR=hpayer@chromium.org, ulan@chromium.org
==========

[commit-bot: I haz the power, 2016-11-14 15:59:00]

Committed patchset #21 (id:390001)

[Michael Achenbach, 2016-11-14 16:46:12]

On 2016/11/14 15:59:00, commit-bot: I haz the power wrote:
> Committed patchset #21 (id:390001)

A test seems to be not suitable for deopt fuzzing:
https://build.chromium.org/p/client.v8/builders/V8%20Deopt%20Fuzzer/builds/14927

Could you add it to the skips in the deopt section in the status file?

[caitp, 2016-11-14 16:47:17]

On 2016/11/14 16:46:12, machenbach (slow) wrote:
> On 2016/11/14 15:59:00, commit-bot: I haz the power wrote:
> > Committed patchset #21 (id:390001)
> 
> A test seems to be not suitable for deopt fuzzing:
>
https://build.chromium.org/p/client.v8/builders/V8%20Deopt%20Fuzzer/builds/14927
> 
> Could you add it to the skips in the deopt section in the status file?

I saw the failure, and I've pushed a CL to fix this (skips ALWAYS, except for
the turbofan variant), would that work okay?

[caitp, 2016-11-14 16:49:22]

On 2016/11/14 16:47:17, caitp wrote:
> On 2016/11/14 16:46:12, machenbach (slow) wrote:
> > On 2016/11/14 15:59:00, commit-bot: I haz the power wrote:
> > > Committed patchset #21 (id:390001)
> > 
> > A test seems to be not suitable for deopt fuzzing:
> >
>
https://build.chromium.org/p/client.v8/builders/V8%20Deopt%20Fuzzer/builds/14927
> > 
> > Could you add it to the skips in the deopt section in the status file?
> 
> I saw the failure, and I've pushed a CL to fix this (skips ALWAYS, except for
> the turbofan variant), would that work okay?

take a look at https://codereview.chromium.org/2496063003/ for a fix.

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

Description was changed from

==========
[builtins] implement JSBuiltinReducer for ArrayIteratorNext()

Adds a protector cell to prevent inlining (which will likely lead to deopt
loops) when a JSArrayIterator's array transitions from a fast JSArray to a
slow JSArray (such as, when the array is touched during iteration in a way
which triggers a map transition).

Also adds TODO comments relating to the spec update proposed by Dan at
https://github.com/tc39/ecma262/pull/724

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
TBR=hpayer@chromium.org, ulan@chromium.org
==========

to

==========
[builtins] implement JSBuiltinReducer for ArrayIteratorNext()

Adds a protector cell to prevent inlining (which will likely lead to deopt
loops) when a JSArrayIterator's array transitions from a fast JSArray to a
slow JSArray (such as, when the array is touched during iteration in a way
which triggers a map transition).

Also adds TODO comments relating to the spec update proposed by Dan at
https://github.com/tc39/ecma262/pull/724

BUG=v8:5388
R=bmeurer@chromium.org, mstarzinger@chromium.org
TBR=hpayer@chromium.org, ulan@chromium.org

Committed: https://crrev.com/7f21e67b38a859c5503777a606d68ef3cb449517
Cr-Commit-Position: refs/heads/master@{#40970}
==========

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

Patchset 21 (id:??) landed as
https://crrev.com/7f21e67b38a859c5503777a606d68ef3cb449517
Cr-Commit-Position: refs/heads/master@{#40970}