Implement iterator result boxing in ES6 generators

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: wingo, Assigned: wingo)

Tracking

({dev-doc-complete})

unspecified
mozilla26
x86_64
Linux
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 793504 [details] [diff] [review]
Implement iterator result boxing in ES6 generators

This patchset causes the parser to yield boxed return values in ES6
generators, generally of the form { value: foo, done: bool }.  However
if value is undefined, that property is omitted.  If done is false, that
property is omitted.

If the generator function does not end in a return, the parser inserts a
"return {done:true}" statement at the end.

When an ES6 generator finishes, it does so with {done:true} instead of
throwing a StopIteration.  As the behavior of legacy and star generators
are diverging here, I forked the iterator method implementations.
(Assignee)

Updated

5 years ago
Assignee: general → wingo
(Assignee)

Updated

5 years ago
Attachment #793504 - Flags: review?(jwalden+bmo)
Attachment #793504 - Flags: review?(jorendorff)
15.19.4.3.4 CreateItrResultObject always creates own data properties "value" and "done". Why omit them from the result object if value is undefined or done is false?
(Assignee)

Comment 3

5 years ago
Hi André,

(In reply to André Bargull from comment #2)
> 15.19.4.3.4 CreateItrResultObject always creates own data properties "value"
> and "done". Why omit them from the result object if value is undefined or
> done is false?

ES6 bug https://bugs.ecmascript.org/show_bug.cgi?id=1799.  We can include them always if needed.  Though it seems that the spec does specify that they are there, no other part of the spec relies on that.  Thus they aren't really necessary, and it could be that not adding them when not needed is a perf gain.  (It could be that it's not a perf gain, of course...)
This is a bit problematic because the object created in CreateItrResultObject inherits from Object.prototype which means "value" or "done" on Object.prototype can mix up things.
(Assignee)

Comment 5

5 years ago
(In reply to André Bargull from comment #4)
> This is a bit problematic because the object created in
> CreateItrResultObject inherits from Object.prototype which means "value" or
> "done" on Object.prototype can mix up things.

Hum, a good point!  Adding them is probably the right thing to do.  Unless they shouldn't inherit from Object.prototype at all?

The other reason I didn't add them was there seems to be no way to get at the undefined value from within the parser.  A mundane reason, I guess I'll poke at that some more.
(In reply to Andy Wingo from comment #5)
> The other reason I didn't add them was there seems to be no way to get at
> the undefined value from within the parser.  A mundane reason, I guess I'll
> poke at that some more.

Do you mean `undefined` as in `void 0`? Or generally speaking `void <literal>`.
(Assignee)

Comment 7

5 years ago
(In reply to André Bargull from comment #6)
> Do you mean `undefined` as in `void 0`? Or generally speaking `void
> <literal>`.

Yes I do.  I had forgotten about this hack; I'll use it!  (When fabricating the value for a "return;" in a generator.)
(Assignee)

Comment 8

5 years ago
Created attachment 794047 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v2

This patchset causes the parser to yield boxed return values in ES6
generators, generally of the form { value: foo, done: bool }.  However
if value is undefined, that property is omitted.  If done is false, that
property is omitted.

If the generator function does not end in a return, the parser inserts a
"return {done:true}" statement at the end.

When an ES6 generator finishes, it does so with {done:true} instead of
throwing a StopIteration.  As the behavior of legacy and star generators
are diverging here, I forked the iterator method implementations.
(Assignee)

Updated

5 years ago
Attachment #793504 - Attachment is obsolete: true
Attachment #793504 - Flags: review?(jwalden+bmo)
Attachment #793504 - Flags: review?(jorendorff)
(Assignee)

Comment 9

5 years ago
Comment on attachment 794047 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v2

Seems I need to edit the commit log -- this patch always adds "value" and "done" properties to an iterator result object.
Attachment #794047 - Attachment description: Implement iterator result boxing in ES6 generators → Implement iterator result boxing in ES6 generators v2
Attachment #794047 - Flags: review?(jwalden+bmo)
Attachment #794047 - Flags: review?(jorendorff)
Comment on attachment 793504 [details] [diff] [review]
Implement iterator result boxing in ES6 generators

Review of attachment 793504 [details] [diff] [review]:
-----------------------------------------------------------------

Andy, here's the partial review I had half-done before you posted the revised patch. Please skip over the parts you already addressed; feel free to fix any others; and I'll look again tomorrow!

::: js/src/frontend/Parser.cpp
@@ +4536,5 @@
> +    Node literal = handler.newObjectLiteral(pos().begin);
> +    if (!literal)
> +        return null();
> +
> +    if (value) {

As we discussed, this should always define both .value and .done; and please add a test that requires both to be present.

@@ +4647,5 @@
>              return null();
>  
> +        exprNode = iteratorResult(exprNode, false);
> +        if (!exprNode)
> +            return null();

Instead of putting this into the AST, which seems like it would mess up Reflect.parse, please emit the code for CreateItrResultObject in the bytecode emitter. Same for returnStatement().

(This also avoids the issue of how to represent undefined in the AST. You don't have to -- just emit JSOP_VOID in the emitter.)

Do we not have Reflect.parse tests for the new syntax? We should!

...Or (thinking out loud) you could even put this in SendToGenerator. It's hard for me to guess if that's better or worse for subsequent optimization. In the immediate short term, it'll run faster. But another issue with SendToGenerator is I imagine you're going to rewrite it from scratch pretty soon...

::: js/src/jsiter.cpp
@@ +1019,5 @@
> +{
> +    if (!IsGenerator(v))
> +        return false;
> +    JSGenerator *gen = v.toObject().as<GeneratorObject>().getGenerator();
> +    return gen->regs.fp()->script()->isStarGenerator();

Nothing wrong here, but this is something like 6 chained loads. Can we just give star generators a different js::Class? (You can do this in a follow-up bug, if so.)

@@ +1650,1 @@
>      gen->fp->clearReturnValue();

This is weird. (Pre-existing weirdness, not yours.) Shouldn't the return value always be undefined at this point, even if an error occurred?

If you assert that and run tests, what breaks?

You don't have to pursue this if you're not curious. :) I just suspect this isn't necessary and prefer tight assertions.

@@ +1678,5 @@
> +
> +    if (gen->state == JSGEN_NEWBORN && args.hasDefined(0)) {
> +        RootedValue val(cx, args[0]);
> +        js_ReportValueError(cx, JSMSG_BAD_GENERATOR_SEND,
> +                            JSDVG_SEARCH_STACK, val, NullPtr());

This reminds me. The message for JSMSG_BAD_GENERATOR_SEND in js.msg seems unclear to me. It uses the verb "send" and doesn't mention that "next" is the method that threw the error. Could you fix it?

@@ +1797,1 @@
>  }

If these methods are all going to throw when invoked on the opposite kind of Generator, then I think it's especially sensible to give them two distinct js::Class objects.

::: js/src/tests/ecma_6/Generators/iteration.js
@@ +18,5 @@
> +        msg = "Assertion failed: expected " + ctor + ", but no exception thrown";
> +    throw new Error(msg + " - " + str);
> +}
> +
> +function assertEqual(found, expected) {

Instead of this, consider copying existing functions from js/src/jit-test/lib/asserts.js into js/src/tests/shell.js. That will help keep our tests more uniform.

There's already an assertEqual in there (though I had to call it assertDeepEq; not enough schemers around here to support different things named assertEq and assertEqual, alas).

There's also assertThrowsInstanceOf().

In any case, utility code like this belongs in a shell.js file somewhere, not in the test file itself. (Use js/src/tests/tests.py to run the test, with -s -o to show exact command lines and test output; a bit cumbersome, but on the upside a quick shell alias will pay for itself within the hour.)

@@ +59,5 @@
> +    assertEqual(result.value, value);
> +    if (done)
> +        assertTruthy(result.done);
> +    else
> +        assertFalsey(result.done);

Let's make this more precise. The spec requires exactly this, I think:

    function assertIteratorResult(result, value, done) {  // also note (actual, expected) order
        assertDeepEq(result, {value: value, done: done});
    }

@@ +75,5 @@
> +    assertEq(Object.getPrototypeOf(result), Object.prototype);
> +    property_names = Object.getOwnPropertyNames(result);
> +    property_names.sort();
> +    assertEqual(property_names, ["value"]);
> +    assertIteratorResult(1, false, result);

Using assertDeepEq() as suggested above pretty much subsumes all of these other checks.
Comment on attachment 794047 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v2

Clearing review flag. This can wait until Andy has a chance to incorporate the other changes (bugzilla says he's out of the office today).
Attachment #794047 - Flags: review?(jorendorff)
(Assignee)

Comment 12

5 years ago
Hi,

Thanks again for the review and apologies for updating the patch while you were looking at it.  I'll be more careful in the future.

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> ::: js/src/frontend/Parser.cpp
> @@ +4647,5 @@
> >              return null();
> >  
> > +        exprNode = iteratorResult(exprNode, false);
> > +        if (!exprNode)
> > +            return null();
> 
> Instead of putting this into the AST, which seems like it would mess up
> Reflect.parse, please emit the code for CreateItrResultObject in the
> bytecode emitter. Same for returnStatement().

Sure, I can do that.  We'll do yield* there too.

> Do we not have Reflect.parse tests for the new syntax? We should!

Filed a FIXME at bug 909329.

> ...Or (thinking out loud) you could even put this in SendToGenerator. It's
> hard for me to guess if that's better or worse for subsequent optimization.
> In the immediate short term, it'll run faster. But another issue with
> SendToGenerator is I imagine you're going to rewrite it from scratch pretty
> soon...

Dunno, I think you really want this visible to the baseline compiler (when that comes).  You can do more sensible allocation there, you get inline caches for the property initializations, etc.  And yes, I think SendToGenerator needs to be written in a different way to get baseline compilation working.

> @@ +1650,1 @@
> >      gen->fp->clearReturnValue();
> 
> This is weird. (Pre-existing weirdness, not yours.) Shouldn't the return
> value always be undefined at this point, even if an error occurred?
> 
> If you assert that and run tests, what breaks?

Good question!  I just tried that, but a whole bunch of jit-tests fail (33 of them).  I didn't run the reftests.  A project for another day, methinks :)

> @@ +1678,5 @@
> > +
> > +    if (gen->state == JSGEN_NEWBORN && args.hasDefined(0)) {
> > +        RootedValue val(cx, args[0]);
> > +        js_ReportValueError(cx, JSMSG_BAD_GENERATOR_SEND,
> > +                            JSDVG_SEARCH_STACK, val, NullPtr());
> 
> This reminds me. The message for JSMSG_BAD_GENERATOR_SEND in js.msg seems
> unclear to me. It uses the verb "send" and doesn't mention that "next" is
> the method that threw the error. Could you fix it?

What kind of message are you looking for here?  "Send" is the right verb to my eye, and you'll see "next" in the stack trace -- and on ES6 generators at least there is only next and throw, so there is little ambiguity.

> If these methods are all going to throw when invoked on the opposite kind of
> Generator, then I think it's especially sensible to give them two distinct
> js::Class objects.

Very much agreed; that would be bug 908920 I suppose.

> ::: js/src/tests/ecma_6/Generators/iteration.js
> @@ +18,5 @@
> > +        msg = "Assertion failed: expected " + ctor + ", but no exception thrown";
> > +    throw new Error(msg + " - " + str);
> > +}
> > +
> > +function assertEqual(found, expected) {
> 
> Instead of this, consider copying existing functions from
> js/src/jit-test/lib/asserts.js into js/src/tests/shell.js. That will help
> keep our tests more uniform.

Unhappily, I can't do that :(  Those functions are written using JS 1.8, and the ecma_6 tests use the default version of JS.

I'll do something to make their names the same.

Will post an updated patch based on the one in bug 908920.
(Assignee)

Comment 13

5 years ago
Created attachment 795510 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v3

This patchset causes the bytecode emitter to yield boxed return values
in ES6 generators, of the form { value: foo, done: bool }.

If the generator function does not end in a return, the compiler inserts
the equivalent of a "return {done:true}" statement at the end.

When an ES6 generator finishes, it does so with {done:true} instead of
throwing a StopIteration.

This patch also ports lib/asserts.js to "default version" JS, and ports
ecma_6/Generators to use it.  I figured it should go there instead of
the toplevel, to avoid slowing down the rest of the tests.
(Assignee)

Updated

5 years ago
Attachment #794047 - Attachment is obsolete: true
Attachment #794047 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 14

5 years ago
Comment on attachment 795510 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v3

Attached patch addresses all the nits, I think.  There's some templatey refactorings in jsiter.cpp that probably merit a try build; I'll get someone to kick one off.
Attachment #795510 - Attachment description: Implement iterator result boxing in ES6 generators → Implement iterator result boxing in ES6 generators v3
Attachment #795510 - Flags: review?(jorendorff)
(Assignee)

Comment 15

5 years ago
Comment on attachment 795510 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v3

Review of attachment 795510 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1808,5 @@
> +        return UINT_MAX;
> +
> +    ObjectBox *objbox = bce->parser->newObjectBox(obj);
> +    if (!objbox)
> +        return false;

Gah!  This should return UINT_MAX.  Will fix in the next patch.

@@ +2626,5 @@
>  
>      if (!EmitTree(cx, bce, body))
>          return false;
>  
> +    // If we fell of the end of an ES6 generator, return a boxed iterator result

off, obviously
(Assignee)

Comment 16

5 years ago
I was wondering if all that template stuff would fly, so here's a tinderbox:

https://tbpl.mozilla.org/?tree=Try&rev=ace1a57b4551

Seems OK so far.
(Assignee)

Comment 17

5 years ago
Created attachment 797158 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v4

This patchset causes the bytecode emitter to yield boxed return values
in ES6 generators, of the form { value: foo, done: bool }.

If the generator function does not end in a return, the compiler inserts
the equivalent of a "return { value: undefined, done:true }" statement
at the end.

When an ES6 generator finishes, it does so with {done:true} instead of
throwing a StopIteration.

This patch also ports lib/asserts.js to "default version" JS, and ports
ecma_6/Generators to use it.  I figured it should go there instead of
the toplevel, to avoid slowing down the rest of the tests.
(Assignee)

Updated

5 years ago
Attachment #795510 - Attachment is obsolete: true
Attachment #795510 - Flags: review?(jorendorff)
(Assignee)

Comment 18

5 years ago
Comment on attachment 797158 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v4

Rebased, and some fixes that were identified by the tighter generator frame clearing in bug 908280.  Please take a look!

Next up is yield*.
Attachment #797158 - Attachment description: Implement iterator result boxing in ES6 generators → Implement iterator result boxing in ES6 generators v4
Attachment #797158 - Flags: review?(jorendorff)
(Assignee)

Comment 19

5 years ago
I seem to not be able to type bug numbers.  I meant "bug 908920".
I'm looking. Not going to finish tonight but I'll keep plugging for another hour.
It's over half tests, so I started with the tests. The rest should be easy, right?

In tests/ecma_6/Generators/iteration.js:
>+var GeneratorFunction = (function*(){yield 1;}).__proto__.constructor;

.__proto__ is unnecessary here.

>+    assertEq(Object.getPrototypeOf(result), Object.prototype);
>+    property_names = Object.getOwnPropertyNames(result);
>+    property_names.sort();
>+    assertDeepEq(property_names, ["done", "value"]);
>+    assertIteratorResult(1, false, result);

Everything except the last line is superfluous; assertIteratorResult
checks all those things.

>+    assertEq(Object.getPrototypeOf(result), Object.prototype);
>+    property_names = Object.getOwnPropertyNames(result);
>+    property_names.sort();
>+    assertDeepEq(property_names, ["done", "value"]);
>+    assertIteratorResult(undefined, true, result);

Same here.

>+TestGeneratorResultPrototype()

Style micro-nit: semicolon, please.

In TestGenerator(), in testThrow():
>+            function Sentinel() {}

Since this function is not immediately inside the outermost block of the
enclosing function, it's nonstandard. Works fine, but please use this
instead:

    var Sentinel = function () {};

The other Sentinel functions are fine.

>+        assertThrowsInstanceOf(function() { iter.next(); }, Error);

All these might as well use the specific error that is thrown.
(TypeError, I think, per
<https://people.mozilla.com/~jorendorff/es6-draft.html#sec-15.19.4.3.2>.)

In TestTryCatch:
>+    function Test7(iter) {
>+        assertIteratorResult(1, false, iter.next());
>+        assertIteratorResult(2, false, iter.next());
>+        assertIteratorResult(3, false, iter.next());
>+        assertIteratorResult(undefined, true, iter.next());
>+        assertThrowsInstanceOf(function() { iter.next(); }, Error);
>+    }
>+    Test7(instantiate(g));

This is the same as Test1.

In TestTryFinally:
>+    function Test8(iter) {
>+        assertIteratorResult(1, false, iter.next());
>+        assertIteratorResult(2, false, iter.next());
>+        assertIteratorResult(3, false, iter.next());
>+        assertIteratorResult(4, false, iter.next());
>+        assertIteratorResult(undefined, true, iter.next());
>+        assertThrowsInstanceOf(function() { iter.next(); }, Error);
>+    }
>+    Test8(instantiate(g));

This is the same as Test1.

In tests/ecma_6/Generators/runtime.js:
> function assertSyntaxError(str) {
>     var msg;
>+    // Non-direct eval.
>     var evil = eval;
>+    assertThrowsInstanceOf(function() { evil(str) }, SyntaxError);
> }

Incidentally, you can use Function(code), rather than indirect
eval. It's slightly better, because you're additionally asserting that
it's an early error (thrown whether the code runs or not).

In tests/ecma_6/shell.js:
>+            function assertSameProto(a, b, msg) {
>+                check(Object_getPrototypeOf(a), Object_getPrototypeOf(b), at(msg, ".__proto__"))
>+            };

Style nit: Please put a semicolon after check() and remove the extra one after "}" :)

In both copies, of course. :-P

Actually, try changing jit-test/lib/asserts.js to do
    load(libdir + "../../tests/ecma_6/shell.js");
just to see if we can manage with a single copy. If not, it's OK.

(I don't know that there's any point removing the arrow functions. That
syntax is allowed in all versions of JS and I'm pretty sure it's not
going anywhere. But it's OK.)
(Assignee)

Comment 22

5 years ago
Thanks for the review!

Bizarre that I managed to get duplicate tests in there.  Will take a look.  Will also take a look to see how to define the assertDeepEq in just one place.

I'll post an updated patch once the other half of the review comes in.
Comment on attachment 797158 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v4

Review of attachment 797158 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these comments, plus the comments above, addressed.

In frontend::BytecodeEmitter.cpp, EmitFinishIteratorResult:
>+{
>+    jsatomid value_id;
>+    if (!bce->makeAtomIndex(cx->names().value, &value_id))
>+        return UINT_MAX;
>+    jsatomid done_id;
>+    if (!bce->makeAtomIndex(cx->names().done, &done_id))
>+        return UINT_MAX;

These should return false, not UINT_MAX!

It's pretty common, but not obligatory, to use an out-param instead, and
make the return type bool (success/failure) when the result isn't a
pointer. Emit1 is the exception that proves why it's a good rule... :-P
Filed bug 912125 to fix that maybe someday.

In frontend::BytecodeEmitter.cpp, frontend::EmitFunctionScript:
>+    // If we fell off the end of an ES6 generator, return a boxed iterator
>+    // result object of the form { value: undefined, done: true }.

Grammar nit: "If we fall off", since the hypothetical is situated in the
future.

>+        if (!EmitFinishIteratorResult(cx, bce, true))
>+            return false;
>+        // No need to check for finally blocks, etc as in EmitReturn.

Style nit: blank line before this comment please.

In jsiter.cpp:
>     JS_ResolveStub,
>     JS_ConvertStub,
>-    generator_finalize,
>+    FinalizeGenerator<LegacyGeneratorObject>,
>     NULL,                    /* checkAccess */
>     NULL,                    /* call        */
>     NULL,                    /* hasInstance */
>     NULL,                    /* construct   */
>-    generator_trace,
>+    TraceGenerator<LegacyGeneratorObject>,

I wouldn't have bothered templatizing these two, as I think the code
reads a little better as it is.

I think I might also have left the implementations of next and throw
commoned up (but using the NativeMethod template to make the JS-visible
methods properly non-generic), because that way you can see what the
differences in behavior are between legacy and star generators without
diffing.

But if you like it better this way, or it's not worth changing back,
that's fine with me.  Let's move ahead...

>+            JS_ASSERT(generatorKind == LegacyGenerator);
>+            // Otherwise we discard the return value and throw a StopIteration
>+            // if needed.
>+            rval.setUndefined();

Nit: Blank line before this comment.
Attachment #797158 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 24

5 years ago
Created attachment 799350 [details] [diff] [review]
Implement iterator result boxing in ES6 generators v5

Updated patch addressing nits.

Removing arrow functions was a well-intentioned mistake -- for some reason I thought arrow functions were not in default-version JS.  I should have left them as-is.  But as you say, it doesn't matter, so rather than introducing more nits to fix I'll leave it as-is.

Regarding templatization, I agree that the e.g. TraceGenerator<LegacyGenerator> instantiations themselves are a wash, but the win was to allow me to get rid of IsGenerator and GetGenerator.  It also makes the assertions in the trace/finalize procedures tighter -- no need to check for a star generator when finalizing a legacy generator, for example.  It does duplicate the code body but these methods are very short and the templatization enables some folding, so I suspect no change in overall size.
Attachment #797158 - Attachment is obsolete: true
Attachment #799350 - Flags: review+
(Assignee)

Comment 25

5 years ago
Patch seems to apply cleanly to mozilla-central; setting checkin-needed.  r=jorendorff.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/02b05f0a7227
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Keywords: dev-doc-needed
Noticed this breaks running the jit-tests on a remote device because
js/src/jit-test/lib/asserts.js needs ../../tests/ecma_6/shell.js
which is not pushed to the remote device.

It is trivial to push tests/ecma_6/shell.j to the remote device to
work around this.  Do you think this would be an appropriate fix?
(In reply to Douglas Crosher [:dougc] from comment #28)
> It is trivial to push tests/ecma_6/shell.j to the remote device to
> work around this.  Do you think this would be an appropriate fix?

Yes. The "shell.js" files in various subdirectories are (potentially) important for running the tests and should all be copied over.
Whiteboard: [DocArea=JS]
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.