Generators return boxed values

Generators return boxed values

Generators now box their return values in object literals of the form

  { value: VAL, done: DONE }

where DONE is false for yield expressions, and true for return
statements.

BUG=v8:2355
TEST=mjsunit/harmony/generators-iteration
R=mstarzinger@chromium.org

Committed: 14563

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

The attached patch applies on top of the "Generators can resume" patch,
https://codereview.chromium.org/14066016/.  The code itself only needs the
"generators can suspend" patch, but the tests need the ability to resume
generators.

[wingo, 2013-04-29 13:03:55]

Updated patch applies to master and adds needed write barrier.  Tested on ia32,
arm, and x64.

[Michael Starzinger, 2013-05-06 11:22:32]

Going into the right direction. First round of comments. The comments for ia32
probably apply to other architectures as well.

https://codereview.chromium.org/13870007/diff/6001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/13870007/diff/6001/src/bootstrapper.cc#newcod...
src/bootstrapper.cc:1399:
STATIC_ASSERT(JSGeneratorObject::kResultValuePropertyIndex == 0);
Is there a particular reason why cannot use the index constant in the calls
below instead of using static-asserts and hard-coded constants? Same applies for
the second index below and the count above.

https://codereview.chromium.org/13870007/diff/6001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/13870007/diff/6001/src/contexts.h#newcode325
src/contexts.h:325: ITERATOR_RESULT_MAP_INDEX,
Let's rename this to GENERATOR_RESULT_MAP_INDEX as it is only used for
generators internally. The result of arbitrary iterators might use a completely
different map.

https://codereview.chromium.org/13870007/diff/6001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/13870007/diff/6001/src/ia32/full-codegen-ia32...
src/ia32/full-codegen-ia32.cc:2037: const int size = 5 * kPointerSize;
The size should be determined by
isolate()->native_context()->iterator_result_map()->instance_size() instead of a
hard-coded constant. Or at least we need to assert that this value matches out
expectation.

https://codereview.chromium.org/13870007/diff/6001/src/ia32/full-codegen-ia32...
src/ia32/full-codegen-ia32.cc:2046: done
Use factory->ToBoolean(done) here.

https://codereview.chromium.org/13870007/diff/6001/src/ia32/full-codegen-ia32...
src/ia32/full-codegen-ia32.cc:2049: __ mov(FieldOperand(eax, 0 * kPointerSize),
ebx);  // map
Same comment as in bootstrapper.cc applies. Can we use the constant directly?

[wingo, 2013-05-06 14:43:15]

Thanks for the review.  Updated patch addresses your comments.

I kept a few assertions as to the total size or field count of the generator
result, because a change to that size should result in a different number of
clauses in the code, not just a different position to write a value.  I can
remove those checks if that's the right thing, but keeping them seemed
beneficial.

[Michael Starzinger, 2013-05-06 19:26:05]

Already looking good. One final comment, then this should be ready to go.

https://codereview.chromium.org/13870007/diff/13001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/13870007/diff/13001/src/bootstrapper.cc#newco...
src/bootstrapper.cc:1392: Handle<Map> generator_result_map =
factory()->CopyMap(object_map,
Can we add one dedicated test-case to generators-iteration.js that checks the
prototype and the layout of the returned object is sane? I was thinking along
the lines of ...

assertSame(Object.prototype, Object.getPrototypeOf(result));
assertEquals(["value", "done"], Object.getOwnPropertyNames(result));

Just having one single test that checks that is enough. Adding this to every
test-case would be overkill.

[Michael Starzinger, 2013-05-06 19:34:18]

One more nit.

https://codereview.chromium.org/13870007/diff/13001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/13870007/diff/13001/src/x64/full-codegen-x64....
src/x64/full-codegen-x64.cc:1943: Smi::FromInt(0));
Is there a particular reason you went back to a hard-coded 0 here instead of the
constant?

[wingo, 2013-05-07 07:42:45]

Thanks for review.  Updated patch addresses comments.

Cheers :)

[Michael Starzinger, 2013-05-07 08:05:51]

LGTM.

[wingo, 2013-05-07 08:46:53]

Committed patchset #5 manually as r14563 (presubmit successful).