349320 - Generator cleanup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

I would like to propose a generator code cleanup that besides making code smaller optimizes  generator operations for the newborn and closed states.

Comment 1

[Igor Bukanov]

Attachment: Implementation v1

Changes:

1. next|send|throw|close methods calls a common dispatcher code, generator_op. That in turn perform various checks and optimizations depending on the generator state. For example, for newborn generators throw just throws the exception to the caller and closes the generator while for the close method the dispatcher just closes the generator.  

2. The generator is registered with GC after the first yield so newborn generators would be swiftly GC-ed.

3. JSGeneratorState is a plain enum again as  the switch in generator_op should properly deal with all the states.

3. js_CloseGeneratorObject calls directly SendToGenerator which transfer control to the generator. In this way the extra checks/setups that the close method has to do are avoided.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 234588 [details] [diff] [review]
Implementation v1

>+    arg = (op == JSGENOP_SEND || op == JSGENOP_THROW)
>+        ? argv[0]
>+        : JSVAL_VOID;

Nit: align the ?: with the opening parenthesis.

>+         * The generator yielded the first time. Register it with GC to ensure
>+         * that suspended finally blocks would be executed.

Nit: s/would/will/.

(s)r=mrbkap

Comment 3

[Brendan Eich [:brendan]]

This is good stuff, I was reviewing with mrbkap before he departed for college.  Marking in a minute.

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 234588 [details] [diff] [review]
Implementation v1

>     /* Store the argument to send as the result of the yield expression. */
>-    gen->frame.sp[-1] = (argc != 0) ? argv[0] : JSVAL_VOID;
>-    gen->state |= JSGEN_RUNNING;
>+    gen->frame.sp[-1] = arg;

There is no such argument stack slot in the case of generator_next, unless you set its JSFunctionSpec.nargs to 1 and not 0.

>+    if (gen->frame.flags & JSFRAME_YIELDING) {
>+        /* Yield can not fail, throw or be called on closing. */

s/can not/cannot/

>+    /* We pass NULL as rval since SendToGenerator never use it with CLOSE. */
>+    return SendToGenerator(cx, JSGENOP_CLOSE, gen->obj, gen, JSVAL_VOID, NULL);

s/use/uses/ and maybe s/NULL/null/ in the comment, to relieve ALLCAPS eyestrain ;-).

>+/*
>+ * Execute generator's close hook after GC detects that the object has become
>+ * unreachable.
>+ */
>+JSBool
>+js_CloseGeneratorObject(JSContext *cx, JSGenerator *gen)

Comment moved, I know, but how about s/generator's/gen's/ and s/after GC/after the GC/ ?

In JSGEN_NEWBORN state case in generator_op:

>+          case JSGENOP_THROW:
>+            JS_SetPendingException(cx, argv[0]);
>+            gen->state = JSGEN_CLOSED;
>+            return JS_FALSE;

This isn't quite the same as the existing jsiter.c implementation, or as Python2.5:

>>> def gen():
...   try:
...     print 'hi'
...     yield 42
...   finally:
...     print 'fin'
... 
>>> it = gen()
>>> it.throw('boo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 1, in gen
boo

The PEP implies (item 3 under "Specification Summary") that even a newborn generator should have the exception thrown at the point it is paused (i.e., at its entry point if it were a function).

>@@ -930,18 +931,24 @@ FindAndMarkObjectsToClose(JSContext *cx,
>     genp = &rt->gcCloseState.reachableList;
>     while ((gen = *genp) != NULL) {
>         if (*js_GetGCThingFlags(gen->obj) & GCF_MARK) {
>             genp = &gen->next;
>         } else {
>             *genp = gen->next;
>             gen->next = NULL;
>             if (gen->state != JSGEN_CLOSED) {
>-                /* Generator cannot be nesting, i.e., running or closing. */
>-                JS_ASSERT(gen->state <= JSGEN_OPEN);
>+                /*
>+                 * Generator cannot be nesting, i.e., running or closing, and
>+                 * newborn generator is never registered with GC.
>+                 *
>+                 * XXX: we do need to run the close hook if the last yield
>+                 * happened outside a try block.

What's the problem here?  Cite a followup bug with FIXME: bug nnnnnn if needed, but I thought this case was handled correctly already.

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> >+                 * XXX: we do need to run the close hook if the last yield
> >+                 * happened outside a try block.
> 
> What's the problem here?  Cite a followup bug with FIXME: bug nnnnnn if needed,
> but I thought this case was handled correctly already.

Never mind, I see bug 349331 comment 10.

/be

Comment 6

[Igor Bukanov]

(In reply to comment #4)
> (From update of attachment 234588 [details] [diff] [review] [edit])
> >     /* Store the argument to send as the result of the yield expression. */
> >-    gen->frame.sp[-1] = (argc != 0) ? argv[0] : JSVAL_VOID;
> >-    gen->state |= JSGEN_RUNNING;
> >+    gen->frame.sp[-1] = arg;
> 
> There is no such argument stack slot in the case of generator_next, unless you
> set its JSFunctionSpec.nargs to 1 and not 0.

The next method just passes JSVAL_VOID to the dispatcher.

> In JSGEN_NEWBORN state case in generator_op:
> 
> >+          case JSGENOP_THROW:
> >+            JS_SetPendingException(cx, argv[0]);
> >+            gen->state = JSGEN_CLOSED;
> >+            return JS_FALSE;
> 
> This isn't quite the same as the existing jsiter.c implementation, or as
> Python2.5:
> 
> >>> def gen():
> ...   try:
> ...     print 'hi'
> ...     yield 42
> ...   finally:
> ...     print 'fin'
> ... 
> >>> it = gen()
> >>> it.throw('boo')
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
>   File "<stdin>", line 1, in gen
> boo
> 
> The PEP implies (item 3 under "Specification Summary") that even a newborn
> generator should have the exception thrown at the point it is paused (i.e., at
> its entry point if it were a function).

But this is only visible from the stack trace which PEP does not define so I though that it would be a good optimization. Is it really necessary?

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> > The PEP implies (item 3 under "Specification Summary") that even a newborn
> > generator should have the exception thrown at the point it is paused (i.e., at
> > its entry point if it were a function).
> 
> But this is only visible from the stack trace which PEP does not define so I
> though that it would be a good optimization. Is it really necessary?
> 

Moreover, this is not even visible in SpiderMonkey through the stack property since the property is recorded when an Error instance is created, not when it is thrown. I.e. the following example

function gen() {
        try {   
                print("hi");
                yield 42;
        } finally {
                print("fin");
        }
}

var iter = gen();
try {   
        iter.throw(new Error());
} catch (e) {
        print(e.stack);
}

continue to print 
Error()@:0
@/home/igor/s/x.js:12
even if iter.throw() enters the newborn generator.

In any case, temporary entering the generator on newborn throw makes code smaller so I will change the patch not to treat that case specially.   

Comment 8

[Igor Bukanov]

Attachment: Implementation v2

This version addresses the nits and removes that optimization for generatorInstance.throw() when generatorInstance is a newborn.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 235396 [details] [diff] [review]
Implementation v2


>     /* Store the argument to send as the result of the yield expression. */
>-    gen->frame.sp[-1] = (argc != 0) ? argv[0] : JSVAL_VOID;
>-    gen->state |= JSGEN_RUNNING;
>+    gen->frame.sp[-1] = arg;

My comment was misstated -- this is a pre-existing bug in the case of the first .next() (or .send(undefined)) call -- gen->frame.sp starts out at the bottom of the operand stack space, so this -1 indexes into the generating-pc "basement".  After the generator is open, JSOP_YIELD keeps the stack slot for its operand alive for its result (from send), so this is exactly right.

To fix the initial state, js_NewGenerator should set sp to newsp + depth + 1.

/be

Comment 10

[Igor Bukanov]

Attachment: Implementation v3

In this version sp[-1] is only touched when send or next is executed on yielding generator.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 235431 [details] [diff] [review]
Implementation v3


> generator_finalize(JSContext *cx, JSObject *obj)
> {
>     JSGenerator *gen;
> 
>     gen = (JSGenerator *) JS_GetPrivate(cx, obj);
>-    if (gen)
>+    if (gen) {
>+        /*
>+         * gen can be open on shutdown when close hooks are ignored or when
>+         * the embedding cancels scheduled close hooks.
>+         */
>+        JS_ASSERT(gen->state == JSGEN_NEWBORN || gen->state == JSGEN_CLOSED
>+                  || gen->state == JSGEN_OPEN);

Standard style has || at end of line for multiline conditions.

Looks great otherwise -- r=me with that nit picked.

/be

Comment 12

[Brendan Eich [:brendan]]

Prerequisite for blocking bug 349331.

/be

Comment 13

[Mike Schroepfer]

plusing since we are generally trying to take all the JS fixes for 1.8...

Comment 14

[Igor Bukanov]

Attachment: Implementation v3b

Patch to commit with nits addressed

Comment 15

[Igor Bukanov]

I committed the patch from comment 14 to the trunk:

Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.171; previous revision: 3.170
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.56; previous revision: 3.55
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.273; previous revision: 3.272
done
Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.35; previous revision: 3.34
done
Checking in jsiter.h;
/cvsroot/mozilla/js/src/jsiter.h,v  <--  jsiter.h
new revision: 3.12; previous revision: 3.11
done

Comment 16

[Mike Schroepfer]

Are we getting close to wrapping up JS changes?  FF2 final is approaching and we don't have much time left to address any possible regressions.

Comment 17

[Brendan Eich [:brendan]]

We are close to done.  This cleanup is important as a prerequisite for more significant changes that finalize semantics, mainly to eliminate GeneratorExit (something Pythonistas are thinking about eliminating in a future version of Python, thanks to Igor making the case).

This cleanup is mostly refactoring and state re-numbering with consequent logic changes to shrink code and reduce extra tests and transitions through useless states.  The state machine is not changing in detectable ways, but the code is better and easier to understand.

/be

Comment 18

[Mike Schroepfer]

Comment on attachment 235546 [details] [diff] [review]
Implementation v3b

a=schrep/beltnzer for drivers.

Comment 19

[Igor Bukanov]

I committed the patch from comment 14 to the MOZILLA_1_8_BRANCH:

Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.17; previous revision: 3.104.2.16
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.33.4.5; previous revision: 3.33.4.4
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.47; previous revision: 3.181.2.46
done
Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.17.2.11; previous revision: 3.17.2.10
done
Checking in jsiter.h;
/cvsroot/mozilla/js/src/jsiter.h,v  <--  jsiter.h
new revision: 3.6.2.6; previous revision: 3.6.2.5
done