Closed Bug 349320 Opened 14 years ago Closed 14 years ago

Generator cleanup

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: fixed1.8.1, Whiteboard: [schrep-181approval pending])

Attachments

(1 file, 3 obsolete files)

I would like to propose a generator code cleanup that besides making code smaller optimizes  generator operations for the newborn and closed states.
Attached patch Implementation v1 (obsolete) — Splinter Review
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.
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Attachment #234588 - Flags: superreview?(mrbkap)
Attachment #234588 - Flags: review?(brendan)
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
Attachment #234588 - Flags: superreview?(mrbkap) → superreview+
This is good stuff, I was reviewing with mrbkap before he departed for college.  Marking in a minute.

/be
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
(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
(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?
(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.   

Attached patch Implementation v2 (obsolete) — Splinter Review
This version addresses the nits and removes that optimization for generatorInstance.throw() when generatorInstance is a newborn.
Attachment #234588 - Attachment is obsolete: true
Attachment #235396 - Flags: review?(brendan)
Attachment #234588 - Flags: review?(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
Attached patch Implementation v3 (obsolete) — Splinter Review
In this version sp[-1] is only touched when send or next is executed on yielding generator.
Attachment #235396 - Attachment is obsolete: true
Attachment #235431 - Flags: review?(brendan)
Attachment #235396 - Flags: review?(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
Attachment #235431 - Flags: review?(brendan) → review+
Prerequisite for blocking bug 349331.

/be
Blocks: geniter, 349331
Flags: blocking1.8.1?
plusing since we are generally trying to take all the JS fixes for 1.8...
Flags: blocking1.8.1? → blocking1.8.1+
Patch to commit with nits addressed
Attachment #235431 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #235546 - Flags: approval1.8.1?
Flags: in-testsuite-
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.
Whiteboard: [schrep-181approval pending]
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 on attachment 235546 [details] [diff] [review]
Implementation v3b

a=schrep/beltnzer for drivers.
Attachment #235546 - Flags: approval1.8.1? → approval1.8.1+
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
Keywords: fixed1.8.1
Depends on: 350559
You need to log in before you can comment on or make changes to this bug.