Closed
Bug 382335
Opened 17 years ago
Closed 17 years ago
Trampolining threads using generators and iterators is broken - recent regression
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: duncan.loveday, Assigned: igor)
References
()
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070528 Minefield/3.0a5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070528 Minefield/3.0a5pre
Neil Mix's threading example at the attached URL no longer works in the latest trunk. Worked fine no more than a couple of weeks ago.
Reproducible: Always
Steps to Reproduce:
1. Go to the attached URL
2. Click "start a thread now"
3.
Actual Results:
Nothing happens
Expected Results:
A thread should be started that counts up to 10. This works on trunk builds from a couple of weeks ago.
Reporter | ||
Updated•17 years ago
|
Keywords: regression
Comment 1•17 years ago
|
||
Regression range is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1179993000&maxdate=1179995159
So this is probably caused by Bug 379758.
Comment 2•17 years ago
|
||
Hoping igor can take this.
/be
Comment 3•17 years ago
|
||
It would be good to unregress this before 1.9a5 freezes (tonight PDT?).
/be
Assignee | ||
Updated•17 years ago
|
Assignee: general → igor
Comment 4•17 years ago
|
||
(In reply to comment #1)
> Regression range is
> http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1179993000&maxdate=1179995159
> So this is probably caused by Bug 379758.
>
Ria, I can confirm that I see the test work in a 2007-05-23-04 linux nightly but not in a 2007-05-24-04 linux nightly, but Brendan thinks the regression is from
<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fjs%2Fsrc&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-17+18%3A00%3A00&maxdate=2007-05-17+19%3A00%3A00&cvsroot=%2Fcvsroot>
Can you double check the regression range to make sure we have the right regressor?
Assignee | ||
Comment 5•17 years ago
|
||
Here is a trivial test case:
~/m/trunk/mozilla/js/src $ cat ~/s/Thread.js
function make_gen() { yield 1; }
var gen2 = make_gen();
gen2.next();
gen2.close();
print(10);
~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js -v 170 ~/s/Thread.js
uncaught exception: true
If one comments out gen2.close(), then the example works.
Comment 6•17 years ago
|
||
(In reply to comment #4)
> Ria, I can confirm that I see the test work in a 2007-05-23-04 linux nightly
> but not in a 2007-05-24-04 linux nightly, but Brendan thinks the regression is
> from
> <http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fjs%2Fsrc&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-17+18%3A00%3A00&maxdate=2007-05-17+19%3A00%3A00&cvsroot=%2Fcvsroot>
>
> Can you double check the regression range to make sure we have the right
> regressor?
Confusion: I was talking about bug 382355, which was caused by the patch for bug 380237. This bug's number differs only in the penultimate digit!
/be
Assignee | ||
Comment 7•17 years ago
|
||
During the refactoring done for bug 379758 I forgot that the closing pseudo exception for a generator must be cleared even when there is no try notes.
Attachment #266560 -
Flags: review?(brendan)
Comment 8•17 years ago
|
||
Comment on attachment 266560 [details] [diff] [review]
fix v1
r=me, thanks.
/be
Attachment #266560 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 9•17 years ago
|
||
After spending 4 hours trying to debug the example my conslusion is rather anti-generator one:
1. JS code that explicitly calls generator method is extremely fragile and hard to follow.
2. JS code that uses generators with for-in loops can be replaced by explicit lambdas or functions without loss of functinality and readability as any loop like
for (i in iterator)
body;
can be written in the following style:
iterator(f);
function f(i) body;
Thus all those extra efforts for the parser and runtime support for generators is just a bloat to support writing unmaintainable code.
Assignee | ||
Comment 10•17 years ago
|
||
I committed the patch from comment 7 to the trunk:
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c
new revision: 3.349; previous revision: 3.348
done
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 11•17 years ago
|
||
(In reply to comment #9)
> After spending 4 hours trying to debug the example my conslusion is rather
> anti-generator one:
>
> 1. JS code that explicitly calls generator method is extremely fragile and hard
> to follow.
Maybe, but generators help avoid explicit state save/restore, or even more difficult for average programmers continuation passing style (which risks stack overflow without proper tail call guarantees).
Don't judge only by this example, esp. from your view of it in gdb!
> 2. JS code that uses generators with for-in loops can be replaced by explicit
> lambdas or functions without loss of functinality and readability as any loop
> like
>
> for (i in iterator)
> body;
>
> can be written in the following style:
>
> iterator(f);
> function f(i) body;
That's true only if you are willing to turn your generator function inside out and make it save state explicitly, or in its activation/continuation.
> Thus all those extra efforts for the parser and runtime support for generators
> is just a bloat to support writing unmaintainable code.
Judging from one example is risky.
Consider the Sudoku solver from Bug 380237.
/be
Comment 12•17 years ago
|
||
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-382335.js,v <-- regress-382335.js
initial revision: 1.1
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•