[ignition] desugar GetIterator() via bytecode rather than via AST

[ignition] desugar GetIterator() via bytecode rather than via AST

Introduces:
- a new AST node representing the GetIterator() algorithm in the specification, to be used by ForOfStatement, YieldExpression (in the case of delegating yield*), and the future `for-await-of` loop proposed in http://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwrap-functions.
- a new opcode (JumpIfJSReceiver), which is useful for `if Type(object) is not Object` checks which are common throughout the specification. This node is easily eliminated by TurboFan.

The AST node is desugared specially in bytecode, rather than manually when building the AST. The benefit of this is that desugaring in the BytecodeGenerator is much simpler and easier to understand than desugaring the AST.

This also reduces parse time very slightly, and allows us to use LoadIC rather than KeyedLoadIC, which seems to have  better baseline performance. This results in a ~20% improvement in test/js-perf-test/Iterators micro-benchmarks, which I believe owes to the use of the slightly faster LoadIC as opposed to the KeyedLoadIC in the baseline case. Both produce identical optimized code via TurboFan when the type check can be eliminated, and the load can be replaced with a constant value.

BUG=v8:4280
R=bmeurer@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, neis@chromium.org, jarin@chromium.org
TBR=rossberg@chromium.org

Committed: https://crrev.com/b5f146a02aeb28b89df6a2494ddf700a98d372ec
Cr-Commit-Position: refs/heads/master@{#41555}

[caitp, 2016-12-06 03:36:40]

caitp@igalia.com changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[caitp, 2016-12-06 03:36:41]

Hi,

I was experimenting with an approach like this mainly because writing bytecode
seems to be much simpler than building an AST, and I wanted to build ontop of
something like this when implementing the GetIterator changes for the async
iteration proposal.

Using the `Iterators` perf tests locally, this actually seems to yield some
degree of consistent advantage:

```
tools/run_perf.py --outdir=out_patched/ --outdir-no-patch=out_nopatched/
test/js-perf-test/Iterators.json 
>>> Running suite: Iterators/Iterators
>>> Stdout (#1):
ForOf-Iterators(Score): 363319

>>> Stdout (#1) - without patch:
ForOf-Iterators(Score): 299337

>>> Stdout (#2):
ForOf-Iterators(Score): 366128

>>> Stdout (#2) - without patch:
ForOf-Iterators(Score): 304000

>>> Stdout (#3):
ForOf-Iterators(Score): 363127

>>> Stdout (#3) - without patch:
ForOf-Iterators(Score): 303437
```

The downside is the new AST node (can't use CallRuntime or similar because of
the need for feedback slots).

What do you think?

[caitp, 2016-12-06 03:56:41]

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

[commit-bot: I haz the power, 2016-12-06 03:56:55]

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-12-06 04:13:58]

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

[commit-bot: I haz the power, 2016-12-06 04:13:58]

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

[Benedikt Meurer, 2016-12-06 05:02:00]

bmeurer@chromium.org changed reviewers:
+ adamk@chromium.org, neis@chromium.org

[Benedikt Meurer, 2016-12-06 05:02:00]

Hey Caitlin,

This goes into the direction we've been discussing briefly in the past (mostly
offline discussions in MUC), so that's definitely a good idea. My vision is that
we have bytecodes for the relevant spec operations on iterators as well and
basically push down most of the for-of desugaring to the bytecode generator to
eventually be able to also avoid the double try/catch around it.

I think the extra AST Node is good, because it makes these things more explicit.

Adding Adam and Georg in case they have an opinion.

-- Benedikt

[Benedikt Meurer, 2016-12-06 05:04:50]

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

[Benedikt Meurer, 2016-12-06 05:04:51]

We talked about ideas how to deopt into "the middle" of a bytecode earlier. In
theory it should be doable, but I'm not sure we want to go there at this point
in time.

https://codereview.chromium.org/2557593004/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2557593004/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2863: .Call(method, args,
feedback_index(call_slot), Call::NAMED_PROPERTY_CALL);
This is probably not going to work w/o additional, because you don't have lazy
bailout locations for the Load and the Call when deoptimizing from TurboFan.

[Benedikt Meurer, 2016-12-06 05:06:32]

https://codereview.chromium.org/2557593004/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2557593004/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2863: .Call(method, args,
feedback_index(call_slot), Call::NAMED_PROPERTY_CALL);
On 2016/12/06 05:04:51, Benedikt Meurer wrote:
> This is probably not going to work w/o additional, because you don't have lazy
> bailout locations for the Load and the Call when deoptimizing from TurboFan.

Ah forget it, and ignore this comment. :-)
I accidentially thought I was looking at interpreter.cc here.
Obviously it works as long as you do in the bytecode-generator

[caitp, 2016-12-06 05:49:11]

https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/bytecode_expectations/ForOf.golden (right):

https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/bytecode_expectations/ForOf.golden:27: B(CallRuntime),
U16(Runtime::kThrowSymbolIteratorInvalid), R(0), U8(0),
well, the other downside is how much bigger the bytecode is because of the
JumpIfJSReceiver + runtime call (implementing step 3 of
https://tc39.github.io/ecma262/#sec-getiterator) --- but the iterator.next()
call may make this somewhat redundant. Might actually shrink the bytecode a bit
if that's gotten rid of. In upstream v8, this exception is only thrown before
the call to .next() for `yield*`, and it's possible to get the same semantics
here.

[caitp, 2016-12-06 14:31:33]

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

[commit-bot: I haz the power, 2016-12-06 14:31:39]

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-12-06 14:33:15]

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

[commit-bot: I haz the power, 2016-12-06 14:33:18]

Dry run: Try jobs failed on following builders:
  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_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17371)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18979)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19103)

[rmcilroy, 2016-12-06 15:02:28]

Approach LGTM from the point of view of the interpreter. A couple of nits and a
question.

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2858: // Let method be ? GetMethod(obj,
@@iterator).
Not sure what the "?" means here. Can we just remove it?

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:2114: __ Bind(&if_notobject);
nit - add a newline above this bind

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:2135: __ Bind(&if_notobject);
ditto

https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/bytecode_expectations/ForOf.golden (right):

https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/bytecode_expectations/ForOf.golden:27: B(CallRuntime),
U16(Runtime::kThrowSymbolIteratorInvalid), R(0), U8(0),
On 2016/12/06 05:49:11, caitp wrote:
> well, the other downside is how much bigger the bytecode is because of the
> JumpIfJSReceiver + runtime call (implementing step 3 of
> https://tc39.github.io/ecma262/#sec-getiterator) --- but the iterator.next()
> call may make this somewhat redundant. Might actually shrink the bytecode a
bit
> if that's gotten rid of. In upstream v8, this exception is only thrown before
> the call to .next() for `yield*`, and it's possible to get the same semantics
> here.

I'm slightly confused by this - it looks like the parser previously should have
added nodes for doing this check / throw, but I don't see the bytecode added in
the old expectations. Am I missing something? Shouldn't this be strictly less
bytecode than before since we now have the special bytecode rather than having
to do the runtime call?

https://codereview.chromium.org/2557593004/diff/20001/test/unittests/interpre...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:238:
.LoadLiteral(factory->prototype_string())
nit - no need to load the literal here (the code in this test is never executed
so doesn't need to make sense :).

[caitp, 2016-12-06 15:14:59]

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2858: // Let method be ? GetMethod(obj,
@@iterator).
On 2016/12/06 15:02:28, rmcilroy wrote:
> Not sure what the "?" means here. Can we just remove it?

It's the spec's short form for "ReturnIfAbrupt(<completion>)", basically just
means the op can throw. Fine to remove it

https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/bytecode_expectations/ForOf.golden (right):

https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/bytecode_expectations/ForOf.golden:27: B(CallRuntime),
U16(Runtime::kThrowSymbolIteratorInvalid), R(0), U8(0),
On 2016/12/06 15:02:28, rmcilroy wrote:
> On 2016/12/06 05:49:11, caitp wrote:
> > well, the other downside is how much bigger the bytecode is because of the
> > JumpIfJSReceiver + runtime call (implementing step 3 of
> > https://tc39.github.io/ecma262/#sec-getiterator) --- but the iterator.next()
> > call may make this somewhat redundant. Might actually shrink the bytecode a
> bit
> > if that's gotten rid of. In upstream v8, this exception is only thrown
before
> > the call to .next() for `yield*`, and it's possible to get the same
semantics
> > here.
> 
> I'm slightly confused by this - it looks like the parser previously should
have
> added nodes for doing this check / throw, but I don't see the bytecode added
in
> the old expectations. Am I missing something? Shouldn't this be strictly less
> bytecode than before since we now have the special bytecode rather than having
> to do the runtime call?

These nodes were only added for yield* desugaring, and not for for-of loops, so
you see them in this case:

```
function* g() { yield* { [Symbol.iterator]() { return 1; } } }
g().next(); // Throws "TypeError: Result of the Symbol.iterator method is not an
object"
```

They were never added for the for-of loop, and we could probably get away with
never adding it, since .next() is always called immediately after GetIterator()
in the current spec. It would be nice to have it, but since it grows the
bytecode so much, it may not be worth it.

So, there are a couple of options:

1) always get rid of the receiver check/runtime call, and rely on the TypeError
thrown by Call. This should shrink the generated bytecode a bit
2) get rid of the receiver check for for-of loops only (this would match the
current behaviour in trunk)
3) always have the check, and be spec compliant at the cost of some bigger
bytecode
4) add a new opcode for throwing if this check fails, and do the runtime call
directly from interpreter.cc and reduce it appropriately in TF

just a question of how to balance strict compliance with the spec and the size
of bytecode

https://codereview.chromium.org/2557593004/diff/20001/test/unittests/interpre...
File test/unittests/interpreter/bytecode-array-builder-unittest.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/test/unittests/interpre...
test/unittests/interpreter/bytecode-array-builder-unittest.cc:238:
.LoadLiteral(factory->prototype_string())
On 2016/12/06 15:02:28, rmcilroy wrote:
> nit - no need to load the literal here (the code in this test is never
executed
> so doesn't need to make sense :).

Acknowledged.

[caitp, 2016-12-06 16:07:07]

https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/bytecode_expectations/ForOf.golden (right):

https://codereview.chromium.org/2557593004/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/bytecode_expectations/ForOf.golden:27: B(CallRuntime),
U16(Runtime::kThrowSymbolIteratorInvalid), R(0), U8(0),
On 2016/12/06 15:14:58, caitp wrote:
> On 2016/12/06 15:02:28, rmcilroy wrote:
> > On 2016/12/06 05:49:11, caitp wrote:
> > > well, the other downside is how much bigger the bytecode is because of the
> > > JumpIfJSReceiver + runtime call (implementing step 3 of
> > > https://tc39.github.io/ecma262/#sec-getiterator) --- but the
iterator.next()
> > > call may make this somewhat redundant. Might actually shrink the bytecode
a
> > bit
> > > if that's gotten rid of. In upstream v8, this exception is only thrown
> before
> > > the call to .next() for `yield*`, and it's possible to get the same
> semantics
> > > here.
> > 
> > I'm slightly confused by this - it looks like the parser previously should
> have
> > added nodes for doing this check / throw, but I don't see the bytecode added
> in
> > the old expectations. Am I missing something? Shouldn't this be strictly
less
> > bytecode than before since we now have the special bytecode rather than
having
> > to do the runtime call?
> 
> These nodes were only added for yield* desugaring, and not for for-of loops,
so
> you see them in this case:
> 
> ```
> function* g() { yield* { [Symbol.iterator]() { return 1; } } }
> g().next(); // Throws "TypeError: Result of the Symbol.iterator method is not
an
> object"
> ```
> 
> They were never added for the for-of loop, and we could probably get away with
> never adding it, since .next() is always called immediately after
GetIterator()
> in the current spec. It would be nice to have it, but since it grows the
> bytecode so much, it may not be worth it.
> 
> So, there are a couple of options:
> 
> 1) always get rid of the receiver check/runtime call, and rely on the
TypeError
> thrown by Call. This should shrink the generated bytecode a bit
> 2) get rid of the receiver check for for-of loops only (this would match the
> current behaviour in trunk)
> 3) always have the check, and be spec compliant at the cost of some bigger
> bytecode
> 4) add a new opcode for throwing if this check fails, and do the runtime call
> directly from interpreter.cc and reduce it appropriately in TF
> 
> just a question of how to balance strict compliance with the spec and the size
> of bytecode

Also note, not including the typecheck is an observable spec violation (which is
incompatible with SpiderMonkey), but this is admittedly unlikely to matter in
real code. (https://jsfiddle.net/gj3uaw1t/)

[adamk, 2016-12-06 18:49:05]

Regarding the question about the receiver check, my vote is for strict
compliance.

Can you add some info about the performance win to the CL description? Without
that I would be skeptical of this change, at least on its own: it's a lot of new
code, and the existing desugaring was pretty straightforward. Do you have any
guesses about why this is such a noticeable win? I don't immediately see why the
desugared AST wouldn't be nearly as fast...

https://codereview.chromium.org/2557593004/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2557593004/diff/40001/src/ast/ast.h#newcode2925
src/ast/ast.h:2925: // "Property" with additional semantics.
This could use a little more detail for readers not well-versed in ES spec
language.

https://codereview.chromium.org/2557593004/diff/40001/src/compiler/ast-loop-a...
File src/compiler/ast-loop-assignment-analyzer.cc (right):

https://codereview.chromium.org/2557593004/diff/40001/src/compiler/ast-loop-a...
src/compiler/ast-loop-assignment-analyzer.cc:204: void
ALAA::VisitGetIterator(GetIterator* e) { Visit(e->iterable()); }
Isn't this just as unreachable as the AstGraphBuilder method?

https://codereview.chromium.org/2557593004/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2557593004/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:4667: // let iterator = iterable[Symbol.iterator];
Please update this comment to to match the full desugaring above.

[caitp, 2016-12-06 19:00:44]

On 2016/12/06 18:49:05, adamk wrote:
> Regarding the question about the receiver check, my vote is for strict
> compliance.
> 
> Can you add some info about the performance win to the CL description? Without
> that I would be skeptical of this change, at least on its own: it's a lot of
new
> code, and the existing desugaring was pretty straightforward. Do you have any
> guesses about why this is such a noticeable win? I don't immediately see why
the
> desugared AST wouldn't be nearly as fast...

My best guess on the reason for the speedup is that it uses LoadIC rather than
KeyedLoadIC for getting the @@iterator method.

https://codereview.chromium.org/2557593004/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2557593004/diff/40001/src/ast/ast.h#newcode2925
src/ast/ast.h:2925: // "Property" with additional semantics.
On 2016/12/06 18:49:05, adamk wrote:
> This could use a little more detail for readers not well-versed in ES spec
> language.

Acknowledged.

https://codereview.chromium.org/2557593004/diff/40001/src/compiler/ast-loop-a...
File src/compiler/ast-loop-assignment-analyzer.cc (right):

https://codereview.chromium.org/2557593004/diff/40001/src/compiler/ast-loop-a...
src/compiler/ast-loop-assignment-analyzer.cc:204: void
ALAA::VisitGetIterator(GetIterator* e) { Visit(e->iterable()); }
On 2016/12/06 18:49:05, adamk wrote:
> Isn't this just as unreachable as the AstGraphBuilder method?

We run into it in cctest/test-loop-assignment-analysis/SimpleLoop2, which runs
the ALAA regardless of whether optimization is disabled or not.

The alternative is to just remove the test, either one seems fine

https://codereview.chromium.org/2557593004/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2557593004/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:4667: // let iterator = iterable[Symbol.iterator];
On 2016/12/06 18:49:05, adamk wrote:
> Please update this comment to to match the full desugaring above.

Acknowledged.

[Benedikt Meurer, 2016-12-06 19:17:07]

I also would like us to fully understand where the performance difference comes
from first. Have you checked the TF generated code?

https://codereview.chromium.org/2557593004/diff/40001/src/compiler/ast-loop-a...
File src/compiler/ast-loop-assignment-analyzer.cc (right):

https://codereview.chromium.org/2557593004/diff/40001/src/compiler/ast-loop-a...
src/compiler/ast-loop-assignment-analyzer.cc:204: void
ALAA::VisitGetIterator(GetIterator* e) { Visit(e->iterable()); }
I'd drop the test in that case.

[caitp, 2016-12-06 19:56:34]

On 2016/12/06 19:17:07, Benedikt Meurer wrote:
> I also would like us to fully understand where the performance difference
comes
> from first. Have you checked the TF generated code?
> 

--trace-turbo-reduction seems to indicate that we're replacing the Load with a
mapcheck and constant (627 == HeapConstant for Array `values`),, and eliminating
the ObjectIsReceiver check and CallRuntime. The call to values appears to get 
inlined, and it doesn't seem to be possible to turn off inlining from
JSBuiltinReducer)

```
- Replacing 30: Checkpoint(34, 626, 15) with 626: CheckMaps[1](624, 625, 624,
15)
- Replacing 40: IfSuccess(35) with 15: IfSuccess(14)
- Replacing 35: JSCallFunction[2, 1, NOT_NULL_OR_UNDEFINED,
DISALLOW_TAIL_CALLS](627, 3, 1, 43, 626, 15) with 652: FinishRegion(644, 651)
- Replacing 44: ObjectIsReceiver(652) with 199: HeapConstant[0x12e0dd3823b1
<true>]
- Replacing 48: IfFalse(45) with 641: Dead
- Replacing 46: IfTrue(45) with 15: IfSuccess(14)
- Replacing 45: Branch[None](199, 15) with 641: Dead
- Replacing 49: Checkpoint(52, 652, 641) with 641: Dead
- Replacing 53: JSCallRuntime[ThrowSymbolIteratorInvalid, 0](1, 59, 641, 641)
with 641: Dead
- Replacing 56: IfSuccess(641) with 641: Dead
```

[caitp, 2016-12-06 19:59:41]

On 2016/12/06 19:56:34, caitp wrote:
> On 2016/12/06 19:17:07, Benedikt Meurer wrote:
> > I also would like us to fully understand where the performance difference
> comes
> > from first. Have you checked the TF generated code?
> > 
> 
> --trace-turbo-reduction seems to indicate that we're replacing the Load with a
> mapcheck and constant (627 == HeapConstant for Array `values`),, and
eliminating
> the ObjectIsReceiver check and CallRuntime. The call to values appears to get 
> inlined, and it doesn't seem to be possible to turn off inlining from
> JSBuiltinReducer)
> 
> ```
> - Replacing 30: Checkpoint(34, 626, 15) with 626: CheckMaps[1](624, 625, 624,
> 15)
> - Replacing 40: IfSuccess(35) with 15: IfSuccess(14)
> - Replacing 35: JSCallFunction[2, 1, NOT_NULL_OR_UNDEFINED,
> DISALLOW_TAIL_CALLS](627, 3, 1, 43, 626, 15) with 652: FinishRegion(644, 651)
> - Replacing 44: ObjectIsReceiver(652) with 199: HeapConstant[0x12e0dd3823b1
> <true>]
> - Replacing 48: IfFalse(45) with 641: Dead
> - Replacing 46: IfTrue(45) with 15: IfSuccess(14)
> - Replacing 45: Branch[None](199, 15) with 641: Dead
> - Replacing 49: Checkpoint(52, 652, 641) with 641: Dead
> - Replacing 53: JSCallRuntime[ThrowSymbolIteratorInvalid, 0](1, 59, 641, 641)
> with 641: Dead
> - Replacing 56: IfSuccess(641) with 641: Dead
> ```

I would expect this is similar to the optimization we'd see with the AST
desugaring, so... will investigate that

[caitp, 2016-12-06 20:13:24]

On 2016/12/06 19:59:41, caitp wrote:
> On 2016/12/06 19:56:34, caitp wrote:
> > On 2016/12/06 19:17:07, Benedikt Meurer wrote:
> > > I also would like us to fully understand where the performance difference
> > comes
> > > from first. Have you checked the TF generated code?
> > > 
> > 
> > --trace-turbo-reduction seems to indicate that we're replacing the Load with
a
> > mapcheck and constant (627 == HeapConstant for Array `values`),, and
> eliminating
> > the ObjectIsReceiver check and CallRuntime. The call to values appears to
get 
> > inlined, and it doesn't seem to be possible to turn off inlining from
> > JSBuiltinReducer)
> > 
> > ```
> > - Replacing 30: Checkpoint(34, 626, 15) with 626: CheckMaps[1](624, 625,
624,
> > 15)
> > - Replacing 40: IfSuccess(35) with 15: IfSuccess(14)
> > - Replacing 35: JSCallFunction[2, 1, NOT_NULL_OR_UNDEFINED,
> > DISALLOW_TAIL_CALLS](627, 3, 1, 43, 626, 15) with 652: FinishRegion(644,
651)
> > - Replacing 44: ObjectIsReceiver(652) with 199: HeapConstant[0x12e0dd3823b1
> > <true>]
> > - Replacing 48: IfFalse(45) with 641: Dead
> > - Replacing 46: IfTrue(45) with 15: IfSuccess(14)
> > - Replacing 45: Branch[None](199, 15) with 641: Dead
> > - Replacing 49: Checkpoint(52, 652, 641) with 641: Dead
> > - Replacing 53: JSCallRuntime[ThrowSymbolIteratorInvalid, 0](1, 59, 641,
641)
> > with 641: Dead
> > - Replacing 56: IfSuccess(641) with 641: Dead
> > ```
> 
> I would expect this is similar to the optimization we'd see with the AST
> desugaring, so... will investigate that

Yeah, the main difference as far as TF seems to be concerned is, previously was
generating `JSLoadProperty[sloppy]`, and now `JSLoadNamed`, but in both cases
the optimized code replaces them with constants in the same way. They both end
up generating identical optimized code, so the win seems to come from the
baseline case.

[caitp, 2016-12-06 20:14:52]

On 2016/12/06 20:13:24, caitp wrote:
> On 2016/12/06 19:59:41, caitp wrote:
> > On 2016/12/06 19:56:34, caitp wrote:
> > > On 2016/12/06 19:17:07, Benedikt Meurer wrote:
> > > > I also would like us to fully understand where the performance
difference
> > > comes
> > > > from first. Have you checked the TF generated code?
> > > > 
> > > 
> > > --trace-turbo-reduction seems to indicate that we're replacing the Load
with
> a
> > > mapcheck and constant (627 == HeapConstant for Array `values`),, and
> > eliminating
> > > the ObjectIsReceiver check and CallRuntime. The call to values appears to
> get 
> > > inlined, and it doesn't seem to be possible to turn off inlining from
> > > JSBuiltinReducer)
> > > 
> > > ```
> > > - Replacing 30: Checkpoint(34, 626, 15) with 626: CheckMaps[1](624, 625,
> 624,
> > > 15)
> > > - Replacing 40: IfSuccess(35) with 15: IfSuccess(14)
> > > - Replacing 35: JSCallFunction[2, 1, NOT_NULL_OR_UNDEFINED,
> > > DISALLOW_TAIL_CALLS](627, 3, 1, 43, 626, 15) with 652: FinishRegion(644,
> 651)
> > > - Replacing 44: ObjectIsReceiver(652) with 199:
HeapConstant[0x12e0dd3823b1
> > > <true>]
> > > - Replacing 48: IfFalse(45) with 641: Dead
> > > - Replacing 46: IfTrue(45) with 15: IfSuccess(14)
> > > - Replacing 45: Branch[None](199, 15) with 641: Dead
> > > - Replacing 49: Checkpoint(52, 652, 641) with 641: Dead
> > > - Replacing 53: JSCallRuntime[ThrowSymbolIteratorInvalid, 0](1, 59, 641,
> 641)
> > > with 641: Dead
> > > - Replacing 56: IfSuccess(641) with 641: Dead
> > > ```
> > 
> > I would expect this is similar to the optimization we'd see with the AST
> > desugaring, so... will investigate that
> 
> Yeah, the main difference as far as TF seems to be concerned is, previously
was
> generating `JSLoadProperty[sloppy]`, and now `JSLoadNamed`, but in both cases
> the optimized code replaces them with constants in the same way. They both end
> up generating identical optimized code, so the win seems to come from the
> baseline case.

To clarify, this is measuring a very simple test:

```
function test(iterable) {
    var sum = 0;
    for (var x of iterable) sum += x;
    return sum;
}

if (test([1, 2, 3]) !== 6) throw new TypeError("test failed #1");
if (test([1, 2, 7]) !== 10) throw new TypeError("test failed #2");
%OptimizeFunctionOnNextCall(test);
if (test([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]) !== 55) throw new TypeError("test
failed #3");
```

using `--trace-codegen --trace-opt --trace-deopt --trace-turbo-scheduler
--trace-turbo-reduction --allow-natives-syntax --no-inlining --no-turbo-inlining
--print-opt-code`

[caitp, 2016-12-06 21:07:28]

Description was changed from

==========
[ignition] desugar GetIterator() via bytecode rather than via AST

BUG=v8:4280
==========

to

==========
[ignition] desugar GetIterator() via bytecode rather than via AST

Introduces:
- a new AST node representing the GetIterator() algorithm in the specification,
to be used by ForOfStatement, YieldExpression (in the case of delegating
yield*), and the future `for-await-of` loop proposed in
http://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwr....
- a new opcode (JumpIfJSReceiver), which is useful for `if Type(object) is not
Object` checks which are common throughout the specification. This node is
easily eliminated by TurboFan.


The AST node is desugared specially in bytecode, rather than manually when
building the AST. The benefit of this is that desugaring in the
BytecodeGenerator is much simpler and easier to understand than desugaring the
AST.

This also reduces parse time very slightly, and allows us to use LoadIC rather
than KeyedLoadIC, which seems to have  better baseline performance. This results
in a ~20% improvement in test/js-perf-test/Iterators micro-benchmarks, which I
believe owes to the use of the slightly faster LoadIC as opposed to the
KeyedLoadIC in the baseline case. Both produce identical optimized code via
TurboFan when the type check can be eliminated, and the load can be replaced
with a constant value.

BUG=v8:4280
R=bmeurer@chromium.org, rmcilroy@chromium.org, adamk@chromium.org,
neis@chromium.org, jarin@chromium.org
==========

[caitp, 2016-12-06 21:22:10]

On 2016/12/06 20:14:52, caitp wrote:
> On 2016/12/06 20:13:24, caitp wrote:
> > On 2016/12/06 19:59:41, caitp wrote:
> > > On 2016/12/06 19:56:34, caitp wrote:
> > > > On 2016/12/06 19:17:07, Benedikt Meurer wrote:
> > > > > I also would like us to fully understand where the performance
> difference
> > > > comes
> > > > > from first. Have you checked the TF generated code?
> > > > > 
> > > > 
> > > > --trace-turbo-reduction seems to indicate that we're replacing the Load
> with
> > a
> > > > mapcheck and constant (627 == HeapConstant for Array `values`),, and
> > > eliminating
> > > > the ObjectIsReceiver check and CallRuntime. The call to values appears
to
> > get 
> > > > inlined, and it doesn't seem to be possible to turn off inlining from
> > > > JSBuiltinReducer)
> > > > 
> > > > ```
> > > > - Replacing 30: Checkpoint(34, 626, 15) with 626: CheckMaps[1](624, 625,
> > 624,
> > > > 15)
> > > > - Replacing 40: IfSuccess(35) with 15: IfSuccess(14)
> > > > - Replacing 35: JSCallFunction[2, 1, NOT_NULL_OR_UNDEFINED,
> > > > DISALLOW_TAIL_CALLS](627, 3, 1, 43, 626, 15) with 652: FinishRegion(644,
> > 651)
> > > > - Replacing 44: ObjectIsReceiver(652) with 199:
> HeapConstant[0x12e0dd3823b1
> > > > <true>]
> > > > - Replacing 48: IfFalse(45) with 641: Dead
> > > > - Replacing 46: IfTrue(45) with 15: IfSuccess(14)
> > > > - Replacing 45: Branch[None](199, 15) with 641: Dead
> > > > - Replacing 49: Checkpoint(52, 652, 641) with 641: Dead
> > > > - Replacing 53: JSCallRuntime[ThrowSymbolIteratorInvalid, 0](1, 59, 641,
> > 641)
> > > > with 641: Dead
> > > > - Replacing 56: IfSuccess(641) with 641: Dead
> > > > ```
> > > 
> > > I would expect this is similar to the optimization we'd see with the AST
> > > desugaring, so... will investigate that
> > 
> > Yeah, the main difference as far as TF seems to be concerned is, previously
> was
> > generating `JSLoadProperty[sloppy]`, and now `JSLoadNamed`, but in both
cases
> > the optimized code replaces them with constants in the same way. They both
end
> > up generating identical optimized code, so the win seems to come from the
> > baseline case.
> 
> To clarify, this is measuring a very simple test:
> 
> ```
> function test(iterable) {
>     var sum = 0;
>     for (var x of iterable) sum += x;
>     return sum;
> }
> 
> if (test([1, 2, 3]) !== 6) throw new TypeError("test failed #1");
> if (test([1, 2, 7]) !== 10) throw new TypeError("test failed #2");
> %OptimizeFunctionOnNextCall(test);
> if (test([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]) !== 55) throw new TypeError("test
> failed #3");
> ```
> 
> using `--trace-codegen --trace-opt --trace-deopt --trace-turbo-scheduler
> --trace-turbo-reduction --allow-natives-syntax --no-inlining
--no-turbo-inlining
> --print-opt-code`

----

And, last update on this: when this patch is modified to generate a keyed load
rather than named load for Symbol.iterator, performance is close to (but
slightly worse than) the AST-generated code (slightly worse due to the extra
type-check added for for-of loops that is there in the spec, but unimplemented
in the parser).

Numbers for the Iterators benchmarks with keyed load:

```
EchoBeach2:v8 caitp$ tools/run_perf.py --outdir=out_patched_keyedload/
--outdir-no-patch=out_nopatched/ test/js-perf-test/Iterators.json 
>>> Running suite: Iterators/Iterators
>>> Stdout (#1):
ForOf-Iterators(Score): 294630

>>> Stdout (#1) - without patch:
ForOf-Iterators(Score): 307780

>>> Stdout (#2):
ForOf-Iterators(Score): 290635

>>> Stdout (#2) - without patch:
ForOf-Iterators(Score): 295866

>>> Stdout (#3):
ForOf-Iterators(Score): 286431

>>> Stdout (#3) - without patch:
ForOf-Iterators(Score): 303439
```

[caitp, 2016-12-06 21:56:57]

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

[commit-bot: I haz the power, 2016-12-06 21:57:04]

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-12-06 22:28:56]

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

[commit-bot: I haz the power, 2016-12-06 22:28:58]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-12-06 23:27:36]

Thanks for the investigation. Given that we'd need to do something funny to the
AST-based desugaring anyway if we wanted to change this from a KeyedLoad to a
Load, then this desugaring via a new AST node seems like a totally reasonable
way forward.

lgtm from my end, but please wait for bmeurer's review as well.

[Benedikt Meurer, 2016-12-07 04:53:03]

On 2016/12/06 23:27:36, adamk wrote:
> Thanks for the investigation. Given that we'd need to do something funny to
the
> AST-based desugaring anyway if we wanted to change this from a KeyedLoad to a
> Load, then this desugaring via a new AST node seems like a totally reasonable
> way forward.
> 
> lgtm from my end, but please wait for bmeurer's review as well.

Indeed, fully agreed. LGTM as well.

[neis, 2016-12-07 10:17:27]

nit comments that i forgot to send yesterday (some may have been addressed
already):

https://codereview.chromium.org/2557593004/diff/20001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2557593004/diff/20001/src/ast/ast.h#newcode2925
src/ast/ast.h:2925: // "Property" with additional semantics.
Please make this comment clearer.

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2859: builder()
Please change the comment to reflect that the result is also called.

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:2098: // JumpIfJSReceiverConstant <imm>
s/Constant//

https://codereview.chromium.org/2557593004/diff/20001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2067: // iterator = iterable[Symbol.iterator]()
Please update this comment.

https://codereview.chromium.org/2557593004/diff/20001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:670: NOT_A_PATTERN(GetIterator)
Looks like this list was sorted.

[rmcilroy, 2016-12-07 10:50:42]

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2858: // Let method be ? GetMethod(obj,
@@iterator).
On 2016/12/06 15:14:58, caitp wrote:
> On 2016/12/06 15:02:28, rmcilroy wrote:
> > Not sure what the "?" means here. Can we just remove it?
> 
> It's the spec's short form for "ReturnIfAbrupt(<completion>)", basically just
> means the op can throw. Fine to remove it

Yeah I would rather we remove it (and add the comment about calling it as Georg
mentioned below).

[caitp, 2016-12-07 14:49:41]

I think that's everything... lets flip the switch and see if the 20% improvement
shows up consistently without breaking anything

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:2098: // JumpIfJSReceiverConstant <imm>
On 2016/12/07 10:17:27, neis wrote:
> s/Constant//

Oop, done

https://codereview.chromium.org/2557593004/diff/20001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2067: // iterator = iterable[Symbol.iterator]()
On 2016/12/07 10:17:27, neis wrote:
> Please update this comment.

Done.

https://codereview.chromium.org/2557593004/diff/20001/src/parsing/pattern-rew...
File src/parsing/pattern-rewriter.cc (right):

https://codereview.chromium.org/2557593004/diff/20001/src/parsing/pattern-rew...
src/parsing/pattern-rewriter.cc:670: NOT_A_PATTERN(GetIterator)
On 2016/12/07 10:17:27, neis wrote:
> Looks like this list was sorted.

Done.

[caitp, 2016-12-07 14:49:48]

The CQ bit was checked by caitp@igalia.com

[caitp, 2016-12-07 14:49:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, bmeurer@chromium.org, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2557593004/#ps80001
(title: "georg's comments")

[commit-bot: I haz the power, 2016-12-07 14:49:52]

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-12-07 14:53:42]

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

[commit-bot: I haz the power, 2016-12-07 14:53:44]

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

[caitp, 2016-12-07 14:54:41]

Description was changed from

==========
[ignition] desugar GetIterator() via bytecode rather than via AST

Introduces:
- a new AST node representing the GetIterator() algorithm in the specification,
to be used by ForOfStatement, YieldExpression (in the case of delegating
yield*), and the future `for-await-of` loop proposed in
http://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwr....
- a new opcode (JumpIfJSReceiver), which is useful for `if Type(object) is not
Object` checks which are common throughout the specification. This node is
easily eliminated by TurboFan.


The AST node is desugared specially in bytecode, rather than manually when
building the AST. The benefit of this is that desugaring in the
BytecodeGenerator is much simpler and easier to understand than desugaring the
AST.

This also reduces parse time very slightly, and allows us to use LoadIC rather
than KeyedLoadIC, which seems to have  better baseline performance. This results
in a ~20% improvement in test/js-perf-test/Iterators micro-benchmarks, which I
believe owes to the use of the slightly faster LoadIC as opposed to the
KeyedLoadIC in the baseline case. Both produce identical optimized code via
TurboFan when the type check can be eliminated, and the load can be replaced
with a constant value.

BUG=v8:4280
R=bmeurer@chromium.org, rmcilroy@chromium.org, adamk@chromium.org,
neis@chromium.org, jarin@chromium.org
==========

to

==========
[ignition] desugar GetIterator() via bytecode rather than via AST

Introduces:
- a new AST node representing the GetIterator() algorithm in the specification,
to be used by ForOfStatement, YieldExpression (in the case of delegating
yield*), and the future `for-await-of` loop proposed in
http://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwr....
- a new opcode (JumpIfJSReceiver), which is useful for `if Type(object) is not
Object` checks which are common throughout the specification. This node is
easily eliminated by TurboFan.


The AST node is desugared specially in bytecode, rather than manually when
building the AST. The benefit of this is that desugaring in the
BytecodeGenerator is much simpler and easier to understand than desugaring the
AST.

This also reduces parse time very slightly, and allows us to use LoadIC rather
than KeyedLoadIC, which seems to have  better baseline performance. This results
in a ~20% improvement in test/js-perf-test/Iterators micro-benchmarks, which I
believe owes to the use of the slightly faster LoadIC as opposed to the
KeyedLoadIC in the baseline case. Both produce identical optimized code via
TurboFan when the type check can be eliminated, and the load can be replaced
with a constant value.

BUG=v8:4280
R=bmeurer@chromium.org, rmcilroy@chromium.org, adamk@chromium.org,
neis@chromium.org, jarin@chromium.org
TBR=rossberg@chromium.org
==========

[caitp, 2016-12-07 14:54:53]

The CQ bit was checked by caitp@igalia.com

[commit-bot: I haz the power, 2016-12-07 14:55: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-12-07 15:19:53]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481122493275030,
"parent_rev": "622c673414c77c3eada1815ad86ba22f3f7865c3", "commit_rev":
"75f2f17b80ac03407cd6711dbec9101b8aa830ca"}

[commit-bot: I haz the power, 2016-12-07 15:19:58]

Description was changed from

==========
[ignition] desugar GetIterator() via bytecode rather than via AST

Introduces:
- a new AST node representing the GetIterator() algorithm in the specification,
to be used by ForOfStatement, YieldExpression (in the case of delegating
yield*), and the future `for-await-of` loop proposed in
http://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwr....
- a new opcode (JumpIfJSReceiver), which is useful for `if Type(object) is not
Object` checks which are common throughout the specification. This node is
easily eliminated by TurboFan.


The AST node is desugared specially in bytecode, rather than manually when
building the AST. The benefit of this is that desugaring in the
BytecodeGenerator is much simpler and easier to understand than desugaring the
AST.

This also reduces parse time very slightly, and allows us to use LoadIC rather
than KeyedLoadIC, which seems to have  better baseline performance. This results
in a ~20% improvement in test/js-perf-test/Iterators micro-benchmarks, which I
believe owes to the use of the slightly faster LoadIC as opposed to the
KeyedLoadIC in the baseline case. Both produce identical optimized code via
TurboFan when the type check can be eliminated, and the load can be replaced
with a constant value.

BUG=v8:4280
R=bmeurer@chromium.org, rmcilroy@chromium.org, adamk@chromium.org,
neis@chromium.org, jarin@chromium.org
TBR=rossberg@chromium.org
==========

to

==========
[ignition] desugar GetIterator() via bytecode rather than via AST

Introduces:
- a new AST node representing the GetIterator() algorithm in the specification,
to be used by ForOfStatement, YieldExpression (in the case of delegating
yield*), and the future `for-await-of` loop proposed in
http://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwr....
- a new opcode (JumpIfJSReceiver), which is useful for `if Type(object) is not
Object` checks which are common throughout the specification. This node is
easily eliminated by TurboFan.


The AST node is desugared specially in bytecode, rather than manually when
building the AST. The benefit of this is that desugaring in the
BytecodeGenerator is much simpler and easier to understand than desugaring the
AST.

This also reduces parse time very slightly, and allows us to use LoadIC rather
than KeyedLoadIC, which seems to have  better baseline performance. This results
in a ~20% improvement in test/js-perf-test/Iterators micro-benchmarks, which I
believe owes to the use of the slightly faster LoadIC as opposed to the
KeyedLoadIC in the baseline case. Both produce identical optimized code via
TurboFan when the type check can be eliminated, and the load can be replaced
with a constant value.

BUG=v8:4280
R=bmeurer@chromium.org, rmcilroy@chromium.org, adamk@chromium.org,
neis@chromium.org, jarin@chromium.org
TBR=rossberg@chromium.org
==========

[commit-bot: I haz the power, 2016-12-07 15:19:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-07 15:20:38]

Description was changed from

==========
[ignition] desugar GetIterator() via bytecode rather than via AST

Introduces:
- a new AST node representing the GetIterator() algorithm in the specification,
to be used by ForOfStatement, YieldExpression (in the case of delegating
yield*), and the future `for-await-of` loop proposed in
http://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwr....
- a new opcode (JumpIfJSReceiver), which is useful for `if Type(object) is not
Object` checks which are common throughout the specification. This node is
easily eliminated by TurboFan.


The AST node is desugared specially in bytecode, rather than manually when
building the AST. The benefit of this is that desugaring in the
BytecodeGenerator is much simpler and easier to understand than desugaring the
AST.

This also reduces parse time very slightly, and allows us to use LoadIC rather
than KeyedLoadIC, which seems to have  better baseline performance. This results
in a ~20% improvement in test/js-perf-test/Iterators micro-benchmarks, which I
believe owes to the use of the slightly faster LoadIC as opposed to the
KeyedLoadIC in the baseline case. Both produce identical optimized code via
TurboFan when the type check can be eliminated, and the load can be replaced
with a constant value.

BUG=v8:4280
R=bmeurer@chromium.org, rmcilroy@chromium.org, adamk@chromium.org,
neis@chromium.org, jarin@chromium.org
TBR=rossberg@chromium.org
==========

to

==========
[ignition] desugar GetIterator() via bytecode rather than via AST

Introduces:
- a new AST node representing the GetIterator() algorithm in the specification,
to be used by ForOfStatement, YieldExpression (in the case of delegating
yield*), and the future `for-await-of` loop proposed in
http://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwr....
- a new opcode (JumpIfJSReceiver), which is useful for `if Type(object) is not
Object` checks which are common throughout the specification. This node is
easily eliminated by TurboFan.

The AST node is desugared specially in bytecode, rather than manually when
building the AST. The benefit of this is that desugaring in the
BytecodeGenerator is much simpler and easier to understand than desugaring the
AST.

This also reduces parse time very slightly, and allows us to use LoadIC rather
than KeyedLoadIC, which seems to have  better baseline performance. This results
in a ~20% improvement in test/js-perf-test/Iterators micro-benchmarks, which I
believe owes to the use of the slightly faster LoadIC as opposed to the
KeyedLoadIC in the baseline case. Both produce identical optimized code via
TurboFan when the type check can be eliminated, and the load can be replaced
with a constant value.

BUG=v8:4280
R=bmeurer@chromium.org, rmcilroy@chromium.org, adamk@chromium.org,
neis@chromium.org, jarin@chromium.org
TBR=rossberg@chromium.org

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

[commit-bot: I haz the power, 2016-12-07 15:20:39]

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