[esnext] prototype runtime implementation for async functions

[esnext] prototype runtime implementation for async functions

BUG=v8:4483
LOG=N
R=littledan@chromium.org, adamk@chromium.org

Committed: https://crrev.com/d08c0304c5779223d6c468373af4815ec3ccdb84
Cr-Commit-Position: refs/heads/master@{#36263}

[caitp (gmail), 2016-04-16 02:19:40]

I hacked together a (partial) prototype today

In order to share more with the generators impl, this adds a hack to
YieldExpression, so that async functions can be spawned via the first `yield`
expression (which ordinarily returns the Generator object). This is pretty
awful, but it simplifies the desugaring a bit (avoiding creation of an inner
function and all of the extra scoping and context management that would go with
that).

There is a lot of stuff not here (or just flat out broken):

1. `throw` resumption is completely broken at the moment, and I'm a bit stumped
on how to fix it.
2. AsyncFunction constructor is not present at the moment
3. async arrow functions don't work at all currently, for whatever reason the
rewriting doesn't seem to affect them, but haven't looked into why yet.

but, it's a quick prototype, I'm sure it can be incrementally improved on.

[caitp (gmail), 2016-04-16 02:50:58]

Patchset #3 (id:40001) has been deleted

[caitp (gmail), 2016-04-16 02:54:04]

Patchset #3 (id:60001) has been deleted

[caitp (gmail), 2016-04-16 02:58:41]

Patchset #2 (id:20001) has been deleted

[caitp (gmail), 2016-04-16 18:17:27]

On 2016/04/16 02:19:40, caitp wrote:
> I hacked together a (partial) prototype today
> 
> In order to share more with the generators impl, this adds a hack to
> YieldExpression, so that async functions can be spawned via the first `yield`
> expression (which ordinarily returns the Generator object). This is pretty
> awful, but it simplifies the desugaring a bit (avoiding creation of an inner
> function and all of the extra scoping and context management that would go
with
> that).
> 
> There is a lot of stuff not here (or just flat out broken):
> 
> 1. `throw` resumption is completely broken at the moment, and I'm a bit
stumped
> on how to fix it.

Hmmmm, this seems to be an off-by-1 error, caused by the wrapper try/finally
block added by the parser. Maybe Andy or someone knows how to fix the off-by-one
in a better way. For now, I've just gone with not adding the try/finally wrapper
for async functions

> 2. AsyncFunction constructor is not present at the moment
> 3. async arrow functions don't work at all currently, for whatever reason the
> rewriting doesn't seem to affect them, but haven't looked into why yet.
> 
> but, it's a quick prototype, I'm sure it can be incrementally improved on.

[caitp (gmail), 2016-04-16 18:20:20]

https://codereview.chromium.org/1895603002/diff/140001/src/builtins.cc
File src/builtins.cc (left):

https://codereview.chromium.org/1895603002/diff/140001/src/builtins.cc#oldcod...
src/builtins.cc:2331: JSGeneratorObject::ResumeMode resume_mode, char const*
const method_name) {
None of these changes are really needed, admittedly. will probably remove these
before landing

[caitp (gmail), 2016-04-16 18:33:03]

(self review)

https://codereview.chromium.org/1895603002/diff/140001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1895603002/diff/140001/src/contexts.h#newcode145
src/contexts.h:145: V(PROMISE_CREATE_REJECTED_INDEX, JSFunction,
promise_create_rejected)       \
admittedly, PromiseCreateResolved doesn't really need a context slot

https://codereview.chromium.org/1895603002/diff/140001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1895603002/diff/140001/src/d8.cc#newcode891
src/d8.cc:891: isolate->RunMicrotasks();
This is useful for testing the async-await-basic.js tests (it tries to emulate
JSC's `drainMicrotasks()`, which helps test this async stuff in a synchronous
way). Worth having, imo

https://codereview.chromium.org/1895603002/diff/140001/src/full-codegen/x64/f...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1895603002/diff/140001/src/full-codegen/x64/f...
src/full-codegen/x64/full-codegen-x64.cc:1839:
DCHECK_EQ(result_register().code(), rax.code());
Not sure if this really adds anything, I guess it's nice to avoid pointless
stack pushing

https://codereview.chromium.org/1895603002/diff/140001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1895603002/diff/140001/src/js/promise.js#newc...
src/js/promise.js:454: function(value) { %GlobalPrint("resume normal...\n");
return AsyncFunctionResume(generator, value, AsyncFunctionNext); },
phantom edits from some debugging =)

https://codereview.chromium.org/1895603002/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1895603002/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:4545: ZoneList<Expression*>* arguments =
Bunch of phantom edits to clean up, since the async function rewriting is moved
to a separate branch...

[caitp (gmail), 2016-04-16 20:09:31]

a few more bits fixed up

https://codereview.chromium.org/1895603002/diff/180001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1895603002/diff/180001/src/frames.cc#newcode870
src/frames.cc:870: if (code ==
*function()->GetIsolate()->builtins()->AsyncFunctionNext() ||
other than the off-by-one bug mentioned and fixed above, this is the remaining
issue with throw resumption. 

Ideally, these functions wouldn't show up in a stack trace at all.

Currently, the stack trace for exceptions thrown within an async function are a
bit weird, since they include the "AsyncFunctionStart" stack entry, which is
serialized as the async function itself (so you can end up with duplicate stack
trace entries, which is not a good look).

[caitp (gmail), 2016-04-26 22:00:56]

The CQ bit was checked by caitpotter88@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-26 22:01:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895603002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895603002/340001

[caitp (gmail), 2016-04-26 22:01:31]

https://codereview.chromium.org/1895603002/diff/200001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1895603002/diff/200001/src/parsing/parser.cc#...
src/parsing/parser.cc:4496:
DCHECK_NOT_NULL(function_state_->generator_object_variable());
these don't really add much, but since they're used more than once, it's good to
have. Saves a few lines of duplicate code at least

https://codereview.chromium.org/1895603002/diff/300001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1895603002/diff/300001/src/factory.cc#newcode...
src/factory.cc:2070: shared->DisableOptimization(kGenerator);
Seems to be okay to not do this. Yield disables optimization anyways, but
without an AwaitExpression, these seem to work fine

https://codereview.chromium.org/1895603002/diff/300001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1895603002/diff/300001/src/frames.cc#newcode867
src/frames.cc:867: // TODO(caitp): these builtins cause crashes here --- that's
not good!
unfortunately still needed in Option C, for reasons I don't understand .____.

https://codereview.chromium.org/1895603002/diff/300001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1895603002/diff/300001/src/isolate.cc#newcode342
src/isolate.cc:342: if (IsAsyncFunction(fun->shared()->kind())) return false;
not needed anymore in Option C

[commit-bot: I haz the power, 2016-04-26 22:04:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-26 22:04:26]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/4886)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/4886)

[caitp (gmail), 2016-04-26 22:37:05]

On 2016/04/26 22:04:26, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/4886)
>   v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/4886)

bleh, new failures started happening after rebasing :(( will fix it later

[caitp (gmail), 2016-04-26 23:57:45]

The CQ bit was checked by caitpotter88@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-26 23:57:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895603002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895603002/360001

[commit-bot: I haz the power, 2016-04-27 00:13:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 00:13:43]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[caitp (gmail), 2016-04-27 00:54:21]

The CQ bit was checked by caitpotter88@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 00:54:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895603002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895603002/380001

[caitp (gmail), 2016-04-27 01:03:10]

https://codereview.chromium.org/1895603002/diff/380001/test/mjsunit/mjsunit.s...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1895603002/diff/380001/test/mjsunit/mjsunit.s...
test/mjsunit/mjsunit.status:59: 'harmony/async-arrow-lexical-super': [FAIL],
So yeah, finally working again after those last few attempts...

Could use some help sorting this out in particular, though.

[commit-bot: I haz the power, 2016-04-27 01:08:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 01:08:04]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
  v8_mac_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/775)
  v8_mac_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[caitp (gmail), 2016-04-27 02:07:30]

The CQ bit was checked by caitpotter88@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 02:07:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895603002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895603002/400001

[commit-bot: I haz the power, 2016-04-27 02:21:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 02:21:08]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/777)
  v8_mac_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[caitp (gmail), 2016-04-27 03:36:31]

The CQ bit was checked by caitpotter88@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 03:36:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895603002/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895603002/420001

[commit-bot: I haz the power, 2016-04-27 03:58:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 03:58:26]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-05-02 02:16:46]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-05-02 02:16:47]

Looking good, some initial comments on the low level bits.

https://codereview.chromium.org/1895603002/diff/420001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1895603002/diff/420001/src/builtins.cc#newcod...
src/builtins.cc:2370: void Generate_GeneratorPrototypeResume(
Please don't hijack this helper function for async resume. The core
functionality is already isolated in the ResumeGenerator builtin, so all you
need for AsyncFunction prototype is a thin wrapper around that. I'd even suggest
to introduce AsyncFunctionAwaitStub with two modes (Fullfilled, Rejected) and
use them to implement %_AsyncFunctionAwaitFulfilled and
%_AsyncFunctionAwaitRejected in your .js file.

[caitp (gmail), 2016-05-02 14:45:46]

https://codereview.chromium.org/1895603002/diff/420001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1895603002/diff/420001/src/builtins.cc#newcod...
src/builtins.cc:2370: void Generate_GeneratorPrototypeResume(
On 2016/05/02 02:16:47, Benedikt Meurer wrote:
> Please don't hijack this helper function for async resume. The core
> functionality is already isolated in the ResumeGenerator builtin, so all you
> need for AsyncFunction prototype is a thin wrapper around that. I'd even
suggest
> to introduce AsyncFunctionAwaitStub with two modes (Fullfilled, Rejected) and
> use them to implement %_AsyncFunctionAwaitFulfilled and
> %_AsyncFunctionAwaitRejected in your .js file.

See the comment on this file in one of the earlier patchsets --- it states that
the changes in builtins.h and builtins.cc are unnecessary, and will be pulled
out later :)

I'm not sure if new codestubs are really needed though, since just reusing the
generator functions is plenty. Are you saying you'd actually prefer code stubs
specifically for AsyncFunctionAwait, rather than the current approach of arrow
functions which call GeneratorNext / GeneratorThrow ?

[Benedikt Meurer (Google), 2016-05-03 02:16:14]

On 2016/05/02 14:45:46, caitp wrote:
> https://codereview.chromium.org/1895603002/diff/420001/src/builtins.cc
> File src/builtins.cc (right):
> 
>
https://codereview.chromium.org/1895603002/diff/420001/src/builtins.cc#newcod...
> src/builtins.cc:2370: void Generate_GeneratorPrototypeResume(
> On 2016/05/02 02:16:47, Benedikt Meurer wrote:
> > Please don't hijack this helper function for async resume. The core
> > functionality is already isolated in the ResumeGenerator builtin, so all you
> > need for AsyncFunction prototype is a thin wrapper around that. I'd even
> suggest
> > to introduce AsyncFunctionAwaitStub with two modes (Fullfilled, Rejected)
and
> > use them to implement %_AsyncFunctionAwaitFulfilled and
> > %_AsyncFunctionAwaitRejected in your .js file.
> 
> See the comment on this file in one of the earlier patchsets --- it states
that
> the changes in builtins.h and builtins.cc are unnecessary, and will be pulled
> out later :)
> 
> I'm not sure if new codestubs are really needed though, since just reusing the
> generator functions is plenty. Are you saying you'd actually prefer code stubs
> specifically for AsyncFunctionAwait, rather than the current approach of arrow
> functions which call GeneratorNext / GeneratorThrow ?

Yes. Otherwise you might see the generator builtins in stack traces and the
debugger, which would require additional magic. The ResumeGenerator builtin was
added for this reason (and another unrelated one); it's the shared core
functionality between generators and async function resume.

[caitp (gmail), 2016-05-04 01:24:17]

I had a stab at writing the CodeStub version of this resume behaviour, but (as
explained in comments below), I have no idea how to set up their calling
conventions to be callable from JS code --- which I believe is what I need to
do.

Any help with this would be terrific

https://codereview.chromium.org/1895603002/diff/440001/src/interface-descript...
File src/interface-descriptors.h (right):

https://codereview.chromium.org/1895603002/diff/440001/src/interface-descript...
src/interface-descriptors.h:853:
DECLARE_DESCRIPTOR_WITH_CUSTOM_FUNCTION_TYPE(AsyncFunctionResumeDescriptor,
If it's possible at all to mimic JS linkage with CallInterfaceDescriptor, I have
no idea how (and could very much use some tips on this front).

It doesn't look like any other code stub needs to do this.

If you don't think the stub needs JS linkage, then I could implement the whole
thing in a builtin, but that leads to the same problem with debugger / stack
trace that existed before. Any tips here would be appreciated.

[Benedikt Meurer, 2016-05-04 04:19:15]

On 2016/05/04 01:24:17, caitp wrote:
> I had a stab at writing the CodeStub version of this resume behaviour, but (as
> explained in comments below), I have no idea how to set up their calling
> conventions to be callable from JS code --- which I believe is what I need to
> do.
> 
> Any help with this would be terrific
> 
>
https://codereview.chromium.org/1895603002/diff/440001/src/interface-descript...
> File src/interface-descriptors.h (right):
> 
>
https://codereview.chromium.org/1895603002/diff/440001/src/interface-descript...
> src/interface-descriptors.h:853:
> DECLARE_DESCRIPTOR_WITH_CUSTOM_FUNCTION_TYPE(AsyncFunctionResumeDescriptor,
> If it's possible at all to mimic JS linkage with CallInterfaceDescriptor, I
have
> no idea how (and could very much use some tips on this front).
> 
> It doesn't look like any other code stub needs to do this.
> 
> If you don't think the stub needs JS linkage, then I could implement the whole
> thing in a builtin, but that leads to the same problem with debugger / stack
> trace that existed before. Any tips here would be appreciated.

I was thinking along the lines of having two runtime functions
%AsyncFunctionAwayFulfilled and %AsyncFunctionAwaitRejected, that would do the
trick, and each of them has an inline version (denoted by the %_ syntax) that is
backed by that stub. Similar to what we do for %ToObject / %_ToObject for
example, which is backed by the ToObjectStub (which currently happens to be a
hydrogen code stub, but that doesn't matter here). But I just figured out that
this would mean that we need a way to resume the generator from within C++,
which we actually wanted to avoid. Plus even if you do that, you still have the
anonymous closures in harmony-async-await.js that wrap around the actual resume
logic showing up in the stack trace (which we can work-around for now by marking
them as native). So even that doesn't fully address the problem. So maybe
there's no so much we can do w/o also doing a proper re-write of the low level
parts of promises (which we'll have to do anyway in the not so far future, but
that shouldn't block async/await short-term).

That being said, I'm fine with your builtin approach as first shot, if you split
the resume frontend to not share the same helper method with an additional enum.
I think that should be easy to refactor later once we have a plan for promises.

Sorry for the distraction.

[Dan Ehrenberg, 2016-05-04 22:52:36]

https://codereview.chromium.org/1895603002/diff/380001/test/mjsunit/mjsunit.s...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1895603002/diff/380001/test/mjsunit/mjsunit.s...
test/mjsunit/mjsunit.status:59: 'harmony/async-arrow-lexical-super': [FAIL],
On 2016/04/27 at 01:03:09, caitp wrote:
> So yeah, finally working again after those last few attempts...
> 
> Could use some help sorting this out in particular, though.

There are also some test262 tests for async functions. Have you tried those out?

https://codereview.chromium.org/1895603002/diff/500001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1895603002/diff/500001/BUILD.gn#newcode397
BUILD.gn:397: "src/js/promise-extra.js",
Nit: No need to add this trailing whitespace

https://codereview.chromium.org/1895603002/diff/500001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/builtins.cc#newcod...
src/builtins.cc:2490: }
Just to make sure I understand, these are separate builtins just so they can be
marked native? Might be good to note in a comment, as our stack trace strategy
could change in the future as the implementation is changed for performance.

https://codereview.chromium.org/1895603002/diff/500001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/compiler.cc#newcod...
src/compiler.cc:492: }
Could you move this into the above conditional, especially if you make a utility
accessor to get at is_generator || is_async? That way, we'll be able to test
async functions with ignition generators, which should work out of the box as
far as I can tell.

https://codereview.chromium.org/1895603002/diff/500001/src/compiler/s390/code...
File src/compiler/s390/code-generator-s390.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/compiler/s390/code...
src/compiler/s390/code-generator-s390.cc:1318: __
StoreDouble(i.InputDoubleRegister(0), MemOperand(sp));
Nit: Maybe back out this style change now that you're not changing anything else
in the file.

https://codereview.chromium.org/1895603002/diff/500001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/d8.cc#newcode1214
src/d8.cc:1214: FunctionTemplate::New(isolate, FlushMicrotasks));
I worry that this flushMicrotasks call won't always work so well, as it sort of
violates the microtask model that things don't have to be re-entrant, but I
don't actually have a concrete problem that I can think of here. Existing
Promise tests use an assertLater pattern (found in test/mjsunit/es6/promises.js
for example) and depend on the fact that d8 will not exit until all microtasks
have run. Would that work? Or could you at least guard this behind a special d8
flag so it's not exposed by default?

https://codereview.chromium.org/1895603002/diff/500001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/factory.cc#newcode...
src/factory.cc:1230:
map.is_identical_to(isolate()->strict_function_without_prototype_map()) ||
Why do we suddenly need this now, but we didn't need it for concise methods?

https://codereview.chromium.org/1895603002/diff/500001/src/factory.cc#newcode...
src/factory.cc:2072: }
Nit: Any way you could merge this with the above DisableOptimization call, since
they're for the same reason and will both need to be changed in the near future?

https://codereview.chromium.org/1895603002/diff/500001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/frames.cc#newcode869
src/frames.cc:869: 
Nit: No need for the stray newline

https://codereview.chromium.org/1895603002/diff/500001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/1895603002/diff/500001/src/globals.h#newcode1077
src/globals.h:1077: };
This is not used in this patch, only in the subsequent one.

https://codereview.chromium.org/1895603002/diff/500001/src/js/harmony-async-a...
File src/js/harmony-async-await.js (right):

https://codereview.chromium.org/1895603002/diff/500001/src/js/harmony-async-a...
src/js/harmony-async-await.js:28: function AsyncFunctionAwait(generator, value)
{
I like this way of defining the function. We may want to optimize the closures
later, but it seems like a good way to start.

If we have to worry about AsyncFunctionNext/Throw showing up on the stack, then
maybe we also have to worry about these anonymous functions here also showing
up. However, I'm not sure we do have to worry about any of that, because
shouldn't both this and AsyncFunctionNext/Throw only be on the stack just before
resuming? I can't understand when you'd ever observe it.

Nit: Avoid confusingly reusing/shadowing `value`.

https://codereview.chromium.org/1895603002/diff/500001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1895603002/diff/500001/src/js/promise.js#newc...
src/js/promise.js:307: }
This seems like a good optimization. Maybe in a separate patch, we could
refactor PromiseResolve and PromiseReject to use them if this === GlobalPromise.
That would make me a bit more comfortable with respect to testing, since there
are already a bunch of tests for those directly, both in mjsunit and test262.
Another alternative to the same end would be to just put PromiseResolve and
PromiseReject on the context (renaming the RejectPromise/FuifillPromise entries
appropriately) and call out to those directly. Either way, the testing surface
is reduced. Normally I wouldn't be so picky, but we had so many bugs with
Promise correctness.

https://codereview.chromium.org/1895603002/diff/500001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1895603002/diff/500001/src/objects.h#newcode6850
src/objects.h:6850: DECL_BOOLEAN_ACCESSORS(is_async)
Maybe make an accessor which checks whether a SharedFunctionInfo is *either* a
generator or async? This comes up in a lot of places and seems like a single
semantic thing about a function.

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.cc#...
src/parsing/parser.cc:4155: function_state.set_generator_object_variable(temp);
This conditional is redundant--you just checked is_generator above.

Looks like all of this logic applies to DesugarAsyncFunctionBody as well. Is
there any way you could let this code execute for that case as well, so it
doesn't have to be in two places?

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.cc#...
src/parsing/parser.cc:4685: 
No need for the extra newline

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.cc#...
src/parsing/parser.cc:4716: FunctionBody::Normal, accept_IN, pos, CHECK_OK);
I like how you factored out DesugarAsyncFunctionBody. Think you could do the
same for desugaring the generator body (just because this function is getting
larger and larger, and you identified an analogous separable part)?

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.cc#...
src/parsing/parser.cc:4740: init_block =
BuildRejectPromiseOnException(init_block);
Rather than adding a second call to BuildRejectPromiseOnException, would it be
possible to make another call to AddParameterInitializationBlock here to avoid
this extra case of special-casing async functions?

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.h
File src/parsing/parser.h (left):

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.h#o...
src/parsing/parser.h:1251: 
Nit: no need to delete the line

https://codereview.chromium.org/1895603002/diff/500001/src/profiler/sampler.h
File src/profiler/sampler.h (left):

https://codereview.chromium.org/1895603002/diff/500001/src/profiler/sampler.h...
src/profiler/sampler.h:143: 
Nit: No need to include these whitespace changes.

[Benedikt Meurer, 2016-05-05 19:04:45]

https://codereview.chromium.org/1895603002/diff/500001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/builtins.cc#newcod...
src/builtins.cc:2490: }
From my POV, these are separate primarily because they are a temporary
work-around to connect async function resume to promise.js. G.p.next and friends
are frontends for generators. This whole dance is just to find a non-intrusive
way for now to call the ResumeGenerator builtin from promise.js. So separation
helps us to easily remove these unnecessary indirections (also the driver
closures in async-await.js) once we're there.

[caitp (gmail), 2016-05-06 16:15:09]

hmm, will investigate the ASAN failures today

[Dan Ehrenberg, 2016-05-06 16:19:25]

It looks here like AsyncFunction is created as a base class, but it needs to be
a subclass of Function per spec.

https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc
File src/bootstrapper.cc (left):

https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc#ol...
src/bootstrapper.cc:2505: Builtins::kAtomicsStore, 3, true);
Looks like a bad rebase conflict resolution

https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2463: JSObject::SetOwnPropertyIgnoreAttributes(
JSFunction::SetPrototype?

[caitp (gmail), 2016-05-06 18:45:26]

On 2016/05/06 16:19:25, Dan Ehrenberg wrote:
> It looks here like AsyncFunction is created as a base class, but it needs to
be
> a subclass of Function per spec.
> 
> https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc
> File src/bootstrapper.cc (left):
> 
>
https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc#ol...
> src/bootstrapper.cc:2505: Builtins::kAtomicsStore, 3, true);
> Looks like a bad rebase conflict resolution
> 
> https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc
> File src/bootstrapper.cc (right):
> 
>
https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc#ne...
> src/bootstrapper.cc:2463: JSObject::SetOwnPropertyIgnoreAttributes(
> JSFunction::SetPrototype?

Most or all of these (and previous) comments should be addressed in
https://codereview.chromium.org/1895603002/#ps620001

https://codereview.chromium.org/1895603002/diff/500001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/d8.cc#newcode1214
src/d8.cc:1214: FunctionTemplate::New(isolate, FlushMicrotasks));
On 2016/05/04 22:52:36, Dan Ehrenberg wrote:
> I worry that this flushMicrotasks call won't always work so well, as it sort
of
> violates the microtask model that things don't have to be re-entrant, but I
> don't actually have a concrete problem that I can think of here. Existing
> Promise tests use an assertLater pattern (found in
test/mjsunit/es6/promises.js
> for example) and depend on the fact that d8 will not exit until all microtasks
> have run. Would that work? Or could you at least guard this behind a special
d8
> flag so it's not exposed by default?

the assertLater thing is kind of a mess, I would prefer not to rely on a similar
model. It makes tests more complicated, and actually makes it harder to read the
results of the tests when failures occur.

I could put this behind a flag, but I don't believe just exposing it should be a
problem in general.

https://codereview.chromium.org/1895603002/diff/500001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/factory.cc#newcode...
src/factory.cc:1230:
map.is_identical_to(isolate()->strict_function_without_prototype_map()) ||
On 2016/05/04 22:52:36, Dan Ehrenberg wrote:
> Why do we suddenly need this now, but we didn't need it for concise methods?

I have no idea --- but I'm pretty sure it is a mistake to omit it from here ---
because NewFunctionWithoutPrototype explicitly uses this map --- so it was just
broken before in debug builds if the function was strict.

Could split that out into a separate patch with a unit test or something, I
guess? But since it's a one liner I don't think it matters a lot

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.cc#...
src/parsing/parser.cc:4155: function_state.set_generator_object_variable(temp);
On 2016/05/04 22:52:36, Dan Ehrenberg wrote:
> This conditional is redundant--you just checked is_generator above.
> 
> Looks like all of this logic applies to DesugarAsyncFunctionBody as well. Is
> there any way you could let this code execute for that case as well, so it
> doesn't have to be in two places?

It complicates things to do that --- because for AsyncArrowFunctions with
concise bodies, this never gets called, I think

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.cc#...
src/parsing/parser.cc:4716: FunctionBody::Normal, accept_IN, pos, CHECK_OK);
On 2016/05/04 22:52:36, Dan Ehrenberg wrote:
> I like how you factored out DesugarAsyncFunctionBody. Think you could do the
> same for desugaring the generator body (just because this function is getting
> larger and larger, and you identified an analogous separable part)?

Sure --- but lets do that in a separate CL

https://codereview.chromium.org/1895603002/diff/500001/src/parsing/parser.cc#...
src/parsing/parser.cc:4740: init_block =
BuildRejectPromiseOnException(init_block);
On 2016/05/04 22:52:36, Dan Ehrenberg wrote:
> Rather than adding a second call to BuildRejectPromiseOnException, would it be
> possible to make another call to AddParameterInitializationBlock here to avoid
> this extra case of special-casing async functions?

You mean like

```
if (IsAsyncFunction(kind)) {
  init_block =
BuildRejectPromiseOnException(BuildParameterInitializationBlock(parameters,
CHECK_OK));
} else {
  init_block = BuildParameterInitializationBlock(parameters, CHECK_OK);
}
```

? I'm not sure I see much value in that, but maybe I'm misunderstanding what
you're getting at

https://codereview.chromium.org/1895603002/diff/500001/src/profiler/sampler.h
File src/profiler/sampler.h (left):

https://codereview.chromium.org/1895603002/diff/500001/src/profiler/sampler.h...
src/profiler/sampler.h:143: 
On 2016/05/04 22:52:36, Dan Ehrenberg wrote:
> Nit: No need to include these whitespace changes.

Done.

https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc
File src/bootstrapper.cc (left):

https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc#ol...
src/bootstrapper.cc:2505: Builtins::kAtomicsStore, 3, true);
On 2016/05/06 16:19:25, Dan Ehrenberg wrote:
> Looks like a bad rebase conflict resolution

I'm not sure, I haven't rebased over that new commit yet --- so it probably just
isn't there? Either way, should be cleaned up soon

https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1895603002/diff/580001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2463: JSObject::SetOwnPropertyIgnoreAttributes(
On 2016/05/06 16:19:25, Dan Ehrenberg wrote:
> JSFunction::SetPrototype?

Done.

[caitp (gmail), 2016-05-06 18:46:55]

https://codereview.chromium.org/1895603002/diff/640001/src/profiler/sampler.cc
File src/profiler/sampler.cc (right):

https://codereview.chromium.org/1895603002/diff/640001/src/profiler/sampler.c...
src/profiler/sampler.cc:337: Simulator* simulator =
isolate->thread_local_top()->simulator_;
This is `git cl format`'s doing, heh :( I'll try to fix the unnecessary things
that it snuck in

[Dan Ehrenberg, 2016-05-09 23:02:54]

https://codereview.chromium.org/1895603002/diff/500001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1895603002/diff/500001/src/factory.cc#newcode...
src/factory.cc:1230:
map.is_identical_to(isolate()->strict_function_without_prototype_map()) ||
On 2016/05/06 at 18:45:26, caitp wrote:
> On 2016/05/04 22:52:36, Dan Ehrenberg wrote:
> > Why do we suddenly need this now, but we didn't need it for concise methods?
> 
> I have no idea --- but I'm pretty sure it is a mistake to omit it from here
--- because NewFunctionWithoutPrototype explicitly uses this map --- so it was
just broken before in debug builds if the function was strict.
> 
> Could split that out into a separate patch with a unit test or something, I
guess? But since it's a one liner I don't think it matters a lot

Well, if it fixes a previous bug, then at least checking in tests for that bug
together with this patch would be a good thing to do. Splitting it into a
separate patch would be optimal still.

https://codereview.chromium.org/1895603002/diff/700001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/1895603002/diff/700001/src/globals.h#newcode993
src/globals.h:993: kind == FunctionKind::kArrowGeneratorFunction;
Huh? What's this? I don't think we have syntax for arrow generator functions.

[caitp (gmail), 2016-05-10 00:34:58]

https://codereview.chromium.org/1895603002/diff/700001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/1895603002/diff/700001/src/globals.h#newcode993
src/globals.h:993: kind == FunctionKind::kArrowGeneratorFunction;
On 2016/05/09 23:02:54, Dan Ehrenberg wrote:
> Huh? What's this? I don't think we have syntax for arrow generator functions.

beats me, i thought it was added in a rebase, i dont remember adding it

[neis, 2016-05-10 10:59:06]

Hi Caitlin, could you rebase this again, or tell me on which checkout it applies
cleanly? Thanks.

[neis, 2016-05-10 11:57:03]

On 2016/05/10 10:59:06, neis wrote:
> Hi Caitlin, could you rebase this again, or tell me on which checkout it
applies
> cleanly? Thanks.

Nevermind. Turns out "git cl patch" doesn't take dependencies into account.
After doing that manually, the only conflict is with bytecode expectations,
which is negligible.

[caitp (gmail), 2016-05-10 12:06:39]

On 2016/05/10 11:57:03, neis wrote:
> On 2016/05/10 10:59:06, neis wrote:
> > Hi Caitlin, could you rebase this again, or tell me on which checkout it
> applies
> > cleanly? Thanks.
> 
> Nevermind. Turns out "git cl patch" doesn't take dependencies into account.
> After doing that manually, the only conflict is with bytecode expectations,
> which is negligible.

I was going to say it should apply cleanly over
5cbe0f5d25592525b1f8244a81c75a9df29969a8 + the dependency patchset --- will
rebase in a few minutes though.

[neis, 2016-05-10 12:22:53]

Description was changed from

==========
[esnext] prototype runtime implementation for async functions

BUG=v8:4483
LOG=N
R=littledan@chromium.org, adamk@chromium.org
==========

to

==========
[esnext] prototype runtime implementation for async functions

BUG=v8:4483
LOG=N
R=littledan@chromium.org, adamk@chromium.org
==========

[neis, 2016-05-10 12:22:53]

neis@chromium.org changed reviewers:
+ neis@chromium.org

[neis, 2016-05-11 09:45:17]

Looking good overall.  Here's a first round of comments.

- The async draft extends the syntax of "Method Definitions" but misses to
  extend its static semantics. I think it's safe to assume that in
  particular SpecialMethod should be extended such that it returns true
  for async methods, thus enforcing that a class "constructor" cannot be
  async.
  Your implementation currently allows this.  For instance,
    class Gaga { async constructor() { this.foo = 42 } }
  is accepted. I think you should throw a syntax error instead.


- Async functions are currently printed the same as ordinary ones:
    d8> async function foo() { }
    undefined
    d8> foo
    function foo() { }
  Can you adapt this (in a followup CL perhaps)?

https://codereview.chromium.org/1895603002/diff/720001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1895603002/diff/720001/src/compiler.cc#newcod...
src/compiler.cc:434: }
This looks wrong, it's not the same as

if (info->shared_info()->is_resumable() && !FLAG_ignition_generators) {
  return false;
}

so you are changing the logic by skipping the filter tests below.

https://codereview.chromium.org/1895603002/diff/720001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1895603002/diff/720001/src/contexts.h#newcode131
src/contexts.h:131: V(ASYNC_FUNCTION_AWAIT_INDEX, JSFunction,
async_function_await)           \
keep it sorted

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser-bas...
src/parsing/parser-base.h:41: ArrowBlock = Normal,
I find that confusing.

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser-bas...
src/parsing/parser-base.h:330: DCHECK(is_generator() || is_async_function());
Didn't you introduce a function for this?

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:2726: return_value = EmitPromiseResolve(return_value,
return_value->position());
Can you call these functions Build* rather than Emit*?

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:3980: bool accept_IN, int pos, bool* ok) {
Please add a comment here on what the desugaring is doing, eg in the form of JS
pseudo code.

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:3983: // stored
format (but not sure it's worth to duplicate this comment here)

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:3992: Expression* assign_yield =
assign_yield is a very misleading name.

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:3999: 
I would rewrite this as follows:

// Create generator, but do not yield.
{
  Variable* temp =
      scope_->NewTemporary(ast_value_factory()->dot_generator_object_string());
  function_state_->set_generator_object_variable(temp);

  Expression* assignment = factory()->NewAssignment(
      Token::INIT, factory()->NewVariableProxy(temp),
      EmitCreateJSGeneratorObject(pos), RelocInfo::kNoPosition);
  body->Add(
      factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition),
      zone());
}

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:4021: Block* catch_block =
BuildRejectPromiseOnException(try_block);
Calling this catch_block is confusing, as it's the whole try-catch statement. I
would just inline it.

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:5547: Expression*
ParserTraits::RewriteAwaitExpression(Expression* value, int pos) {
Also add a comment here please.

https://codereview.chromium.org/1895603002/diff/720001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-await-basic.js (right):

https://codereview.chromium.org/1895603002/diff/720001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-await-basic.js:7: // TODO(caitp): give async
functions a non-constructor map
This comment is outdated, isn't it?

https://codereview.chromium.org/1895603002/diff/720001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-await-basic.js:118: log.push("start:" + value);
indentation

[caitp (gmail), 2016-05-11 13:42:23]

I've updated the frontend CL add async methods to the SpecialMethod handling

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:3980: bool accept_IN, int pos, bool* ok) {
On 2016/05/11 09:45:16, neis wrote:
> Please add a comment here on what the desugaring is doing, eg in the form of
JS
> pseudo code.

Done.

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:3983: // stored
On 2016/05/11 09:45:16, neis wrote:
> format (but not sure it's worth to duplicate this comment here)

Removed it

https://codereview.chromium.org/1895603002/diff/720001/src/parsing/parser.cc#...
src/parsing/parser.cc:3992: Expression* assign_yield =
On 2016/05/11 09:45:16, neis wrote:
> assign_yield is a very misleading name.

Renamed to `init_generator_variable`

[Dan Ehrenberg, 2016-05-11 18:00:51]

Just for the record, but it doesn't need to be addressed in this patch: the V8
API should probably be extended with an IsAsyncFunction() method.

[Dan Ehrenberg, 2016-05-12 02:10:33]

https://codereview.chromium.org/1895603002/diff/780001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/async-arrow-lexical-super.js (right):

https://codereview.chromium.org/1895603002/diff/780001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-arrow-lexical-super.js:3: // found in the LICENSE
file.
This test passes if you patch in https://codereview.chromium.org/1968263002 .
That patch needs a small bug fix for debugging generator frames, but otherwise,
should be good to prototype with.

https://codereview.chromium.org/1895603002/diff/780001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-arrow-lexical-super.js:22: new
DeferredSuperCall("LexicalSuperCall")().then(x => x.name));
Insert () => at the beginning of this line.

https://codereview.chromium.org/1895603002/diff/780001/test/mjsunit/harmony/a...
test/mjsunit/harmony/async-arrow-lexical-super.js:31: new
DeferredSuperProperty("LexicalSuperProperty").deferredName()());
Insert () => at the beginning of this line.

[neis, 2016-05-12 16:34:23]

https://codereview.chromium.org/1895603002/diff/840001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1895603002/diff/840001/src/builtins.cc#newcod...
src/builtins.cc:2506: // ES6 section 25.3.1.4 Generator.prototype.throw (
exception )
wrong comment

https://codereview.chromium.org/1895603002/diff/840001/src/builtins.cc#newcod...
src/builtins.cc:2511: 
Can we get rid of these functions and just use GeneratorPrototypeNext/Throw
instead? The error case in which the difference would matter can't really
happen, right?

[caitp (gmail), 2016-05-12 16:38:39]

https://codereview.chromium.org/1895603002/diff/840001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1895603002/diff/840001/src/builtins.cc#newcod...
src/builtins.cc:2506: // ES6 section 25.3.1.4 Generator.prototype.throw (
exception )
On 2016/05/12 16:34:22, neis wrote:
> wrong comment

Acknowledged.

https://codereview.chromium.org/1895603002/diff/840001/src/builtins.cc#newcod...
src/builtins.cc:2511: 
On 2016/05/12 16:34:22, neis wrote:
> Can we get rid of these functions and just use GeneratorPrototypeNext/Throw
> instead? The error case in which the difference would matter can't really
> happen, right?

We mark these as native, which prevents them from being included in the stack
trace --- I believe for generators, you want them to appear in the stack trace.

[neis, 2016-05-12 16:41:38]

> We mark these as native, which prevents them from being included in the stack
> trace --- I believe for generators, you want them to appear in the stack
trace.

Ah I see, thanks.

[neis, 2016-05-12 16:42:12]

On 2016/05/12 16:41:38, neis wrote:
> > We mark these as native, which prevents them from being included in the
stack
> > trace --- I believe for generators, you want them to appear in the stack
> trace.
> 
> Ah I see, thanks.

Maybe add that as a comment.

[neis, 2016-05-13 08:54:42]

On 2016/05/12 16:42:12, neis wrote:
> On 2016/05/12 16:41:38, neis wrote:
> > > We mark these as native, which prevents them from being included in the
> stack
> > > trace --- I believe for generators, you want them to appear in the stack
> > trace.
> > 
> > Ah I see, thanks.
> 
> Maybe add that as a comment.

Actually, you can still use GeneratorPrototypeNext/Throw since the native flag
is on the JSFunction, not on the code.

[Dan Ehrenberg, 2016-05-13 18:39:51]

https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2483: *isolate->builtins()->GeneratorFunctionConstructor());
You probably meant to write AsyncFunctionConstructor here (which you implemented
in this patch :). That change fixes a test262 test.

[caitp (gmail), 2016-05-13 18:41:13]

https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2483: *isolate->builtins()->GeneratorFunctionConstructor());
On 2016/05/13 18:39:51, Dan Ehrenberg wrote:
> You probably meant to write AsyncFunctionConstructor here (which you
implemented
> in this patch :). That change fixes a test262 test.

Hmm --- can you point out the failing test? I added some tests for
AsyncFunction()-constructed fns that seem to pass

[caitp (gmail), 2016-05-13 18:41:14]

https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2483: *isolate->builtins()->GeneratorFunctionConstructor());
On 2016/05/13 18:39:51, Dan Ehrenberg wrote:
> You probably meant to write AsyncFunctionConstructor here (which you
implemented
> in this patch :). That change fixes a test262 test.

Hmm --- can you point out the failing test? I added some tests for
AsyncFunction()-constructed fns that seem to pass

[Dan Ehrenberg, 2016-05-13 18:53:37]

https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:2483: *isolate->builtins()->GeneratorFunctionConstructor());
On 2016/05/13 at 18:41:14, caitp wrote:
> On 2016/05/13 18:39:51, Dan Ehrenberg wrote:
> > You probably meant to write AsyncFunctionConstructor here (which you
implemented
> > in this patch :). That change fixes a test262 test.
> 
> Hmm --- can you point out the failing test? I added some tests for
AsyncFunction()-constructed fns that seem to pass

Anything with 'new AsyncFunction()'.

[caitp (gmail), 2016-05-13 19:16:18]

On 2016/05/13 18:53:37, Dan Ehrenberg wrote:
> https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc
> File src/bootstrapper.cc (right):
> 
>
https://codereview.chromium.org/1895603002/diff/940001/src/bootstrapper.cc#ne...
> src/bootstrapper.cc:2483:
*isolate->builtins()->GeneratorFunctionConstructor());
> On 2016/05/13 at 18:41:14, caitp wrote:
> > On 2016/05/13 18:39:51, Dan Ehrenberg wrote:
> > > You probably meant to write AsyncFunctionConstructor here (which you
> implemented
> > > in this patch :). That change fixes a test262 test.
> > 
> > Hmm --- can you point out the failing test? I added some tests for
> AsyncFunction()-constructed fns that seem to pass
> 
> Anything with 'new AsyncFunction()'.

I see --- fixed and added some more tests

[Dan Ehrenberg, 2016-05-16 22:36:47]

https://codereview.chromium.org/1895603002/diff/1020001/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (right):

https://codereview.chromium.org/1895603002/diff/1020001/test/mjsunit/mjsunit....
test/mjsunit/mjsunit.js:309: %RunMicrotasks();
Won't this throw without --allow-natives-syntax? Maybe these library things have
to live somewhere else because of that.

[caitp (gmail), 2016-05-16 22:40:00]

https://codereview.chromium.org/1895603002/diff/1020001/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (right):

https://codereview.chromium.org/1895603002/diff/1020001/test/mjsunit/mjsunit....
test/mjsunit/mjsunit.js:309: %RunMicrotasks();
On 2016/05/16 22:36:47, Dan Ehrenberg wrote:
> Won't this throw without --allow-natives-syntax? Maybe these library things
have
> to live somewhere else because of that.

Well it will throw, but I don't think anything else is needed other than a
comment explaining that.

Alternatively, we could bring back the version in d8.cc, maybe as a property of
a `TestUtils` object or something, which is probably less likely to hurt people
using d8 in production for some reason (which I think is pretty unlikely
regardless).

[Dan Ehrenberg, 2016-05-16 22:49:20]

https://codereview.chromium.org/1895603002/diff/1020001/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (right):

https://codereview.chromium.org/1895603002/diff/1020001/test/mjsunit/mjsunit....
test/mjsunit/mjsunit.js:309: %RunMicrotasks();
On 2016/05/16 at 22:39:59, caitp wrote:
> On 2016/05/16 22:36:47, Dan Ehrenberg wrote:
> > Won't this throw without --allow-natives-syntax? Maybe these library things
have
> > to live somewhere else because of that.
> 
> Well it will throw, but I don't think anything else is needed other than a
comment explaining that.
> 
> Alternatively, we could bring back the version in d8.cc, maybe as a property
of a `TestUtils` object or something, which is probably less likely to hurt
people using d8 in production for some reason (which I think is pretty unlikely
regardless).

This will probably throw at parse time. % functions cannot live in mjsunit.js
unfortunately.

[Dan Ehrenberg, 2016-05-16 23:01:16]

Just a couple nits, then we're good to go

https://codereview.chromium.org/1895603002/diff/1040001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/1895603002/diff/1040001/src/d8.h#newcode406
src/d8.h:406: static void FlushMicrotasks(const
v8::FunctionCallbackInfo<v8::Value>& args);
Remove this dead declaration

https://codereview.chromium.org/1895603002/diff/1040001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1895603002/diff/1040001/src/objects.cc#newcod...
src/objects.cc:11952: function->shared()->is_async());
Nit: use is_resumable()

https://codereview.chromium.org/1895603002/diff/1040001/src/objects.cc#newcod...
src/objects.cc:11963: if (function->shared()->is_generator() ||
function->shared()->is_async()) {
Nit: use is_resumable()

https://codereview.chromium.org/1895603002/diff/1040001/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (left):

https://codereview.chromium.org/1895603002/diff/1040001/test/mjsunit/mjsunit....
test/mjsunit/mjsunit.js:57: 
Revert the irrelevant deletion

[caitp (gmail), 2016-05-16 23:08:31]

https://codereview.chromium.org/1895603002/diff/1040001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/1895603002/diff/1040001/src/d8.h#newcode406
src/d8.h:406: static void FlushMicrotasks(const
v8::FunctionCallbackInfo<v8::Value>& args);
On 2016/05/16 23:01:16, Dan Ehrenberg wrote:
> Remove this dead declaration

Done.

https://codereview.chromium.org/1895603002/diff/1040001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1895603002/diff/1040001/src/objects.cc#newcod...
src/objects.cc:11952: function->shared()->is_async());
On 2016/05/16 23:01:16, Dan Ehrenberg wrote:
> Nit: use is_resumable()

Done.

https://codereview.chromium.org/1895603002/diff/1040001/src/objects.cc#newcod...
src/objects.cc:11963: if (function->shared()->is_generator() ||
function->shared()->is_async()) {
On 2016/05/16 23:01:16, Dan Ehrenberg wrote:
> Nit: use is_resumable()

Done.

https://codereview.chromium.org/1895603002/diff/1040001/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (left):

https://codereview.chromium.org/1895603002/diff/1040001/test/mjsunit/mjsunit....
test/mjsunit/mjsunit.js:57: 
On 2016/05/16 23:01:16, Dan Ehrenberg wrote:
> Revert the irrelevant deletion

Done.

[Dan Ehrenberg, 2016-05-16 23:45:00]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-16 23:45:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895603002/1060001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895603002/1060001

[Dan Ehrenberg, 2016-05-16 23:45:32]

lgtm

[caitp (gmail), 2016-05-16 23:55:37]

The CQ bit was unchecked by caitpotter88@gmail.com

[caitp (gmail), 2016-05-16 23:55:50]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2016-05-16 23:56:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895603002/1060001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895603002/1060001

[commit-bot: I haz the power, 2016-05-17 00:27:03]

Description was changed from

==========
[esnext] prototype runtime implementation for async functions

BUG=v8:4483
LOG=N
R=littledan@chromium.org, adamk@chromium.org
==========

to

==========
[esnext] prototype runtime implementation for async functions

BUG=v8:4483
LOG=N
R=littledan@chromium.org, adamk@chromium.org
==========

[commit-bot: I haz the power, 2016-05-17 00:27:06]

Committed patchset #51 (id:1060001)

[commit-bot: I haz the power, 2016-05-17 00:28:01]

Description was changed from

==========
[esnext] prototype runtime implementation for async functions

BUG=v8:4483
LOG=N
R=littledan@chromium.org, adamk@chromium.org
==========

to

==========
[esnext] prototype runtime implementation for async functions

BUG=v8:4483
LOG=N
R=littledan@chromium.org, adamk@chromium.org

Committed: https://crrev.com/d08c0304c5779223d6c468373af4815ec3ccdb84
Cr-Commit-Position: refs/heads/master@{#36263}
==========

[commit-bot: I haz the power, 2016-05-17 00:28:03]

Patchset 51 (id:??) landed as
https://crrev.com/d08c0304c5779223d6c468373af4815ec3ccdb84
Cr-Commit-Position: refs/heads/master@{#36263}