WIP [esnext] implement async iteration proposal

WIP [esnext] implement async iteration proposal

Submitted for early review. There are still missing pieces, and likely places where it could be done better
(generating smaller code, for example).

TODO(caitp): Finish cleanup and split apart into separate/easier-to-review patches and land separately.

BUG=

[caitp, 2017-01-10 04:13:42]

Sneak peak for early review/feedback. Some self review attached.

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-async.cc
File src/builtins/builtins-async.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-async...
src/builtins/builtins-async.cc:15: Node* AsyncBuiltinsAssembler::Await(
I think this thing could be used to refactor the Async Functions await code into
TF code. In the current design, the async generator version pretty much needs to
be implemented in TF in order to get access to the fields of the
AsyncGeneratorRequest objects.

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-promi...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:620: CSA_SLOW_ASSERT(this,
HasInstanceType(promise, JS_PROMISE_TYPE));
Basically just a slightly smaller version of InternalPerformPromiseThen(), with
assertions about being an internal Promise. Downside of code duplication might
not be worth my expected code size reduction

https://codereview.chromium.org/2622833002/diff/1/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2622833002/diff/1/src/code-stub-assembler.h#n...
src/code-stub-assembler.h:1125: void ReleasePrint(const char* c_str) {
I find myself writing these helpers for builtins an awful lot, in order to trace
through control flow of builtins easier. I think it makes sense to just have
them in CSA.

https://codereview.chromium.org/2622833002/diff/1/src/compiler/machine-graph-...
File src/compiler/machine-graph-verifier.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/compiler/machine-graph-...
src/compiler/machine-graph-verifier.cc:522: case IrOpcode::kRetain:
TODO(caitp): get rid of this (and any calls to Retain() added in CSA builtins in
this patch, they didn't really solve any GC issues I was debugging previously)

https://codereview.chromium.org/2622833002/diff/1/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/2622833002/diff/1/src/contexts.h#newcode257
src/contexts.h:257: V(INITIAL_ASYNC_ITERATOR_PROTOTYPE_INDEX, JSObject,         
                \
I don't think these are necessarily needed.

https://codereview.chromium.org/2622833002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2268: builder()
AwaitExpressions are still desugared in the parser/ast-numbering, so they use
the sync style yield bytecode.

https://codereview.chromium.org/2622833002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-intrinsics.cc:395: Node* sync_iterator = __
LoadRegister(args_reg);
This seems better than calling a runtime function to me, but for async iteration
it probably isn't really a bottleneck. Thoughts?

https://codereview.chromium.org/2622833002/diff/1/src/runtime/runtime-generat...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/runtime/runtime-generat...
src/runtime/runtime-generator.cc:98: return isolate->heap()->undefined_value();
This function is nonsense. TODO(caitp): remove this

https://codereview.chromium.org/2622833002/diff/1/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (left):

https://codereview.chromium.org/2622833002/diff/1/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:3946: CmpObjectType(object,
JS_GENERATOR_OBJECT_TYPE, object);
Need to alter these assertions in order to reuse the ResumeGenerator stub.

[caitp, 2017-01-10 04:19:33]

caitp@igalia.com changed reviewers:
+ gsathya@chromium.org

[caitp, 2017-01-10 04:19:34]

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-promi...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:55: Node* context) {
This seemed more useful earlier today, but after discussion with Domenic about
fixing async-from-sync Iterator, it's probably safe to get rid of it.

The main idea was to not report Promise Hook events for an intermediate Promise
allocated by Async-from-Sync iterator methods, which makes sense to me.

Sathya wdyt?

[caitp, 2017-01-10 17:40:19]

caitp@igalia.com changed reviewers:
+ adamk@chromium.org, bmeurer@chromium.org, jgruber@chromium.org,
littledan@chromium.org

[caitp, 2017-01-10 17:40:19]

+a few people who might have comments/criticisms on some parts of this.

[gsathya, 2017-01-10 17:47:24]

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-promi...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:55: Node* context) {
On 2017/01/10 04:19:33, caitp wrote:
> This seemed more useful earlier today, but after discussion with Domenic about
> fixing async-from-sync Iterator, it's probably safe to get rid of it.
> 
> The main idea was to not report Promise Hook events for an intermediate
Promise
> allocated by Async-from-Sync iterator methods, which makes sense to me.
> 
> Sathya wdyt?

The Async-from-Sync iterator methods seems to use NewPromiseCapability so we
should fire the hooks. The hooks are expected to fire for all promises including
intermediate promises. We  do the same for async-await.

[caitp, 2017-01-10 17:48:54]

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-promi...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:55: Node* context) {
On 2017/01/10 17:47:24, gsathya wrote:
> On 2017/01/10 04:19:33, caitp wrote:
> > This seemed more useful earlier today, but after discussion with Domenic
about
> > fixing async-from-sync Iterator, it's probably safe to get rid of it.
> > 
> > The main idea was to not report Promise Hook events for an intermediate
> Promise
> > allocated by Async-from-Sync iterator methods, which makes sense to me.
> > 
> > Sathya wdyt?
> 
> The Async-from-Sync iterator methods seems to use NewPromiseCapability so we
> should fire the hooks. The hooks are expected to fire for all promises
including
> intermediate promises. We  do the same for async-await.

sgtm

[caitp, 2017-01-10 19:23:17]

for-await-of is entirely broken atm, working on this.

[caitp, 2017-01-11 16:13:14]

On 2017/01/10 19:23:17, caitp wrote:
> for-await-of is entirely broken atm, working on this.

To get Yield* to work in async generators, it's necessary to restore the old
function.sent value when resuming after an Await expression.

So, this is a bit unfortunate, and adds a fair bit of code :(

[caitp, 2017-01-11 17:21:45]

Yield* support is in the latest patchset, which _I believe_ is the last part of
this. I'm going to begin writing test coverage now.

[neis, 2017-01-12 12:30:46]

Description was changed from

==========
WIP [esnext] implement async iteration proposal

Submitted for early review. There are still missing pieces, and likely places
where it could be done better
(generating smaller code, for example).

TODO(caitp): Finish cleanup and split apart into separate/easier-to-review
patches and land separately.

BUG=
==========

to

==========
WIP [esnext] implement async iteration proposal

Submitted for early review. There are still missing pieces, and likely places
where it could be done better
(generating smaller code, for example).

TODO(caitp): Finish cleanup and split apart into separate/easier-to-review
patches and land separately.

BUG=
==========

[neis, 2017-01-12 12:30:46]

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

[jgruber, 2017-01-13 13:34:41]

Some minor comments below from a first look.

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1412:
isolate->builtins()->builtin(Builtins::kAsyncIteratorValueUnwrap),
Nit: isolate->builtins()->AsyncIteratorValueUnwrap()

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1455: Builtins::kAsyncGeneratorAwaitResolveClosure),
Same here and below.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
File src/builtins/builtins-async-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:83: Node* const iter_result =
AllocateJSIteratorResult(context, value, false);
I just recently landed CreateIterResultObject, perhaps we can merge these.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:129: // This loop is resides in
AsyncGeneratorResumeNext.
Nit: is resides

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:257: StoreObjectField(generator,
JSAsyncGeneratorObject::kContinuationOffset,
I guess we could use the NoWriteBarrier variant here.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:262: Branch(WordEqual(continuation,
SmiEqual here and at other smi comparisons.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:366: StoreObjectField(request,
AsyncGeneratorRequest::kAwaitedPromiseOffset,
NoWriteBarrier

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:372:
GotoUnless(SmiAboveOrEqual(continuation, SmiConstant(0)), &done);
Isn't an unsigned '>= 0' comparison always true?

[jgruber, 2017-01-13 13:34:43]

Some minor comments below from a first look.

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1412:
isolate->builtins()->builtin(Builtins::kAsyncIteratorValueUnwrap),
Nit: isolate->builtins()->AsyncIteratorValueUnwrap()

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1455: Builtins::kAsyncGeneratorAwaitResolveClosure),
Same here and below.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
File src/builtins/builtins-async-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:83: Node* const iter_result =
AllocateJSIteratorResult(context, value, false);
I just recently landed CreateIterResultObject, perhaps we can merge these.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:129: // This loop is resides in
AsyncGeneratorResumeNext.
Nit: is resides

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:257: StoreObjectField(generator,
JSAsyncGeneratorObject::kContinuationOffset,
I guess we could use the NoWriteBarrier variant here.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:262: Branch(WordEqual(continuation,
SmiEqual here and at other smi comparisons.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:366: StoreObjectField(request,
AsyncGeneratorRequest::kAwaitedPromiseOffset,
NoWriteBarrier

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:372:
GotoUnless(SmiAboveOrEqual(continuation, SmiConstant(0)), &done);
Isn't an unsigned '>= 0' comparison always true?

[caitp, 2017-01-13 14:36:04]

Thanks

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1412:
isolate->builtins()->builtin(Builtins::kAsyncIteratorValueUnwrap),
On 2017/01/13 13:34:34, jgruber wrote:
> Nit: isolate->builtins()->AsyncIteratorValueUnwrap()

Acknowledged.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
File src/builtins/builtins-async-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:83: Node* const iter_result =
AllocateJSIteratorResult(context, value, false);
On 2017/01/13 13:34:39, jgruber wrote:
> I just recently landed CreateIterResultObject, perhaps we can merge these.

Sgtm

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:366: StoreObjectField(request,
AsyncGeneratorRequest::kAwaitedPromiseOffset,
On 2017/01/13 13:34:36, jgruber wrote:
> NoWriteBarrier

Acknowledged.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:372:
GotoUnless(SmiAboveOrEqual(continuation, SmiConstant(0)), &done);
On 2017/01/13 13:34:35, jgruber wrote:
> Isn't an unsigned '>= 0' comparison always true?

If the generator is executing or closed, no.

[jgruber, 2017-01-13 15:46:40]

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
File src/builtins/builtins-async-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:372:
GotoUnless(SmiAboveOrEqual(continuation, SmiConstant(0)), &done);
On 2017/01/13 14:36:04, caitp wrote:
> On 2017/01/13 13:34:35, jgruber wrote:
> > Isn't an unsigned '>= 0' comparison always true?
> 
> If the generator is executing or closed, no.

My point was that there is no way to make SmiAboveOrEqual(..., 0) return false.
SmiAboveOrEqual(-1, 0) --> true since the comparison is unsigned.
Maybe you're looking for SmiGreaterThanOrEqual?

[Dan Ehrenberg, 2017-01-13 19:41:09]

Some initial comments; still taking a closer look at the meatier parts.

https://codereview.chromium.org/2622833002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-intrinsics.cc (right):

https://codereview.chromium.org/2622833002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-intrinsics.cc:395: Node* sync_iterator = __
LoadRegister(args_reg);
On 2017/01/10 04:13:42, caitp wrote:
> This seems better than calling a runtime function to me, but for async
iteration
> it probably isn't really a bottleneck. Thoughts?

Eh, seems like a good idea to me. I don't really see a downside; this code is
not hard to understand or maintain. You might have a short iterable where the
perf here becomes visible (`for await (let _ of []) {}` will run way faster!)

https://codereview.chromium.org/2622833002/diff/80001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2622833002/diff/80001/include/v8.h#newcode8444
include/v8.h:8444: static const int kJSObjectType = 0xbd;
+jochen I see that the heading disavows it, but does renumbering these affect
any embedders in a way which could hurt compatibility?

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-array.cc (left):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-array.cc:2284: 
Nit: Revert whitespace change

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:18: bool is_catchable) {
This is awesome. Suggestion: Break this patch moving async-await.js into this
form off into a separate patch that's first in the series.

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:23: InternalResolvePromise(context,
wrapped_value, value);
Looks like you're fixing v8:5694 here, right?

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:39: LoadContextElement(native_context,
on_reject_context_index + 1);
Could you put a comment cross-referencing here with the places that have the two
adjacent context elements, to make sure that we maintain this property? It
doesn't look like AsyncFunction{Next,Throw} even have context indices; are those
the ones you'd use when calling this from ordinary async functions?

If you end up deciding to check in async iterators/generators *before* making
this change to async functions, maybe you should just remove the generality here
and refer explicitly to the particular context indices you're interested in,
rather than taking this on_reject_context_index argument.

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async.h (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.h:10: class AsyncBuiltinsAssembler : public
PromiseBuiltinsAssembler {
I'm a little surprised by this inheritance, and don't see other similar
examples, but I don't see anything else that is trying to compose code like
this. Stupid suggestion: private inheritance?

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins.h...
src/builtins/builtins.h:780: /*
proposal-async-iteration/#sec-asyncgeneratorfunction-constructor */       \
+1 to this notation

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/x64/builti...
File src/builtins/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:566: //  -- rbx    : the JSGeneratorObject to
resume
Presumably this would only be called on JSAsyncGeneratorObjects, right?

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:581: kDontSaveFPRegs);
Is the only difference here vs Generate_ResumeGeneratorTrampoline whether we are
saving and restoring from await_input? If so, it'd be nice to refactor this a
little bit to reduce all this duplicated assembly.

https://codereview.chromium.org/2622833002/diff/80001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2622833002/diff/80001/src/runtime/runtime.h#n...
src/runtime/runtime.h:415: F(DefineDataPropertyInLiteral, 4, 1)               \
This looks like a broken rebase.

https://codereview.chromium.org/2622833002/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2622833002/diff/120001/src/objects.h#newcode7946
src/objects.h:7946: DECL_ACCESSORS(await_input, Object)
Why is this added to JSGeneratorObject? I would expect it to only be added to
JSAsyncGeneratorObject.

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

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser-bas...
src/parsing/parser-base.h:2066: if (token == Token::MUL &&
allow_harmony_async_iteration() &&
Style nit: Put the flag check first

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser-bas...
src/parsing/parser-base.h:2493: FunctionKind kind = kMethodKinds[index];
Nit: Isn't this what a two-dimensional array is for?

static const FunctionKind kMethodKinds[2][2] = {
  { FunctionKind::kConciseMethod, FunctionKind::kConciseGeneratorMethod },
  { FunctionKind::kAsyncConciseMethod,
FunctionKind::kAsyncConciseGeneratorMethod }
};
FunctionKind kind = kMethodKinds[is_async][is_generator];

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

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:4889: // output = Await(%_Call(iteratorReturn, iterator,
input));
Comment nit: should be if (IteratorType == kAsync) { ... }

[caitp, 2017-01-13 20:11:53]

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:23: InternalResolvePromise(context,
wrapped_value, value);
On 2017/01/13 19:41:08, Dan Ehrenberg wrote:
> Looks like you're fixing v8:5694 here, right?

yeah, this fixes the anba bug

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:39: LoadContextElement(native_context,
on_reject_context_index + 1);
On 2017/01/13 19:41:08, Dan Ehrenberg wrote:
> Could you put a comment cross-referencing here with the places that have the
two
> adjacent context elements, to make sure that we maintain this property? It
> doesn't look like AsyncFunction{Next,Throw} even have context indices; are
those
> the ones you'd use when calling this from ordinary async functions?
> 
> If you end up deciding to check in async iterators/generators *before* making
> this change to async functions, maybe you should just remove the generality
here
> and refer explicitly to the particular context indices you're interested in,
> rather than taking this on_reject_context_index argument.

AsyncFunctionNext / Throw are aliases for GeneratorNext / Throw, and are
exported to JS builtins. When/if those are ported to use this shared Await
thing, they'll be installed to the native context instead, and likely
implemented similar to the AsyncGeneratorAwaitResolveClosure/RejectClosure
functions (but slightly different).

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async.h (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.h:10: class AsyncBuiltinsAssembler : public
PromiseBuiltinsAssembler {
On 2017/01/13 19:41:09, Dan Ehrenberg wrote:
> I'm a little surprised by this inheritance, and don't see other similar
> examples, but I don't see anything else that is trying to compose code like
> this. Stupid suggestion: private inheritance?

Would that make CSA methods private, or just PromiseBuiltinsAssembler methods?

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/x64/builti...
File src/builtins/x64/builtins-x64.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:566: //  -- rbx    : the JSGeneratorObject to
resume
On 2017/01/13 19:41:09, Dan Ehrenberg wrote:
> Presumably this would only be called on JSAsyncGeneratorObjects, right?

Yes, will update the comment. I don't think it's worth having another
AssertAsyncGeneratorObject macro though

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:581: kDontSaveFPRegs);
On 2017/01/13 19:41:09, Dan Ehrenberg wrote:
> Is the only difference here vs Generate_ResumeGeneratorTrampoline whether we
are
> saving and restoring from await_input? If so, it'd be nice to refactor this a
> little bit to reduce all this duplicated assembly.

I agree :D but... I kind of want to avoid adding another register of input to
the ResumeGenerator stub. I'm not sure why the ResumeGenerator stub is the one
updating these fields in the first place, tbh. Maybe Georg has an opinion on if
that could just be done in the callers of ResumeGenerator instead (which would
allow me to get rid of this extra resume function)

https://codereview.chromium.org/2622833002/diff/80001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/2622833002/diff/80001/src/runtime/runtime.h#n...
src/runtime/runtime.h:415: F(DefineDataPropertyInLiteral, 4, 1)               \
On 2017/01/13 19:41:09, Dan Ehrenberg wrote:
> This looks like a broken rebase.

didn't get any merge conflicts here last time I rebased, I think it's probably
just out of date, but I'll check on Monday

https://codereview.chromium.org/2622833002/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2622833002/diff/120001/src/objects.h#newcode7946
src/objects.h:7946: DECL_ACCESSORS(await_input, Object)
On 2017/01/13 19:41:09, Dan Ehrenberg wrote:
> Why is this added to JSGeneratorObject? I would expect it to only be added to
> JSAsyncGeneratorObject.

I don't think it matters a whole lot, but OK, will move it

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

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser-bas...
src/parsing/parser-base.h:2066: if (token == Token::MUL &&
allow_harmony_async_iteration() &&
On 2017/01/13 19:41:09, Dan Ehrenberg wrote:
> Style nit: Put the flag check first

Acknowledged.

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

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:4889: // output = Await(%_Call(iteratorReturn, iterator,
input));
On 2017/01/13 19:41:09, Dan Ehrenberg wrote:
> Comment nit: should be if (IteratorType == kAsync) { ... }

It kind of gives the impression that the desugaring conditionally Awaits based
on some runtime value, but it statically decides to Await or not depending on
whether it's an async iterator or not.

I haven't found a good way to word these yet, so I'll use whatever prose people
want here.

[Dan Ehrenberg, 2017-01-13 22:09:02]

The main thing here so far that's implemented differently from what I expected
is the way closures are used. Seems potentially OK, as the context is allocated
and used within the same file, based on shared constants. Another strategy would
be to use JSBoundFunctions. What do you think, Benedikt?

TODO(littledan): Review generators in depth

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:432: };
I'm a little surprised by this sort of usage of contexts. What if you used bound
functions instead? Ditto in builtins-async-iterator.cc

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:23: InternalResolvePromise(context,
wrapped_value, value);
On 2017/01/13 20:11:52, caitp wrote:
> On 2017/01/13 19:41:08, Dan Ehrenberg wrote:
> > Looks like you're fixing v8:5694 here, right?
> 
> yeah, this fixes the anba bug

What if we do that bug fix in a separate patch? We sort of deliberately have
that spec violation for now because fixing it will hurt performance; we could do
this fix conjunction with performance mitigations (like, the microtask queue
could be in TF, and we could have a super-cheap microtask queue item when this
operation is trivial). The fix doesn't seem urgent, and is a sort of separate
work item from the rest of this patch.

https://codereview.chromium.org/2622833002/diff/120001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2622833002/diff/120001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:19: };
Interesting new way to use contexts. I'm a little scared, especially since this
ValueUnwrapContext thing is not shared between other files which are involved in
making these closures. What if we used a JSBoundFunction--would that be a lot
slower?

Nit: How about enum class ValueUnwrapContext { kPromise... } some cute C++
syntax that does basically this?

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

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser-bas...
src/parsing/parser-base.h:3819: FunctionKind kind = kFunctionKinds[index];
And given the multiple duplicated copies, seems like you should lift this into a
common definition.

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser-bas...
src/parsing/parser-base.h:5604: BlockT init_block =
impl()->RewriteForVarInLegacy(for_info);
The grammar doesn't permit `for await (var i = j in z)`, so this line won't have
any effect.

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

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:2102: int next_result_pos) {
This is really similar to  Parser::InitializeForOfStatement . Should that just
take an IteratorType argument?

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:4070: Yield::kOnExceptionRethrow, Yield::kAwait);
Why don't you have to apply the same sort of block thing that the rest of this
function does in order to get single stepping to work as expected? I would've
expected that your modification here would be to just switch between the two
different runtime function indices, and otherwise do all the same things? (Even
if you're not passing the Promise for now, presumably you'll need to once catch
prediction support is added.)

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:4889: // output = Await(%_Call(iteratorReturn, iterator,
input));
On 2017/01/13 20:11:52, caitp wrote:
> On 2017/01/13 19:41:09, Dan Ehrenberg wrote:
> > Comment nit: should be if (IteratorType == kAsync) { ... }
> 
> It kind of gives the impression that the desugaring conditionally Awaits based
> on some runtime value, but it statically decides to Await or not depending on
> whether it's an async iterator or not.
> 
> I haven't found a good way to word these yet, so I'll use whatever prose
people
> want here.

I was just copying the way that you wrote the syntax above, which already seemed
fine to me, but good point about the compile time vs runtime mismatch. Anyway,
for a strawman way to do the syntax in these comments, what if we use something
traditional and conservative--IE conditional comments!
https://msdn.microsoft.com/en-us/library/ms537512(v=vs.85).aspx

[if IteratorType == kAsync]
  output = Await(output)
[endif]

Anyway, I don't think it's a big deal which syntax you use, but it's nicer if
some sort of conditionality is explicit.

[caitp, 2017-01-13 22:33:31]

No strong opinion on the closure approach vs bound functions, but i __believe__
there's a bit more overhead to invoke bound functions that are not inlined, if
there's any difference at all.

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:23: InternalResolvePromise(context,
wrapped_value, value);
On 2017/01/13 22:09:02, Dan Ehrenberg wrote:
> On 2017/01/13 20:11:52, caitp wrote:
> > On 2017/01/13 19:41:08, Dan Ehrenberg wrote:
> > > Looks like you're fixing v8:5694 here, right?
> > 
> > yeah, this fixes the anba bug
> 
> What if we do that bug fix in a separate patch? We sort of deliberately have
> that spec violation for now because fixing it will hurt performance; we could
do
> this fix conjunction with performance mitigations (like, the microtask queue
> could be in TF, and we could have a super-cheap microtask queue item when this
> operation is trivial). The fix doesn't seem urgent, and is a sort of separate
> work item from the rest of this patch.

I'm not sure what you mean, this only fixes the anba bug for async generators,
per the proposal text. Changing Await in async functions will be a different
patch for sure

https://codereview.chromium.org/2622833002/diff/120001/src/builtins/builtins-...
File src/builtins/builtins-async-iterator.cc (right):

https://codereview.chromium.org/2622833002/diff/120001/src/builtins/builtins-...
src/builtins/builtins-async-iterator.cc:19: };
On 2017/01/13 22:09:02, Dan Ehrenberg wrote:
> Interesting new way to use contexts. I'm a little scared, especially since
this
> ValueUnwrapContext thing is not shared between other files which are involved
in
> making these closures. What if we used a JSBoundFunction--would that be a lot
> slower?
> 
> Nit: How about enum class ValueUnwrapContext { kPromise... } some cute C++
> syntax that does basically this?

I believe bound functions are slower to call from unoptimized code, based on my
understanding of it --- and this also wouldn't change much, would just be
allocating a different container rather than a context, no?

The main reason I'm not using enum class syntax here is that it's a pain to
convert enum class values to underlying types / is harder to read code where
that is done. These classes solve the scoping issue without introducing a strong
type, since it doesn't really need a strong type.

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

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:2102: int next_result_pos) {
On 2017/01/13 22:09:02, Dan Ehrenberg wrote:
> This is really similar to  Parser::InitializeForOfStatement . Should that just
> take an IteratorType argument?

I'm undecided on this.

I think the code is easier to  understand when separated, but since i opted to
pass the IteratorType for other methods to avoid duplication, it's probably fine
to reuse it for for-of

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:4070: Yield::kOnExceptionRethrow, Yield::kAwait);
On 2017/01/13 22:09:02, Dan Ehrenberg wrote:
> Why don't you have to apply the same sort of block thing that the rest of this
> function does in order to get single stepping to work as expected? I would've
> expected that your modification here would be to just switch between the two
> different runtime function indices, and otherwise do all the same things?
(Even
> if you're not passing the Promise for now, presumably you'll need to once
catch
> prediction support is added.)

I haven't tested any of this with the inspector yet, but I'd like to minimize
the number of new runtime mechanics in the parser, as it doesn't seem like a
sensible place for them to me. If debugging is awful with it, it will be
improved, but hopefully not using do expressions or the parser

[Dan Ehrenberg, 2017-01-14 00:48:39]

I still have to review the TF builtins in more detail, but from what I've seen
so far, I'm happy with the overall design of this change.

Suggestion for breaking up into patches:
1. Move await into a TF builtin for async functions (whether you want to fix
v8:5694 as part of the same change or separately later)
2. for await loops
3. Async generators

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:23: InternalResolvePromise(context,
wrapped_value, value);
On 2017/01/13 22:33:30, caitp wrote:
> On 2017/01/13 22:09:02, Dan Ehrenberg wrote:
> > On 2017/01/13 20:11:52, caitp wrote:
> > > On 2017/01/13 19:41:08, Dan Ehrenberg wrote:
> > > > Looks like you're fixing v8:5694 here, right?
> > > 
> > > yeah, this fixes the anba bug
> > 
> > What if we do that bug fix in a separate patch? We sort of deliberately have
> > that spec violation for now because fixing it will hurt performance; we
could
> do
> > this fix conjunction with performance mitigations (like, the microtask queue
> > could be in TF, and we could have a super-cheap microtask queue item when
this
> > operation is trivial). The fix doesn't seem urgent, and is a sort of
separate
> > work item from the rest of this patch.
> 
> I'm not sure what you mean, this only fixes the anba bug for async generators,
> per the proposal text. Changing Await in async functions will be a different
> patch for sure

I would prefer if we didn't check in code to have two implementations of await
at the same time, unless there's a good reason. This just gives us more surface
to have bugs, and less test coverage. If you first move await to this form, then
you'll get all the test coverage of async/await right off the bat. Definitely we
want to be aligned with the spec and implemented in TF long-term, just a
question of ordering.

If you need await to be implemented this way for async generators to work, then
first landing a patch to change it for async/await would be good. (On the other
hand, in the short term, I don't see why you couldn't call out to something very
close to the existing await function here. It seems like you'd just need to
construct the closures in the caller, rather than inlined here, and then call
out to the context index.)

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:39: LoadContextElement(native_context,
on_reject_context_index + 1);
On 2017/01/13 20:11:52, caitp wrote:
> On 2017/01/13 19:41:08, Dan Ehrenberg wrote:
> > Could you put a comment cross-referencing here with the places that have the
> two
> > adjacent context elements, to make sure that we maintain this property? It
> > doesn't look like AsyncFunction{Next,Throw} even have context indices; are
> those
> > the ones you'd use when calling this from ordinary async functions?
> > 
> > If you end up deciding to check in async iterators/generators *before*
making
> > this change to async functions, maybe you should just remove the generality
> here
> > and refer explicitly to the particular context indices you're interested in,
> > rather than taking this on_reject_context_index argument.
> 
> AsyncFunctionNext / Throw are aliases for GeneratorNext / Throw, and are
> exported to JS builtins. When/if those are ported to use this shared Await
> thing, they'll be installed to the native context instead, and likely
> implemented similar to the AsyncGeneratorAwaitResolveClosure/RejectClosure
> functions (but slightly different).

What if we just had Await() take two arguments, one for on_reject_context_index
and one for on_resolve_context_index? Then this would avoid the cross-cutting
dependency on their adjacency, which isn't apparent to someone who's making
changes to src/context.h.

https://codereview.chromium.org/2622833002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2309:
->CallRuntime(Runtime::kInlineGeneratorGetAwaitInput, generator)
Nit: 'GeneratorGetAwaitInput' is a sort of funny name; we are getting the input
for the last yield *across* an await, right? How about
AsyncGeneratorGetYieldInput (and, correspondingly,
AsyncGeneratorStoreYieldInput)?

Idle thought: Will TF be able to realize that, if you have multiple awaits, then
some of the reads and writes to this slot are dead? Probably this will not be
the bottleneck anytime soon, though, given that you have to go through a Promise
to get from one step to the next.

https://codereview.chromium.org/2622833002/diff/120001/src/builtins/builtins-...
File src/builtins/builtins-async-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/120001/src/builtins/builtins-...
src/builtins/builtins-async-generator.cc:439:
Context::ASYNC_GENERATOR_AWAIT_RESOLVE_SHARED_FUN);
Oh sorry I had previously missed this--I guess you have the safety covered.
Still, I'm wondering if things would all be simpler if you passed in two Node*s
which will evaluate to JSFunctions, rather than closure_context and
reject_index, and handled setting that up in the caller.

https://codereview.chromium.org/2622833002/diff/120001/src/builtins/builtins-...
src/builtins/builtins-async-generator.cc:453: Node* request) {
+gsathya would it make sense to share this (admittedly very simple) queue
implementation with the future implementation of the microtask queue?

https://codereview.chromium.org/2622833002/diff/120001/src/builtins/builtins-...
src/builtins/builtins-async-generator.cc:492: // JSAsyncGeneratorObject's queue.
Asserts that the queue is not empty.
Ignorable style suggestion: For a comment that describes what a function does,
put it above the function, rather than before a line (because then it looks like
it describes what that line does).

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

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:2102: int next_result_pos) {
On 2017/01/13 22:33:30, caitp wrote:
> On 2017/01/13 22:09:02, Dan Ehrenberg wrote:
> > This is really similar to  Parser::InitializeForOfStatement . Should that
just
> > take an IteratorType argument?
> 
> I'm undecided on this.
> 
> I think the code is easier to  understand when separated, but since i opted to
> pass the IteratorType for other methods to avoid duplication, it's probably
fine
> to reuse it for for-of

For me, it's easier to understand code when it's not duplicated, at least when
it's in a large amount like this. When there's so much copy-paste, it takes me a
while to go through everything to find what the difference is between the two
sections. At least that was my experience reviewing this patch.

https://codereview.chromium.org/2622833002/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:4070: Yield::kOnExceptionRethrow, Yield::kAwait);
On 2017/01/13 22:33:30, caitp wrote:
> On 2017/01/13 22:09:02, Dan Ehrenberg wrote:
> > Why don't you have to apply the same sort of block thing that the rest of
this
> > function does in order to get single stepping to work as expected? I
would've
> > expected that your modification here would be to just switch between the two
> > different runtime function indices, and otherwise do all the same things?
> (Even
> > if you're not passing the Promise for now, presumably you'll need to once
> catch
> > prediction support is added.)
> 
> I haven't tested any of this with the inspector yet, but I'd like to minimize
> the number of new runtime mechanics in the parser, as it doesn't seem like a
> sensible place for them to me. If debugging is awful with it, it will be
> improved, but hopefully not using do expressions or the parser

I'm confused; if you wanted to minimize the amount of mechanics in the parser,
why not just do the exact same thing that the rest of this does? It would mean
that this patch just adds one line here instead of 12. I don't think Yang or I
were happy to put this logic in the parser, but that's where it goes in the
current system. When we refactor things to not have the logic in the parser,
then we can do it for both async functions and async generators.

[Dan Ehrenberg, 2017-01-14 00:54:03]

Oh sorry for my confusion; using contexts this way is following Benedikt's
suggestion, applied in Promises. No need to change.

[caitp, 2017-01-17 19:23:11]

I've done some refactoring to try to duplicate code less.

Builtins::ResumeGenerator() is now the meat of ResumeGeneratorTrampoline and
ResumeAwaitedGeneratorTrampoline(), and just picks which generator field to get
the "value" from.

It changes the JSGeneratorStore operator to take an extra "SuspendType"
parameter indicating what field holds the input position, but still shares most
of the work. This allows the transformation of 2 bytecodes (SuspendGenerator and
AwaitGenerator) into a single operation in TF, since they are virtually the same
thing.

Some other misc stuff in builtins-async-generator.cc has been simplified a bit.

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1412:
isolate->builtins()->builtin(Builtins::kAsyncIteratorValueUnwrap),
On 2017/01/13 14:36:03, caitp wrote:
> On 2017/01/13 13:34:34, jgruber wrote:
> > Nit: isolate->builtins()->AsyncIteratorValueUnwrap()
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2622833002/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1455: Builtins::kAsyncGeneratorAwaitResolveClosure),
On 2017/01/13 13:34:33, jgruber wrote:
> Same here and below.

Done.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
File src/builtins/builtins-async-generator.cc (right):

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:83: Node* const iter_result =
AllocateJSIteratorResult(context, value, false);
On 2017/01/13 14:36:04, caitp wrote:
> On 2017/01/13 13:34:39, jgruber wrote:
> > I just recently landed CreateIterResultObject, perhaps we can merge these.
> 
> Sgtm

Done.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:257: StoreObjectField(generator,
JSAsyncGeneratorObject::kContinuationOffset,
On 2017/01/13 13:34:36, jgruber wrote:
> I guess we could use the NoWriteBarrier variant here.

Done.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:262: Branch(WordEqual(continuation,
On 2017/01/13 13:34:38, jgruber wrote:
> SmiEqual here and at other smi comparisons.

Done.

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:366: StoreObjectField(request,
AsyncGeneratorRequest::kAwaitedPromiseOffset,
On 2017/01/13 14:36:04, caitp wrote:
> On 2017/01/13 13:34:36, jgruber wrote:
> > NoWriteBarrier
> 
> Acknowledged.

and done

https://codereview.chromium.org/2622833002/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-async-generator.cc:372:
GotoUnless(SmiAboveOrEqual(continuation, SmiConstant(0)), &done);
On 2017/01/13 15:46:40, jgruber wrote:
> On 2017/01/13 14:36:04, caitp wrote:
> > On 2017/01/13 13:34:35, jgruber wrote:
> > > Isn't an unsigned '>= 0' comparison always true?
> > 
> > If the generator is executing or closed, no.
> 
> My point was that there is no way to make SmiAboveOrEqual(..., 0) return
false.
> SmiAboveOrEqual(-1, 0) --> true since the comparison is unsigned.
> Maybe you're looking for SmiGreaterThanOrEqual?

I understand now, per offline discussion. Changed this to use the signed compare
macro instead.

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async.cc (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.cc:39: LoadContextElement(native_context,
on_reject_context_index + 1);
On 2017/01/14 00:48:38, Dan Ehrenberg wrote:
> On 2017/01/13 20:11:52, caitp wrote:
> > On 2017/01/13 19:41:08, Dan Ehrenberg wrote:
> > > Could you put a comment cross-referencing here with the places that have
the
> > two
> > > adjacent context elements, to make sure that we maintain this property? It
> > > doesn't look like AsyncFunction{Next,Throw} even have context indices; are
> > those
> > > the ones you'd use when calling this from ordinary async functions?
> > > 
> > > If you end up deciding to check in async iterators/generators *before*
> making
> > > this change to async functions, maybe you should just remove the
generality
> > here
> > > and refer explicitly to the particular context indices you're interested
in,
> > > rather than taking this on_reject_context_index argument.
> > 
> > AsyncFunctionNext / Throw are aliases for GeneratorNext / Throw, and are
> > exported to JS builtins. When/if those are ported to use this shared Await
> > thing, they'll be installed to the native context instead, and likely
> > implemented similar to the AsyncGeneratorAwaitResolveClosure/RejectClosure
> > functions (but slightly different).
> 
> What if we just had Await() take two arguments, one for
on_reject_context_index
> and one for on_resolve_context_index? Then this would avoid the cross-cutting
> dependency on their adjacency, which isn't apparent to someone who's making
> changes to src/context.h.

I've done this (passing explicit context indexes for both closure SFIs)

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
File src/builtins/builtins-async.h (right):

https://codereview.chromium.org/2622833002/diff/80001/src/builtins/builtins-a...
src/builtins/builtins-async.h:10: class AsyncBuiltinsAssembler : public
PromiseBuiltinsAssembler {
On 2017/01/13 20:11:52, caitp wrote:
> On 2017/01/13 19:41:09, Dan Ehrenberg wrote:
> > I'm a little surprised by this inheritance, and don't see other similar
> > examples, but I don't see anything else that is trying to compose code like
> > this. Stupid suggestion: private inheritance?
> 
> Would that make CSA methods private, or just PromiseBuiltinsAssembler methods?

This causes problems for the TF_BUILTIN pattern, it would need to be `protected
PromiseBuiltinAssembler` --- but I don't think doing this helps with anything

[neis, 2017-01-18 12:26:22]

I'm afraid this CL is too huge for me to understand all the interactions and
give meaningful feedback.  If you could prepare a CL that implements for-await
only, I'd be happy to review that.  On top of that we can then look at async
generators.  Thanks!