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)

x86
Windows XP
defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: duncan.loveday, Assigned: igor)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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.
Keywords: regression
Blocks: 379758
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Hoping igor can take this. /be
It would be good to unregress this before 1.9a5 freezes (tonight PDT?). /be
Assignee: general → igor
(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?
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.
(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
Attached patch fix v1 (deleted) — Splinter Review
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 on attachment 266560 [details] [diff] [review] fix v1 r=me, thanks. /be
Attachment #266560 - Flags: review?(brendan) → review+
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.
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
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
(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
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-382335.js,v <-- regress-382335.js initial revision: 1.1
Flags: in-testsuite+
verified fixed 1.9.0 linux/mac*/windows.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: