Status

()

--
enhancement
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({fixed1.8.1})

Trunk
fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [schrep-181approval pending])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

12 years ago
I would like to propose a generator code cleanup that besides making code smaller optimizes  generator operations for the newborn and closed states.
(Assignee)

Comment 1

12 years ago
Created attachment 234588 [details] [diff] [review]
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.
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
(Assignee)

Comment 6

12 years ago
(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?
(Assignee)

Comment 7

12 years ago
(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.   

(Assignee)

Comment 8

12 years ago
Created attachment 235396 [details] [diff] [review]
Implementation v2

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
(Assignee)

Comment 10

12 years ago
Created attachment 235431 [details] [diff] [review]
Implementation v3

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: 326466, 349331
Flags: blocking1.8.1?

Comment 13

12 years ago
plusing since we are generally trying to take all the JS fixes for 1.8...
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 14

12 years ago
Created attachment 235546 [details] [diff] [review]
Implementation v3b

Patch to commit with nits addressed
Attachment #235431 - Attachment is obsolete: true
(Assignee)

Comment 15

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #235546 - Flags: approval1.8.1?

Updated

12 years ago
Flags: in-testsuite-

Comment 16

12 years ago
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 18

12 years ago
Comment on attachment 235546 [details] [diff] [review]
Implementation v3b

a=schrep/beltnzer for drivers.
Attachment #235546 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 19

12 years ago
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

Updated

12 years ago
Depends on: 350559
You need to log in before you can comment on or make changes to this bug.