Generators can resume

Generators can resume

The generator object methods "next", "send", and "throw" now
include some inline assembly to set up a resumed stack frame.  In some
common cases, we can just jump back into the frame to resume it.
Otherwise the resume code calls out to a runtime to fill in the operand
stack, rewind the handlers, and possibly to throw an exception.

BUG=v8:2355
TESTS=mjsunit/harmony/generators-iteration

Committed: http://code.google.com/p/v8/source/detail?r=14415

[wingo, 2013-04-16 15:25:47]

This is an early patch that I am pushing out for comments.  Still to be
done:

  * Generator object states (running, closed, etc)
  * Boxing of yielded values (will come in another patch)
  * Tests
  * ARM port

Depends on https://codereview.chromium.org/14262004/ and
https://codereview.chromium.org/13704010/.

[wingo, 2013-04-17 13:57:32]

Updated patch removes the close() function, implements throw(), and correctly
tracks generator states (suspended, running, or finished).  Attempting to resume
a generator that is not in the suspended state will fail.  Also there are a
bunch of tests now.

It also adds a final yield() to a generator's AST, to mark the generator object
as done.

Still to do: boxing the return value (separate patch, I think) and an ARM port.

Comments welcome.

[wingo, 2013-04-17 15:39:04]

Updated patch removes changes that were incorporated into r14309 and
https://codereview.chromium.org/13704010/.

[wingo, 2013-04-18 10:55:23]

Updated patch adds untested ARM port. Now ready for review, once
https://codereview.chromium.org/13704010 sorts itself out.

[wingo, 2013-04-19 15:34:31]

Updated patch applies against master and uses the new sentinel values.

[Michael Starzinger, 2013-04-21 22:45:21]

First round of comments. Mainly the test-coverage should be increased. I left
comments about some concrete test-cases that are missing in the
generator-iteration.js file.

https://codereview.chromium.org/14066016/diff/23002/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/14066016/diff/23002/src/full-codegen.h#newcod...
src/full-codegen.h:489: void EmitResume(Expression *generator_object, Expression
*value,
Let's rename this helper to EmitGeneratorResume() and add a short one-liner
comment that this is "Platform-specific support for generators".

https://codereview.chromium.org/14066016/diff/23002/src/ia32/full-codegen-ia3...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/14066016/diff/23002/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:1945: __ mov(ebx, result_register());
The ebx register might get clobbered while evaluating the value expression of
the resume below. This will be hard to find if someone changes the usage of
%_GeneratorSend in the runtime code. Use the stack to preserve generator object.

https://codereview.chromium.org/14066016/diff/23002/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:1954: __ SmiUntag(edx);
No need to untag and compare, we can compare right against a Smi. Also we should
be able to use the FieldOperand directly in the compare, just one instruction.

https://codereview.chromium.org/14066016/diff/23002/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/14066016/diff/23002/src/messages.js#newcode162
src/messages.js:162: generator_running:             ["Generator is already
running"],
Since generators throw Error objects for now, let's move this to the top into
the section for Error objects (this is the section for SyntaxError).

https://codereview.chromium.org/14066016/diff/23002/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/14066016/diff/23002/src/objects-debug.cc#newc...
src/objects-debug.cc:424: STATIC_ASSERT(JSGeneratorObject::kGeneratorExecuting
<= 0);
Move these static asserts into the runtime functions right to where the
assumption is actually needed. This will also make the comment obsolete.

https://codereview.chromium.org/14066016/diff/23002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/14066016/diff/23002/src/objects.h#newcode6334
src/objects.h:6334: kSend,
style: Enums use all-uppercase names without the "k" prefix. Also the
declaration should fit into one line.

https://codereview.chromium.org/14066016/diff/23002/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/14066016/diff/23002/src/runtime.cc#newcode2491
src/runtime.cc:2491: JSGeneratorObject::ResumeMode resume_mode =
Move the "resume_mode" local variable down to right before the switch. Local
code FTW. The CONVERT_SMI_ARG_CHECKED should stay here though.

https://codereview.chromium.org/14066016/diff/23002/src/runtime.cc#newcode2498
src/runtime.cc:2498: byte* pc =
generator_object->function()->code()->instruction_start();
Use "Address" instead of "byte*" here.

https://codereview.chromium.org/14066016/diff/23002/src/runtime.cc#newcode2510
src/runtime.cc:2510: default:
Drop the default case for enums.

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
File test/mjsunit/harmony/generators-iteration.js (right):

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-iteration.js:59: 
nit: Two newlines between the test cases makes it easier to read. Also two more
occurrences below.

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-iteration.js:104: function* g() { yield
iter.next(); }
This throws only because "iter" is undefined, not because of the recursion. Make
"iter" a local variable in TestNextRecurion().

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-iteration.js:108: function* g() { yield
iter.send(42); }
Likewise.

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-iteration.js:112: function* g() { yield
iter.throw(1); }
Likewise.

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-iteration.js:115:
assertThrows(TestNextRecursion, Error);
Add a third argument to these three assertThrows checking the specific type of
the Error object.

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-iteration.js:119: TestRecursion();
The existing test cases look good. But can we add additional test cases (i.e.
not modify existing ones) to cover the following cases:

- generator function takes arguments and accessed them after resume
- mismatch between formal parameter count and number of arguments
- force a GC within the generator with the --expose-gc flag
- generator accessing receiver:
    function* g() { yield this.x; yield this.y; }
    var o = { start:g; x:1, y:2 };
    var iter = o.start();
    assertEquals(1, iter.next());
    assertEquals(2, iter.next());
- generator called as constructor accessing receiver:
    function* g() { yield this.x; yield this.y; }
    var iter = new g; iter.x = 1; iter.y = 2;
    assertEquals(1, iter.next());
    assertEquals(2, iter.next());

Should any of these tests fail, then I am fine with disabling them for now and
fixing the problem in a follow-up CL.

[wingo, 2013-04-23 13:51:04]

Updated patch addresses comments, except the assertThrows() comment which I
didn't fully understand.  Also, access to "this" in a generator currently does
not work; the test is skipped.  A follow-on CL will fix that up.  PTAL :)

https://codereview.chromium.org/14066016/diff/23002/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/14066016/diff/23002/src/full-codegen.h#newcod...
src/full-codegen.h:489: void EmitResume(Expression *generator_object, Expression
*value,
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> Let's rename this helper to EmitGeneratorResume() and add a short one-liner
> comment that this is "Platform-specific support for generators".

Done.

https://codereview.chromium.org/14066016/diff/23002/src/ia32/full-codegen-ia3...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/14066016/diff/23002/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:1945: __ mov(ebx, result_register());
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> The ebx register might get clobbered while evaluating the value expression of
> the resume below. This will be hard to find if someone changes the usage of
> %_GeneratorSend in the runtime code. Use the stack to preserve generator
object.

Good catch!  Fixed.

https://codereview.chromium.org/14066016/diff/23002/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:1954: __ SmiUntag(edx);
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> No need to untag and compare, we can compare right against a Smi. Also we
should
> be able to use the FieldOperand directly in the compare, just one instruction.

Done.

https://codereview.chromium.org/14066016/diff/23002/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/14066016/diff/23002/src/messages.js#newcode162
src/messages.js:162: generator_running:             ["Generator is already
running"],
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> Since generators throw Error objects for now, let's move this to the top into
> the section for Error objects (this is the section for SyntaxError).

Done.

https://codereview.chromium.org/14066016/diff/23002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/14066016/diff/23002/src/objects.h#newcode6334
src/objects.h:6334: kSend,
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> style: Enums use all-uppercase names without the "k" prefix. Also the
> declaration should fit into one line.

Done.

https://codereview.chromium.org/14066016/diff/23002/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/14066016/diff/23002/src/runtime.cc#newcode2491
src/runtime.cc:2491: JSGeneratorObject::ResumeMode resume_mode =
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> Move the "resume_mode" local variable down to right before the switch. Local
> code FTW. The CONVERT_SMI_ARG_CHECKED should stay here though.

Done.

https://codereview.chromium.org/14066016/diff/23002/src/runtime.cc#newcode2498
src/runtime.cc:2498: byte* pc =
generator_object->function()->code()->instruction_start();
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> Use "Address" instead of "byte*" here.

Done.

https://codereview.chromium.org/14066016/diff/23002/src/runtime.cc#newcode2510
src/runtime.cc:2510: default:
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> Drop the default case for enums.

Done, though I still had to have an UNREACHABLE at the function's end to get the
compiler to be happy.

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
File test/mjsunit/harmony/generators-iteration.js (right):

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-iteration.js:104: function* g() { yield
iter.next(); }
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> This throws only because "iter" is undefined, not because of the recursion.
Make
> "iter" a local variable in TestNextRecurion().

Good catch; fixed this and the two following cases below.

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-iteration.js:115:
assertThrows(TestNextRecursion, Error);
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> Add a third argument to these three assertThrows checking the specific type of
> the Error object.

The error that is thrown is Error, which does not seem to have a ".type"
property.  Should we be throwing something more specific?

https://codereview.chromium.org/14066016/diff/23002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-iteration.js:119: TestRecursion();
On 2013/04/21 22:45:21, Michael Starzinger wrote:
> The existing test cases look good. But can we add additional test cases (i.e.
> not modify existing ones) to cover the following cases:
> 
> - generator function takes arguments and accessed them after resume
> - mismatch between formal parameter count and number of arguments
> - force a GC within the generator with the --expose-gc flag
> - generator accessing receiver:
>     function* g() { yield this.x; yield this.y; }
>     var o = { start:g; x:1, y:2 };
>     var iter = o.start();
>     assertEquals(1, iter.next());
>     assertEquals(2, iter.next());
> - generator called as constructor accessing receiver:
>     function* g() { yield this.x; yield this.y; }
>     var iter = new g; iter.x = 1; iter.y = 2;
>     assertEquals(1, iter.next());
>     assertEquals(2, iter.next());
> 
> Should any of these tests fail, then I am fine with disabling them for now and
> fixing the problem in a follow-up CL.

Good ideas all.  The receiver tests that I added are not working and so are
skipped; presumably we'll need to capture the receiver explicitly in a new slot
in the generator object.

[Michael Starzinger, 2013-04-24 12:08:43]

LGTM. Just one minor nit. I'll land this once the nit is addressed. Awesome
work.

https://codereview.chromium.org/14066016/diff/49001/src/ia32/full-codegen-ia3...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/14066016/diff/49001/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:2020: __ int3();
Use "__ Abort(some_message)" here, that's easier to debug when hit.

[wingo, 2013-04-24 12:21:03]

Thanks for the review :)  Updated patch fixes nit.

https://codereview.chromium.org/14066016/diff/49001/src/ia32/full-codegen-ia3...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/14066016/diff/49001/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:2020: __ int3();
On 2013/04/24 12:08:43, Michael Starzinger wrote:
> Use "__ Abort(some_message)" here, that's easier to debug when hit.

Done.

[Michael Starzinger, 2013-04-24 13:00:29]

Committed patchset #13 manually as r14415 (presubmit successful).