Move generator-close on exception from the generator function to the GeneratorResume builtin.

Move generator-close on exception from the generator function to the GeneratorResume builtin.

The change also moves creation of the iterator result from the parser to the bytecode generator.

Unfortunately, async generators will stay on the old scheme (try-finally around generator body) because I am not exactly sure how they work.

Review-Url: https://codereview.chromium.org/2917263002
Cr-Commit-Position: refs/heads/master@{#45713}
Committed: https://chromium.googlesource.com/v8/v8/+/7fa77063cf98cddf5ee721df7279b0f0c8ebf0d3

[Jarin, 2017-06-03 18:23:05]

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

[commit-bot: I haz the power, 2017-06-03 18:23:41]

Dry run: CQ is trying da patch.

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

[Jarin, 2017-06-03 18:27:03]

Description was changed from

==========
Move generator-close on exception from the generator function to the
GeneratorResume builtin.
==========

to

==========
Move generator-close on exception from the generator function to the
GeneratorResume builtin.

The change also moves creation of the iterator result from the parser to the
bytecode generator.

Unfortunately, async generators will stay on the old scheme (try-finally around
generator body) because I am not exactly sure how they work.
==========

[Jarin, 2017-06-03 18:27:04]

jarin@chromium.org changed reviewers:
+ adamk@chromium.org, neis@chromium.org, rmcilroy@chromium.org

[commit-bot: I haz the power, 2017-06-03 18:42:07]

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

[commit-bot: I haz the power, 2017-06-03 18:42: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...)

[Jarin, 2017-06-03 19:32:10]

Could you take a look, please?

[Jarin, 2017-06-03 19:35:06]

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

[commit-bot: I haz the power, 2017-06-03 19:35:08]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-03 20:07:53]

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

[commit-bot: I haz the power, 2017-06-03 20:07:54]

Dry run: This issue passed the CQ dry run.

[caitp, 2017-06-03 22:54:17]

> Unfortunately, async generators will stay on the old scheme (try-finally
around generator body) because I am not exactly sure how they work.

When I saw this in my bugmail, I was wondering if it would be possible to work
in async generators as well.

So, for closing the generator on exception, it should be fairly straight forward
from AsyncGeneratorResumeNext: handle exception -> set the generator state to
closed -> continue draining the AsyncGeneratorRequest queue.

For closing the generator on normal `return` statements, I think you can just
check if the generator state is not suspended after resuming the generator.

If you want to try it here, it seems workable to me. If not, I can give it a try
next week.

[caitp, 2017-06-03 23:14:00]

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

[caitp, 2017-06-03 23:14:00]

https://codereview.chromium.org/2917263002/diff/40001/src/builtins/builtins-g...
File src/builtins/builtins-generator-gen.cc (right):

https://codereview.chromium.org/2917263002/diff/40001/src/builtins/builtins-g...
src/builtins/builtins-generator-gen.cc:57: GotoIfException(result,
&if_exception, &var_exception);
also, given what I said in the top-level comment, I think you also need to
handle normal returns here, too.

If, after this stub call, the generator state is still "Executing", you can set
the state to complete before returning the value.

Another thing: we could shrink the bytecode by creating the IteratorResult in
the resume stub at this point, rather than in bytecode. (`{ value:
stubReturnValue, done: generator state ==JSGeneratorObject::kExecuting }`).

[Jarin, 2017-06-04 08:49:39]

I also moved creation of IteratorResult for final return to the GeneratorResume
builtin. 

I would prefer not to do the async generator work in this CL as there is much
higher risk that I will get that wrong.

https://codereview.chromium.org/2917263002/diff/40001/src/builtins/builtins-g...
File src/builtins/builtins-generator-gen.cc (right):

https://codereview.chromium.org/2917263002/diff/40001/src/builtins/builtins-g...
src/builtins/builtins-generator-gen.cc:57: GotoIfException(result,
&if_exception, &var_exception);
On 2017/06/03 23:14:00, caitp wrote:
> also, given what I said in the top-level comment, I think you also need to
> handle normal returns here, too.

At the moment normal returns are handled in BytecodeGenerator, so it should not
be wrong. 

I agree that having all the closing logic in one place sounds better, but this
somehow breaks modules.  

> If, after this stub call, the generator state is still "Executing", you can
set
> the state to complete before returning the value.
> 
> Another thing: we could shrink the bytecode by creating the IteratorResult in
> the resume stub at this point, rather than in bytecode. (`{ value:
> stubReturnValue, done: generator state ==JSGeneratorObject::kExecuting }`).

Done.

[Jarin, 2017-06-04 08:56:47]

https://codereview.chromium.org/2917263002/diff/40001/src/builtins/builtins-g...
File src/builtins/builtins-generator-gen.cc (right):

https://codereview.chromium.org/2917263002/diff/40001/src/builtins/builtins-g...
src/builtins/builtins-generator-gen.cc:57: GotoIfException(result,
&if_exception, &var_exception);
On 2017/06/03 23:14:00, caitp wrote:
> Another thing: we could shrink the bytecode by creating the IteratorResult in
> the resume stub at this point, rather than in bytecode. (`{ value:
> stubReturnValue, done: generator state ==JSGeneratorObject::kExecuting }`).

Note that we can only do this for normal returns because yields coming from
yield* must preserve the identity of the IteratorResult. It would be against the
spec to unwrap and wrap the value from yield*.

[Jarin, 2017-06-04 11:59:07]

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

[commit-bot: I haz the power, 2017-06-04 11:59: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, 2017-06-04 12:30:07]

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

[commit-bot: I haz the power, 2017-06-04 12:30:08]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-06-05 10:02:58]

Nice, LGTM.

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

https://codereview.chromium.org/2917263002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2190: Register result =
register_allocator()->NewRegister();
nit - add a comment that we need to close the generator before returning.

https://codereview.chromium.org/2917263002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2559: } else if
(expr->IsNonInitialGeneratorYield()) {
nit - let's put this above expr->IsNonInitialAsyncGeneratorYield() (since it's
the more general case)

[Jarin, 2017-06-05 15:38:18]

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

[commit-bot: I haz the power, 2017-06-05 15:38:26]

Dry run: CQ is trying da patch.

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

[Jarin, 2017-06-05 15:59:58]

Patchset #6 (id:100001) has been deleted

[Jarin, 2017-06-05 16:03:25]

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

https://codereview.chromium.org/2917263002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2190: Register result =
register_allocator()->NewRegister();
On 2017/06/05 10:02:58, rmcilroy wrote:
> nit - add a comment that we need to close the generator before returning.

Done.

https://codereview.chromium.org/2917263002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2559: } else if
(expr->IsNonInitialGeneratorYield()) {
On 2017/06/05 10:02:58, rmcilroy wrote:
> nit - let's put this above expr->IsNonInitialAsyncGeneratorYield() (since it's
> the more general case)

Done.

[Jarin, 2017-06-05 18:18:55]

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

[commit-bot: I haz the power, 2017-06-05 18:19:03]

Dry run: CQ is trying da patch.

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

[adamk, 2017-06-05 18:37:37]

lgtm % register scope (unless I'm missing something)

https://codereview.chromium.org/2917263002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2917263002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:2191: Register result =
register_allocator()->NewRegister();
Should there be a RegisterAllocationScope here?

[commit-bot: I haz the power, 2017-06-05 18:50:42]

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

[commit-bot: I haz the power, 2017-06-05 18:50:43]

Dry run: This issue passed the CQ dry run.

[Jarin, 2017-06-05 19:24:39]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2017-06-05 19:24:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2917263002/#ps160001
(title: "Add register allocation scope")

[commit-bot: I haz the power, 2017-06-05 19:24:45]

CQ is trying da patch.

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

[rmcilroy, 2017-06-05 19:28:42]

https://codereview.chromium.org/2917263002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2917263002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:2191: Register result =
register_allocator()->NewRegister();
On 2017/06/05 18:37:36, adamk wrote:
> Should there be a RegisterAllocationScope here?

Each statement is in its own RegisterAllocationScope, and given we are at the
very end of a return statement, there is no need for a new scope here.

[commit-bot: I haz the power, 2017-06-05 19:54:15]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496690679832620,
"parent_rev": "e5e55c61c7b9c37444980fa6620a1a2c36f66656", "commit_rev":
"7fa77063cf98cddf5ee721df7279b0f0c8ebf0d3"}

[commit-bot: I haz the power, 2017-06-05 19:54:22]

Description was changed from

==========
Move generator-close on exception from the generator function to the
GeneratorResume builtin.

The change also moves creation of the iterator result from the parser to the
bytecode generator.

Unfortunately, async generators will stay on the old scheme (try-finally around
generator body) because I am not exactly sure how they work.
==========

to

==========
Move generator-close on exception from the generator function to the
GeneratorResume builtin.

The change also moves creation of the iterator result from the parser to the
bytecode generator.

Unfortunately, async generators will stay on the old scheme (try-finally around
generator body) because I am not exactly sure how they work.

Review-Url: https://codereview.chromium.org/2917263002
Cr-Commit-Position: refs/heads/master@{#45713}
Committed:
https://chromium.googlesource.com/v8/v8/+/7fa77063cf98cddf5ee721df7279b0f0c8e...
==========

[commit-bot: I haz the power, 2017-06-05 19:54:24]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/7fa77063cf98cddf5ee721df7279b0f0c8e...