Leaving a generator via an exception causes it to close

Leaving a generator via an exception causes it to close

R=rossberg@chromium.org
BUG=v8:3096
LOG=Y

Committed: https://code.google.com/p/v8/source/detail?r=25297

[wingo, 2014-11-12 11:53:21]

I looked into doing this in assembly, if there were a faster way, but no.  We
just have to turbofan %_GeneratorResume it seems.

[rossberg, 2014-11-12 11:56:27]

https://codereview.chromium.org/717123002/diff/1/src/generator.js
File src/generator.js (right):

https://codereview.chromium.org/717123002/diff/1/src/generator.js#newcode40
src/generator.js:40: } catch (e) {
Wait, won't this catch the very throw above?

[wingo, 2014-11-12 12:03:02]

https://codereview.chromium.org/717123002/diff/1/src/generator.js
File src/generator.js (right):

https://codereview.chromium.org/717123002/diff/1/src/generator.js#newcode40
src/generator.js:40: } catch (e) {
On 2014/11/12 11:56:27, rossberg wrote:
> Wait, won't this catch the very throw above?

It could, if the generator itself doesn't catch it.   But the generator could
catch it and return a value.

[rossberg, 2014-11-12 12:04:07]

lgtm

[wingo, 2014-11-12 12:04:11]

On 2014/11/12 11:53:21, wingo wrote:
> I looked into doing this in assembly, if there were a faster way, but no.  We
> just have to turbofan %_GeneratorResume it seems.

FWIW I see no perf impact on a microbenchmark.

[wingo, 2014-11-12 12:08:23]

This patch is incorrect, as it could close a generator that is executing.

var g = (function* () { yield g.next() })();
g.next()

[wingo, 2014-11-12 12:54:18]

Andreas, I added state checks to the JS.  I seem to recall some concern that
throwing the exception directly from the next() or throw() functions wouldn't do
the right thing, but I don't see how that would be; perhaps I am ignorant.  If
this approach works for you I'll update the patch to remove the corresponding
checks from the full-codegen generators.

Note that adding this runtime call reduces our performance by 33% on a
microbenchmark (from 30ms for a loop fetching 1e6 results from a generator to
40ms).  We can inline this if needed; dunno where and whether this investment is
worth it.

[rossberg, 2014-11-12 12:59:38]

On 2014/11/12 12:54:18, wingo wrote:
> Andreas, I added state checks to the JS.  I seem to recall some concern that
> throwing the exception directly from the next() or throw() functions wouldn't
do
> the right thing, but I don't see how that would be; perhaps I am ignorant.  If
> this approach works for you I'll update the patch to remove the corresponding
> checks from the full-codegen generators.
> 
> Note that adding this runtime call reduces our performance by 33% on a
> microbenchmark (from 30ms for a loop fetching 1e6 results from a generator to
> 40ms).  We can inline this if needed; dunno where and whether this investment
is
> worth it.

LGTM. (I don't recall that concern, do you remember in what context that came
up?)

[wingo, 2014-11-12 13:04:27]

On 2014/11/12 12:59:38, rossberg wrote:
> On 2014/11/12 12:54:18, wingo wrote:
> > I seem to recall some concern that
> > throwing the exception directly from the next() or throw() functions
wouldn't
> do
> > the right thing, but I don't see how that would be; perhaps I am ignorant.
> 
> LGTM. (I don't recall that concern, do you remember in what context that came
> up?)

I think it was this: https://codereview.chromium.org/136003003/#msg5  But from
what I read, we are good here.

[rossberg, 2014-11-12 13:12:46]

rossberg@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[rossberg, 2014-11-12 13:13:55]

On 2014/11/12 13:04:27, wingo wrote:
> On 2014/11/12 12:59:38, rossberg wrote:
> > On 2014/11/12 12:54:18, wingo wrote:
> > > I seem to recall some concern that
> > > throwing the exception directly from the next() or throw() functions
> wouldn't
> > do
> > > the right thing, but I don't see how that would be; perhaps I am ignorant.
> > 
> > LGTM. (I don't recall that concern, do you remember in what context that
came
> > up?)
> 
> I think it was this: https://codereview.chromium.org/136003003/#msg5  But from
> what I read, we are good here.

Michi, want to comment on that?

[wingo, 2014-11-12 13:55:56]

OK, final version removes the assembly code that checks states, yay.  PTAL if
you don't mind; sorry for the multiple round-trips here.

[wingo, 2014-11-12 13:56:38]

On 2014/11/12 13:55:56, wingo wrote:
> OK, final version removes the assembly code that checks states, yay.  PTAL if
> you don't mind; sorry for the multiple round-trips here.

To test I built arm, arm64, ia32, and x64 debug binaries and ran them on the
mjsunit/generators tests.

[rossberg, 2014-11-12 14:14:45]

LGTM

[wingo, 2014-11-12 14:29:13]

Committed patchset #3 (id:40001) manually as 25297 (presubmit successful).