Closed Bug 1331092 Opened 3 years ago Closed 3 years ago

Implement Async Iteration for web content on Nightly builds

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: shu, Assigned: arai)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(14 files, 16 obsolete files)

1.02 KB, patch
shu
: review+
Details | Diff | Splinter Review
1.15 KB, patch
shu
: review+
Details | Diff | Splinter Review
82.79 KB, patch
arai
: review+
Details | Diff | Splinter Review
39.61 KB, patch
arai
: review+
Details | Diff | Splinter Review
15.14 KB, patch
shu
: review+
Details | Diff | Splinter Review
12.80 KB, patch
jandem
: review+
Details | Diff | Splinter Review
11.93 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.13 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
8.37 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.70 KB, patch
shu
: review+
Details | Diff | Splinter Review
3.34 KB, patch
shu
: review+
Details | Diff | Splinter Review
2.31 KB, patch
shu
: review+
Details | Diff | Splinter Review
9.98 KB, patch
shu
: review+
Details | Diff | Splinter Review
781 bytes, patch
shu
: review+
Details | Diff | Splinter Review
https://github.com/tc39/proposal-async-iteration is stage 3, let's get some implementation experience.
Component: JavaScript → JavaScript Engine
Product: Developer Documentation → Core
so far, here's the list of things to be defined:
  * the difference between async function and async generator,
    in JSScript/LazyScript flags
    (currently unwrapped async function is also star generator)
  * the difference between await and yield inside async generator
    (currently await is converted to yield)

about the first one, I guess we should refactor GeneratorKind and FunctionAsyncKind.
there will be 5 kind of functions:
  * normal function (sync function)
  * legacy generator
  * star generator
  * async function
  * async generator

current GeneratorKind(2 bits) + FunctionAsyncKind(1 bit) have no space to express the difference between async function and async generator, as long as we keep using GeneratorKind==StarGenerator for async function.

on the other hand, we have 3 bits to express those 5 kinds, so if we stop using current enum and moved to different enum, it can be expressed without consuming extra space.

what I'm thinking is, using 2 bits for standard things:
  * normal function (sync function)
  * star generator
  * async function
  * async generator
(or 1 bit for generator, and 1 bit for async, separately)

and remaining 1 bit for legacy generator, so that we can free that 1 bit when we stop supporting legacy generator (not sure when it happens tho :P


I haven't yet figured out what's the best way to express the difference between await and yield...
(In reply to Tooru Fujisawa [:arai] from comment #1)
> I haven't yet figured out what's the best way to express the difference
> between await and yield...

we could add JSOP_AWAIT for await expression, and check current opcode to determine if it was yield or await.
in WIP patch, I'm using unmodified GeneratorKind and FunctionAsyncKind, but NotGenerator for async function and StarGenerator for async generator.
also using JSOP_YIELD/JSOP_AWAIT difference for checking whether the generator returns from yield or await.
current WIP patches:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bb6bc86e9f612826523c05b0b23dc23d7c8cf0d

TODO:
  * AsyncIteratorClose with JSTRY_ITERCLOSE note in for-await-of
    (we should do "await" in  IteratorCloseForException...?)
  * add tests
  * so many bug fix :P
I'm about to use try-finally instead of try-note for AsyncIteratorClose, to embed JSOP_AWAIT into bytecode.
bug 1322069 would be useful for it.
See Also: → 1322069
updated
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=104c6bac68ee7c0c818087d886899d567cb1c000

now AsyncIteratorClose uses try-catch, instead of try-finally, since we should only handle throwing case there,
and all other non-local-jump can be handled in NonLocalExitControl::prepareForNonLocalJump.

also it's based on bug 1322069 to add multiple try-catch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01163cd43c58adfe7adc5e0e002cdc9262ef2367

supported newly added opcodes in JIT, and also some fix.
I'm about to look the best way for testing (want to avoid creating duplicated tests between jstests and test262...
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
test262 for AsyncIteration is tracked in https://github.com/tc39/proposal-async-iteration/issues/69
I'll post tests there.
Summary: Implement async iterators → Implement Async Iteration
I'll post preliminary patches as a first step.
the whole patch series are in https://hg.mozilla.org/try/pushloghtml?changeset=c54e213af6d30a0d902f1be64c9fcd050e0bedd3

[Part 1 / Part 2]

I'll remove isStarGenerator from Async Function and use isStarGenerator+isAsync for Async Generator,
so that isStarGenerator means "*" in syntax, and isAsync means "async" in syntax.

before:
                   | isStarGenerator | isAsync | isLegacyGenerator
  -----------------+-----------------+---------+-------------------
  Function         | false           | false   | false
  Star Generator   | true            | false   | false
  Async Function   | true            | true    | false

  Legacy Generator | false           | false   | true

after:
                   | isStarGenerator | isAsync | isLegacyGenerator
  -----------------+-----------------+---------+-------------------
  Function         | false           | false   | false
  Star Generator   | true            | false   | false
  Async Function   | false           | true    | false
  Async Generator  | true            | true    | false

  Legacy Generator | false           | false   | true

then, several places needs to handle both {isStarGenerator,isAsync} cases, or all {isStarGenerator,isLegacyGenerator,isAsync} cases.
so I think isGenerator is misleading for such purpose, and just enumerating them, or adding more specific/descriptive accessors (needsFinalYield, needsDotGeneratorName, etc) should be better.

Part 1 removes isGenerator and replaces its consumer with equivalent conditions,
and Part 2 removes isStarGenerator flag from Async Function, and handle isAsync==true case in some places.
Attachment #8832998 - Flags: review?(shu)
Attachment #8832999 - Flags: review?(shu)
[Part 3 / Part 4]

In Async Generator, I need to check whether the result object {value,done} is returned from yield, await, or return.
I'm about to use the opcode pointed by `yieldAndAwaitOffsets[yieldAndAwaitIndex()]` (renamed from `yieldOffsets[yieldIndex()]`), to see which operation was the last thing done in the function.

(actually, there could be several more ways other than checking opcode itself, like, using dedicated object (with special clasp) for result object used by await, or store some flag in GeneratorObject and flip it in JSOP_YIELD/JSOP_AWAIT, etc..., but at least this patch seems to work)
Attachment #8833001 - Flags: review?(shu)
[Part 5]

Before adding Async Generator related things to Promise, renamed Async Function related things to say "AsyncFunction" explicitly.
Attachment #8833004 - Flags: review?(shu)
[Part 6]

Added native wrapper for GetInternalError/GetTypeError self-hosted functions, to simplify callsites.
I'll use GetTypeError in later patches.
Attachment #8833005 - Flags: review?(shu)
[Part 7]

Just added Symbol.asyncIterator.
Attachment #8833006 - Flags: review?(shu)
I guess I should've summarize the whole structure of async generator implementation before getting those parts reviewed.

 * it uses similar approach as async function, that is, 1 unwrapped function + 1 wrapped function.
   unwrapped function corresponds to scripted function that is compiled from the JS source code (as a star generator),
   and wrapped function is a native function that implements the interface, and calls unwrapped function when it gets called

 * async generator returns dedicated native object (AsyncGeneratorObject) that implements the interface,
   and operates on GeneratorObject returned by unwrapped function.

 * unwrapped function behaves almost same as star generator.
   yield and await behaves almost identically on that part.

 * after returned from StarGeneratorNext/StarGeneratorThrow/StarGeneratorReturn, it checks whether the last operation was yield/await/return,
   and does corresponding things (resolved already returned promise for yield/return, wait for the passed promise for await)

let me know if I'm missing something that needs explanation.
Comment on attachment 8832998 [details] [diff] [review]
Part 1: Remove {JSFunction,JSScript,LazyScript}.isGenerator() method.

Review of attachment 8832998 [details] [diff] [review]:
-----------------------------------------------------------------

Your comments are amazingly helpful, as always.
Attachment #8832998 - Flags: review?(shu) → review+
Comment on attachment 8832999 [details] [diff] [review]
Part 2: Stop using StarGegerator for async function.

Review of attachment 8832999 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8119,5 @@
>  {
>      if (!updateSourceCoordNotes(pn->pn_pos.begin))
>          return false;
>  
> +    bool needsResultObject = sc->isFunctionBox() && sc->asFunctionBox()->needsResultObject();

I'd prefer needsIteratorResult over needsResultObject.

::: js/src/frontend/Parser.cpp
@@ +2543,2 @@
>                   ? JSFunction::INTERPRETED_LAMBDA
>                   : JSFunction::INTERPRETED_LAMBDA_GENERATOR);

We should change INTERPRETED_LAMBDA_GENERATOR be changed to INTERPRETED_LAMBDA_GENERATOR_OR_ASYNC to be more accurate.
Attachment #8832999 - Flags: review?(shu) → review+
Comment on attachment 8833001 [details] [diff] [review]
Part 3: Add JSOP_AWAIT and rename {yieldIndex,yieldOffset} to {yieldAndAwaitIndex,yieldAndAwaitOffset}.

Review of attachment 8833001 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.h
@@ +227,5 @@
>      CGTryNoteList    tryNoteList;    /* list of emitted try notes */
>      CGScopeNoteList  scopeNoteList;  /* list of emitted block scope notes */
>  
>      /*
> +     * For each yield op, map the yield and await index (stored as bytecode

Nit: for each yield or await op

::: js/src/jit/BaselineCompiler.cpp
@@ +274,5 @@
>      // The last entry in the last index found, and is used to avoid binary
>      // searches for the sought entry when queries are in linear order.
>      bytecodeMap[script->nTypeSets()] = 0;
>  
> +    baselineScript->copyYieldEntries(script, yieldAndAwaitOffsets_);

Should rename copyYieldEntries as well.

::: js/src/vm/GeneratorObject.cpp
@@ +75,3 @@
>  
> +    if ((*pc == JSOP_YIELD || *pc == JSOP_AWAIT) &&
> +        genObj->isClosing() && genObj->is<LegacyGeneratorObject>())

Does this condition need changing? Legacy generators shouldn't be able to emit JSOP_AWAIT.

::: js/src/vm/GeneratorObject.h
@@ +138,3 @@
>      }
>      bool isClosing() const {
> +        return getFixedSlot(YIELD_AND_AWAIT_INDEX_SLOT).toInt32() == YIELD_INDEX_CLOSING;

For consistency, YIELD_AND_AWAIT_INDEX_RUNNING and YIELD_AND_AWAIT_INDEX_CLOSING.

::: js/src/vm/Opcodes.h
@@ +2132,5 @@
>       */ \
>      macro(JSOP_DEBUGAFTERYIELD,  208, "debugafteryield",  NULL,  1,  0,  0,  JOF_BYTE) \
> +    /*
> +     * Pops the generator and the return value 'rval1', stops interpretation and
> +     * returns 'rval1'. Pushes resolved value onto the stack.

Stale comment. 'rval1' -> 'promise'
Attachment #8833001 - Flags: review?(shu) → review+
Attachment #8833003 - Flags: review?(shu) → review+
Comment on attachment 8833004 [details] [diff] [review]
Part 5: Rename AsyncFunction-related names in Promise.cpp to explicitly say Async Function.

Review of attachment 8833004 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Promise.cpp
@@ +820,5 @@
>      RootedValue generatorVal(cx, resultPromise->getFixedSlot(PromiseSlot_AwaitGenerator));
>  
>      int32_t handlerNum = int32_t(handlerVal.toNumber());
> +    MOZ_ASSERT(handlerNum == PromiseHandlerAsyncFunctionAwaitFulfilled
> +               || handlerNum == PromiseHandlerAsyncFunctionAwaitRejected);

Style nit: that's a weird style, put || on previous line while you're here please.
Attachment #8833004 - Flags: review?(shu) → review+
Attachment #8833005 - Flags: review?(shu) → review+
Attachment #8833006 - Flags: review?(shu) → review+
here's remaining patches.

all of them has no testcase now, but at least this passes test262 (trunk and PRs),
I'm about to add testcases to test262.


[[Part 8]]

through this patch, many places do similar thing as async function, with minor difference.
we could merge them into single function with parameter or template tho,
I'd like to use different implementation as a first step.

some notes:

[parser]

Removed the code to reject async generator syntax.


[bytecode]

emit JSOP_TOASYNCGEN instead of JSOP_TOASYNC, for async generator.
that wraps unwapped function and pushes wrapped async generator.
we could use same opcode and branch inside it, but adding dedicated opcode is easier to handle it in JIT.


[objects]

Added the following objects:
  * %AsyncIteratorPrototype%
    https://tc39.github.io/proposal-async-iteration/#sec-asynciteratorprototype
  * %AsyncGeneratorPrototype%
    https://tc39.github.io/proposal-async-iteration/#sec-properties-of-asyncgenerator-prototype
  * %AsyncGenerator%
    https://tc39.github.io/proposal-async-iteration/#sec-properties-of-asyncgeneratorfunction-prototype
  * %AsyncGeneratorFunction%
    https://tc39.github.io/proposal-async-iteration/#sec-properties-of-asyncgeneratorfunction


[promise]

new PromiseHandler for:
  * Async Iterator Value Unwrap Functions
    (PromiseHandlerAsyncIteratorValueUnwrapDone,
     PromiseHandlerAsyncIteratorValueUnwrapNotDone)
    https://tc39.github.io/proposal-async-iteration/#sec-async-iterator-value-unwrap-functions
  * Await Fulfilled Functions
    (PromiseHandlerAsyncGeneratorAwaitFulfilled)
    https://tc39.github.io/proposal-async-iteration/#await-fulfilled
  * Await Rejected Functions
    (PromiseHandlerAsyncGeneratorAwaitRejected)
    https://tc39.github.io/proposal-async-iteration/#await-rejected

PromiseReactionRecord for async generator has ReactionRecordSlot_Generator slot, that contains AsyncGeneratorObject,
to pass it to actual handler functions (AsyncGeneratorAwaitedFulfilled, AsyncGeneratorAwaitedRejected)


[AsyncGeneratorResume]

AsyncGeneratorResume is called to resume async generator, for following cases:
  * when await resolves
  * when await rejects
  * AsyncGeneratorResumeNext
    https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresumenext

it resumes unwrapped function, and it returns with following 4 reasons:
  * yield
  * await
  * return
  * throw

"throw" case is detected by abrupt completion (CallSelfHostedFunction() == false),
other cases are detected by opcode pointed by PC (actually, previous opcode of PC that would point JSOP_DEBUGAFTERYIELD, for yield/await)


[own prototype property of async generator]

async generator instance has own prototype property, just like star generator.
the property should be defined on wrapped function, so ResolveInterpretedFunctionPrototype now accepts native function, only if it's wrapped async generator,
and defined object which prototype is %AsyncGeneratorPrototype%


[queue of AsyncGeneratorObject]

queue uses single slot, for single request or an array for multiple requests.

> +    // Queue is implemented in 2 ways.  If only one request is queued ever,
> +    // request is stored directly to the slot.  Once 2 requests are queued, an
> +    // array is created and requests are pushed into it, and the array is
> +    // stored to the slot.

current implementation is not optimized for queue with multiple requests, since
it just calls array functions (push, shift).
we could optimize it with 2 arrays or some other way when it's necessary,
but so far I'm not sure if it's common to enqueue multiple requests.
Attachment #8836282 - Flags: review?(shu)
[bytecode]

Added IteratorKind (Sync or Async) parameter to emitIteratorClose, to emit AsyncIteratorClose, that does await for values returned by iterator.
https://tc39.github.io/proposal-async-iteration/#sec-asynciteratorclose
also added "ParseNode* generator" parameter to use it while emitting JSOP_AWAIT.

emitAsyncIterator is added separately from emitIterator, since it does so much different things than emitIterator.
https://tc39.github.io/proposal-async-iteration/#sec-getiterator
it emits JSOP_TOASYNCITER that does CreateAsyncFromSyncIterator.

emitYieldStar is also changed to do await for values returned by iterator.
https://tc39.github.io/proposal-async-iteration/#prod-YieldExpression


[objects]

Added the following object:
  * %AsyncFromSyncIteratorPrototype%
    https://tc39.github.io/proposal-async-iteration/#sec-%asyncfromsynciteratorprototype%-object
Attachment #8836284 - Flags: review?(shu)
Attached patch Part 12: Implement for-await-of. (obsolete) — Splinter Review
Added ForAwaitOfLoopControl, that handles AsyncIteratorClose, like ForOfLoopControl,
but ForAwaitOfLoopControl uses try-catch, instead of try-note.
since we need to await inside it, and it's hard to handle it in try-note.

Added IteratorKind and `ParseNode* generator` parameter to emitIteratorNext, just like emitIteratorClose,
to do await.

Also, added CompletionKind to emitIteratorClose, to emit dedicated bytecode for AsyncIteratorClose with abrupt completion.

kid2 of PNK_FOROF was unused, so this patch uses kid2 to store dotGenerator name node, to pass it to JSOP_AWAIT.
Attachment #8836286 - Flags: review?(shu)
Comment on attachment 8836282 [details] [diff] [review]
Part 8: Implement Async Generator except yield*.

Review of attachment 8836282 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, I think I understand all of this now. It's a very faithful translation of the spec, and should be fun to optimize later.

::: js/src/builtin/Promise.cpp
@@ +40,5 @@
> +    PromiseHandlerAsyncGeneratorAwaitFulfilled,
> +    PromiseHandlerAsyncGeneratorAwaitRejected,
> +
> +    // Async Iteration proposal 6.1.1.2.1.
> +    // Both for [[Done]] internal slot is true and false cases.

This comment confused me. How about:

Async iterator handlers take the resolved value and create new iterator objects. To do so it needs to forward whether the iterator is done. In spec, this is achieved via the [[Done]] internal slot. We enumerate both true and false cases here.

@@ +860,5 @@
>  }
>  
> +static MOZ_MUST_USE bool
> +AsyncGeneratorAwaitPromiseReactionJob(JSContext* cx, Handle<PromiseReactionRecord*> reaction,
> +                                     MutableHandleValue rval)

Style nit: line up argument

@@ +888,5 @@
> +}
> +
> +// ES 2017 draft 7.4.7.
> +static JSObject*
> +CreateIterResultObject(JSContext* cx, HandleValue value, bool done)

There is already the strangely named and unused js::CreateItrResultObject in jsiter.{h,cpp}. Please rename that js::CreateIterResultObject, and update and use that instead.

@@ +2282,5 @@
>  
> +// Async Iteration proposal 5.1 steps 2-9.
> +MOZ_MUST_USE bool
> +js::AsyncGeneratorAwait(JSContext* cx, Handle<AsyncGeneratorObject*> asyncGenObj,
> +                        HandleValue value)

I'm fine with keeping the await implementations separate between functions and generators for now if that makes your patches easier to maintain. Could you file a followup bug to refactor?

::: js/src/frontend/Parser.cpp
@@ -8891,5 @@
>          }
>      }
>  
> -    if (isAsync && isGenerator) {
> -        error(JSMSG_ASYNC_GENERATOR);

JSMSG_ASYNC_GENERATOR shouldn't be used after this patch series, so please remove it.

::: js/src/jsfun.cpp
@@ +306,5 @@
>  
>          JSFunction* callerFun = &callerObj->as<JSFunction>();
>          if (IsWrappedAsyncFunction(callerFun))
>              callerFun = GetUnwrappedAsyncFunction(callerFun);
> +        if (IsWrappedAsyncGenerator(callerFun))

else if

@@ +1637,5 @@
> +            introductionType = "AsyncGenerator";
> +        else
> +            introductionType = "AsyncFunction";
> +    }
> +    else if (generatorKind != NotGenerator) {

Style nit: } else if (...) {

@@ +1829,5 @@
> +js::AsyncGeneratorConstructor(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    // Save the callee before its reset in FunctionConstructor().

Typo: it's

::: js/src/vm/AsyncIteration.cpp
@@ +137,5 @@
> +
> +// Async Iteration proposal 5.1.2 Await Rejected Functions.
> +MOZ_MUST_USE bool
> +js::AsyncGeneratorAwaitedRejected(JSContext* cx, Handle<AsyncGeneratorObject*> asyncGenObj,
> +                             HandleValue reason)

Style nit: alignment

::: js/src/vm/AsyncIteration.h
@@ +125,5 @@
> +    }
> +    void setSingleQueueRequest(AsyncGeneratorRequest* request) {
> +        setFixedSlot(Slot_QueueOrRequest, ObjectValue(*request));
> +    }
> +    void resetSingleQueueRequest() {

How about "clear" instead of "reset"?
Attachment #8836282 - Flags: review?(shu) → review+
Comment on attachment 8836284 [details] [diff] [review]
Part 10: Implement Async Generator yield*.

Review of attachment 8836284 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Promise.cpp
@@ +2281,5 @@
>  }
>  
> +// ES 2017 draft 7.4.2.
> +static bool
> +IteratorNext(JSContext* cx, HandleObject iter, HandleValue value, MutableHandleObject resultObj)

Hm it's kind of weird that this function is here. I'm surprised we didn't have a C++ implementation of this already.

@@ +2387,5 @@
> +
> +    // Steps 5-6.
> +    RootedObject resultObj(cx);
> +    if (completionKind == CompletionKind::Normal) {
> +        if (!IteratorNext(cx, iter, args.get(0), &resultObj))

IteratorNext isn't used anywhere except here, right? Could you inline most of it and factor out the ThrowCheckIsObject part?

@@ +2441,5 @@
> +            return AbruptRejectPromise(cx, args, resultPromise, nullptr);
> +
> +        // Steps 10.
> +        if (!resultVal.isObject()) {
> +            MOZ_ALWAYS_FALSE(ThrowCheckIsObject(cx, CheckIsObjectKind::IteratorNext));

CheckIsObjectKind should differ depending on completionKind here.

@@ +2464,5 @@
> +        return AbruptRejectPromise(cx, args, resultPromise, nullptr);
> +    bool done = ToBoolean(doneVal);
> +
> +    // Step 11.
> +    Rooted<PromiseObject*> promise(cx, CreatePromiseObjectWithoutResolutionFunctions(cx));

I don't quite understand in the spec why this promise is made with the iterator value unwrap functions, then resolved immediately. Is it just for ease of chaining into the result promise? Isn't this part all sync?

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5240,5 @@
> +            if (!emitTree(generator))                     // ... FTYPE FVALUE RESULT GEN
> +                return false;
> +            if (!emitYieldOp(JSOP_AWAIT))                 // ... FTYPE FVALUE RESULT
> +                return false;
> +        }

This await and the one below are for the yield* cases, right? I don't see an await for non-yield* AsyncIteratorClose.

For AsyncIteratorClose, the spec says Await(innerResult.[[Value]]). First, is the spec wrong and is that supposed to be Await(innerResult), like for the .next case? Second, here's what I understand this snippet to be doing in pseudocode:

  innerResult = iter.return(value);
  newInnerResult = { value: innerResult, done: false };
  innerResult = await newInnerResult;

That doesn't seem right, or am I reading this wrong?

@@ +6754,5 @@
> +        return false;
> +    if (!emitCall(JSOP_CALLITER, 0))                              // ITER
> +        return false;
> +    checkTypeSet(JSOP_CALLITER);
> +    if (!emitCheckIsObj(CheckIsObjectKind::GetIterator))          // ITER

Should add GetAsyncIterator

@@ +8428,5 @@
> +        if (!emitTree(gen))                                      // ITER OLDRESULT RESULT GEN
> +            return false;
> +        if (!emitYieldOp(JSOP_AWAIT))                            // ITER OLDRESULT RESULT
> +            return false;
> +    }

Same question as above, this seems to be repackaging an iterator result object into a new iterator result and awaiting that instead of just awaiting the iterator result.

@@ +8482,5 @@
> +        if (!emitTree(gen))                                      // ITER RESULT RESULT GEN
> +            return false;
> +        if (!emitYieldOp(JSOP_AWAIT))                            // ITER RESULT
> +            return false;
> +    }

Ditto.
Attachment #8836284 - Flags: review?(shu)
Comment on attachment 8836284 [details] [diff] [review]
Part 10: Implement Async Generator yield*.

Review of attachment 8836284 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5240,5 @@
> +            if (!emitTree(generator))                     // ... FTYPE FVALUE RESULT GEN
> +                return false;
> +            if (!emitYieldOp(JSOP_AWAIT))                 // ... FTYPE FVALUE RESULT
> +                return false;
> +        }

Ah I just understood what's going on. Async iterators return Promises, and since we compile awaits as yields that expect the value to implement the IteratorResult interface, we need to wrap the returned Promise into a not-done IteratorResult.

Could you please refactor this bit out to a method on BCE with a comment explaining what it's doing?
Attachment #8836284 - Flags: review+
Comment on attachment 8836286 [details] [diff] [review]
Part 12: Implement for-await-of.

Review of attachment 8836286 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not happy with the extra try/catch stuff. Let me think on the approach for a bit.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1912,5 @@
> +    //     // try-block.
> +    //     try {
> +    //       ...
> +    //       if (...) {
> +    //         // 1. process StatementKind::Finally

Where is finally used? I don't see it.

@@ +1938,5 @@
> +        generator_(generator)
> +    {
> +    }
> +
> +    bool emitBefore(BytecodeEmitter* bce) {

The before/after naming isn't very clear. I'm not sure what would be better right now.

@@ +1941,5 @@
> +
> +    bool emitBefore(BytecodeEmitter* bce) {
> +        tryCatch_.emplace(bce, TryEmitter::TryCatch, TryEmitter::DontUseRetVal);
> +
> +        if (!tryCatch_.ref().emitTry())

if (!tryCatch_->emitTry())

Please change the other places where you use ref() as well.

@@ +1956,5 @@
> +        unsigned slotFromTop = bce->stackDepth - iterDepth_;
> +        if (!bce->emitDupAt(slotFromTop))         // ITER ... EXCEPTION ITER
> +            return false;
> +        IteratorKind kind = IteratorKind::Async;
> +        ParseNode* gen = generator();

Is this always just the name '.generator'? You could remove this and all other instances where you pass along a ParseNode and manually do something like the following where you need it:

  NameLocation loc = *locationOfNameBoundInFunctionScope(cx->names().dotGenerator);
  if (!emitGetNameAtLocation(cx->names().dotGenerator, loc))
      return false;

@@ +5284,5 @@
>          return false;
> +
> +    if (kind == IteratorKind::Async) {
> +        if (!emitToIteratorResult(false))                 // ... RESULT
> +            return false;

I'm confused by this for the same reason as the previous patch. How does wrapping it in a new { value, done } let you await?

@@ +7130,5 @@
> +    MOZ_ASSERT_IF(iterKind == IteratorKind::Async, sc->asFunctionBox()->isAsync());
> +
> +    ParseNode* generator = forOfHead->pn_kid2;
> +    MOZ_ASSERT_IF(iterKind == IteratorKind::Async, generator);
> +    MOZ_ASSERT_IF(iterKind == IteratorKind::Sync, !generator);

Like above, it would be cleaner to manually emit .generator names instead of carrying around an extra ParseNode in for-of nodes.
Thank you for reviewing :)

(In reply to Shu-yu Guo [:shu] from comment #27)
> @@ +2464,5 @@
> > +        return AbruptRejectPromise(cx, args, resultPromise, nullptr);
> > +    bool done = ToBoolean(doneVal);
> > +
> > +    // Step 11.
> > +    Rooted<PromiseObject*> promise(cx, CreatePromiseObjectWithoutResolutionFunctions(cx));
> 
> I don't quite understand in the spec why this promise is made with the
> iterator value unwrap functions, then resolved immediately. Is it just for
> ease of chaining into the result promise? Isn't this part all sync?

uh, I don't understand the reasoning or underlying requirement.
I'll ask in #jslang.


> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +5240,5 @@
> > +            if (!emitTree(generator))                     // ... FTYPE FVALUE RESULT GEN
> > +                return false;
> > +            if (!emitYieldOp(JSOP_AWAIT))                 // ... FTYPE FVALUE RESULT
> > +                return false;
> > +        }
> 
> This await and the one below are for the yield* cases, right? I don't see an
> await for non-yield* AsyncIteratorClose.

non-yield* part is changed in Part 12.


> For AsyncIteratorClose, the spec says Await(innerResult.[[Value]]). First,
> is the spec wrong and is that supposed to be Await(innerResult), like for
> the .next case? Second, here's what I understand this snippet to be doing in
> pseudocode:
> 
>   innerResult = iter.return(value);
>   newInnerResult = { value: innerResult, done: false };
>   innerResult = await newInnerResult;
> 
> That doesn't seem right, or am I reading this wrong?

(I think you've already understood.  just for future reference)

before this patch series, unwrapped function of Async Function was purely a star generator.  "await" was implemented with JSOP_YIELD, and both "await" and "return creates result object.  The result object's "done" property is used to determine if the generator returned from "await" or "return".

Then, Part 3 added JSOP_AWAIT, and now we can detect how the generator returns, without using result object's "done" property:
  * from yield - GeneratorObject::isAfterYield()
  * from await - GeneratorObject::isAfterAwait()
  * from throw - abrupt completion
  * from return - other case

so, basically we no more need result object for "await" and "return".
I'm about to optimize them out in bug 1316098.
(In reply to Shu-yu Guo [:shu] from comment #29)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +1912,5 @@
> > +    //     // try-block.
> > +    //     try {
> > +    //       ...
> > +    //       if (...) {
> > +    //         // 1. process StatementKind::Finally
> 
> Where is finally used? I don't see it.

sorry, it was stale comment.
it should say something like "leave try-catch block by popping stack value".


> @@ +1938,5 @@
> > +        generator_(generator)
> > +    {
> > +    }
> > +
> > +    bool emitBefore(BytecodeEmitter* bce) {
> 
> The before/after naming isn't very clear. I'm not sure what would be better
> right now.

how about emitBeginIteratorCloseTarget/emitEndIteratorCloseTarget ?


> @@ +1956,5 @@
> > +        unsigned slotFromTop = bce->stackDepth - iterDepth_;
> > +        if (!bce->emitDupAt(slotFromTop))         // ITER ... EXCEPTION ITER
> > +            return false;
> > +        IteratorKind kind = IteratorKind::Async;
> > +        ParseNode* gen = generator();
> 
> Is this always just the name '.generator'? You could remove this and all
> other instances where you pass along a ParseNode and manually do something
> like the following where you need it:
> 
>   NameLocation loc =
> *locationOfNameBoundInFunctionScope(cx->names().dotGenerator);
>   if (!emitGetNameAtLocation(cx->names().dotGenerator, loc))
>       return false;

Yes, it's to emit '.generator' reference, just like TOK_YIELD and TOK_AWAIT.
Does it apply for PNK_YIELD (that has dotGenerator name in pn_right) too?
apparently enclosing `x = next().value` part of `for (let x of y) { ... }` with try-catch cannot be done simply, if we need to support Ion.

it breaks the following assertion:
  https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/js/src/jit/MIR.cpp#2601


with try-catch way, it generates something like following:

  let x = uninitialized;
  tmp = IteratorNext();
  try {
    x = tmp;
  } catch (e) {
    IteratorClose();
    throw e;
  }
  // for-of body, use v here


looks like Ion assumes that `x` is uninitialized after the try-catch :/

since generator itself is not compatible with Ion, the issue doesn't happen with for-await-of tho, if we use try-catch for for-of, it hits the issue.


I'll try investigating it.
(In reply to Tooru Fujisawa [:arai] from comment #30)
> Thank you for reviewing :)
> 
> (In reply to Shu-yu Guo [:shu] from comment #27)
> > @@ +2464,5 @@
> > > +        return AbruptRejectPromise(cx, args, resultPromise, nullptr);
> > > +    bool done = ToBoolean(doneVal);
> > > +
> > > +    // Step 11.
> > > +    Rooted<PromiseObject*> promise(cx, CreatePromiseObjectWithoutResolutionFunctions(cx));
> > 
> > I don't quite understand in the spec why this promise is made with the
> > iterator value unwrap functions, then resolved immediately. Is it just for
> > ease of chaining into the result promise? Isn't this part all sync?
> 
> uh, I don't understand the reasoning or underlying requirement.
> I'll ask in #jslang.

caitp told me that it's to accept thennable for yielded value:

  function* g() {
    yield a_promise_that_will_be_resolved_to(10);
  }
  ...
  for await (let x of g()) {
    // x becomes 10, instead of the promise above.
  }
(In reply to Tooru Fujisawa [:arai] from comment #31)
> 
> > @@ +1938,5 @@
> > > +        generator_(generator)
> > > +    {
> > > +    }
> > > +
> > > +    bool emitBefore(BytecodeEmitter* bce) {
> > 
> > The before/after naming isn't very clear. I'm not sure what would be better
> > right now.
> 
> how about emitBeginIteratorCloseTarget/emitEndIteratorCloseTarget ?
> 

That's better, but I'm still not clear on what "target" means in this context.
(In reply to Tooru Fujisawa [:arai] from comment #32)
> apparently enclosing `x = next().value` part of `for (let x of y) { ... }`
> with try-catch cannot be done simply, if we need to support Ion.
> 
> it breaks the following assertion:
>  
> https://dxr.mozilla.org/mozilla-central/rev/
> 0eef1d5a39366059677c6d7944cfe8a97265a011/js/src/jit/MIR.cpp#2601
> 
> 
> with try-catch way, it generates something like following:
> 
>   let x = uninitialized;
>   tmp = IteratorNext();
>   try {
>     x = tmp;
>   } catch (e) {
>     IteratorClose();
>     throw e;
>   }
>   // for-of body, use v here
> 
> 
> looks like Ion assumes that `x` is uninitialized after the try-catch :/
> 
> since generator itself is not compatible with Ion, the issue doesn't happen
> with for-await-of tho, if we use try-catch for for-of, it hits the issue.
> 
> 
> I'll try investigating it.

this was just because I was using 2 try-catch, each for head and body (to maintain the different stack depth)
we could tweak the stack to unify them.
that will solve the duplicated catch block issue too.
changed ForOfLoopControl to use try-catch, and separated the patch
try is running:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a581cd8f2c7e3d1615fbb1287c4f0b622881fdc4
added some more test cases.

the result varies for throwing case.
17.3% slower on worst case and 24.2% faster on best case,
depending on where the code throws.

I'm not sure if hot code throws so much in wild tho...


code:

// simple loop
function A() {
    var A = [];
    for (var i = 0; i < 30000000; i++)
        A.push(i);
    var x = 0;
    for (var a of A)
        x += a;
    return x;
}

// loop with destructuring in head
function B() {
    var A = [];
    for (var i = 0; i < 3000000; i++)
        A.push([i, i, i]);
    var x = 0;
    for (var [a, b, c] of A)
        x += a;
    return x;
}

// break
function C() {
    var A = [1, 2, 3];
    var x = 0;
    for (var i = 0; i < 10000000; i++) {
        for (var a of A) {
            x += a;
            if (a == 2)
                break;
        }
    }
    return x;
}

// throw after 1 loop
function D() {
    var A = [1, 2, 3];
    var x = 0;
    for (var i = 0; i < 1000000; i++) {
        try {
            for (var a of A) {
                x += a;
                if (a == 2)
                    throw 10;
            }
        } catch (e) {
        }
    }
    return x;
}

// throw after many loop
function E() {
    var A = [];
    for (var i = 0; i < 3000; i++)
        A.push([i, i, i]);
    var x = 0;
    for (i = 0; i < 200; i++) {
        try {
            for (var a of A) {
                x += a;
                if (a == 2900)
                    throw 10;
            }
        } catch (e) {
        }
    }
    return x;
}

// throw in next
function F() {
    var A = {
      [Symbol.iterator]() {
        return this;
      },
      next() {
        throw 10;
      }
    };
    var x = 0;
    for (var i = 0; i < 1000000; i++) {
        try {
            for (var a of A) {
                x += a;
            }
        } catch (e) {
        }
    }
    return x;
}

// throw in return
function G() {
    var A = {
      [Symbol.iterator]() {
        return this;
      },
      next() {
        return { value: 10, done: false };
      },
      return() {
        throw 10;
      },
    };
    var x = 0;
    for (var i = 0; i < 1000000; i++) {
        try {
            for (var a of A) {
                break;
            }
        } catch (e) {
        }
    }
    return x;
}

// throw in return getter
function H() {
    var A = {
      [Symbol.iterator]() {
        return this;
      },
      next() {
        return { value: 10, done: false };
      },
      get return() {
        throw 10;
      },
    };
    var x = 0;
    for (var i = 0; i < 1000000; i++) {
        try {
            for (var a of A) {
                break;
            }
        } catch (e) {
        }
    }
    return x;
}

result:
     | try-note [us] | try-catch [us]
  ---+---------------+----------------------
   A | 734627        | 766428 (+  4.3 %)
   B | 510020        | 512687 (+  0.5 %)
   C | 683960        | 700039 (+  2.4 %)
   D | 619942        | 727185 (+ 17.3 %)
   E | 950225        | 966191 (+  1.7 %)
   F | 650356        | 513020 (- 21.1 %)
   G | 738028        | 726812 (-  1.5 %)
   H | 735558        | 557719 (- 24.2 %)
Changed ForOfLoopControl implementation to ForAwaitOfLoopControl's one that uses try-catch.

here's difference from previous ForAwaitOfLoopControl:
  1. for-head and for-body are now in single try-catch block

  2. stack layout is changed to balance stack around loop and try-catch.
     now "result.value" is also pushed to the stack before the loop,
     and the depth of for-of loop is 3 instead of 2

  3. ITER value on the stack is cleared to UNDEFINED before non-local jump,
     and catch block checks the ITER value, and skip IteratorClose if
     ITER is UNDEFINED

  4. code in NonLocalExitControl::prepareForNonLocalJump is moved to
     ForOfLoopControl::emitPrepareForNonLocalJump, to reduce the duplication

Also, some fix:
  5. in BytecodeEmitter::emitIteratorClose with CompletionKind::Throw,
     check if the iter.return is callable, outside of try-catch for JSOP_CALL

     https://tc39.github.io/ecma262/#sec-iteratorclose

     7.4.6 IteratorClose ( iterator, completion )
       ...
       3. Let return be ? GetMethod(iterator, "return").
       4. If return is undefined, return Completion(completion).
       5. Let innerResult be Call(return, iterator, « »).
       6. If completion.[[Type]] is throw, return Completion(completion).
       7. If innerResult.[[Type]] is throw, return Completion(innerResult).

     in CompletionKind::Normal case, this was done in JSOP_CALL for Step 5,
     but we should check if callable in Step 3, and the failure there should
     throw immediately, ignoring completion.[[Type]].

  6. to implement above, added JSOP_CHECKISCALLABLE, that throws if the
     top of the stack value is not callable,
     internal implementation is just like JSOP_CHECKISOBJ and ThrowCheckIsObject.
     JIT support is in next patch

  7. Also applied the same stack balance fix for BytecodeEmitter::emitComprehensionForOf
     to match to the depth of ForOfLoop

  8. removed JSTRY_ITERCLOSE

  9. moved CompletionKind to jsapi.h
Attachment #8838913 - Flags: review?(shu)
Comment on attachment 8836286 [details] [diff] [review]
Part 12: Implement for-await-of.

clearing for now.
this will become smaller since most part is now in Part 0.1
Attachment #8836286 - Flags: review?(shu)
Attachment #8838913 - Attachment description: 01-Bug_1331092___Part_0_1__Use_try_catch_fo.patch → Part 0.1: Use try-catch for IteratorClose in for-of.
The following ParseNode were BINARY and pn_right was Generator NAME or assignment (.generator = PNK_GENERATOR).
  * PNK_YIELD
  * PNK_YIELD_STAR
  * PNK_AWAIT

now Generator NAME can be emitted without having the ParseNode,
so changed them (except assignment case) to UNARY with the operand.

also added PNK_INITIALYIELD, that is UNARY and holds assignment,
separated from PNK_YIELD with JSOP_INITIALYIELD.
Attachment #8838916 - Flags: review?(shu)
also skip the test that checks async generator syntax throws SyntaxError.
Attachment #8838917 - Flags: review?(shu)
addressed review comments.

(I guess I should've posted in original order... :P
Attachment #8836282 - Attachment is obsolete: true
Attachment #8838918 - Flags: review+
Attachment #8836284 - Attachment is obsolete: true
Attachment #8838919 - Flags: review+
Attached patch Part 12: Implement for-await-of. (obsolete) — Splinter Review
Attachment #8836286 - Attachment is obsolete: true
Comment on attachment 8838913 [details] [diff] [review]
Part 0.1: Use try-catch for IteratorClose in for-of.

Review of attachment 8838913 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! 

One question: the current way this is emitted doesn't yet try to cut down on the # of catch blocks generated, like what we talked about on IRC, with more trynote tricks or with catch blocks jumping to a common catch block that has the IteratorClose code. Will that be done later? In the for-of bytecode there happens to be just one catch block anyways, so we're fine here. But this deduplication will be necessary for replacing JSTRY_DESTRUCTURING_ITERCLOSE with a similar approach, else the destructuring bytecode will get even more bloated.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2053,5 @@
> +        MOZ_ASSERT(slotFromTop == unsigned(bce->stackDepth - iterDepth_));
> +        if (!bce->emitDupAt(slotFromTop))         // ITER ... EXCEPTION ITER
> +            return false;
> +        CompletionKind completionKind = CompletionKind::Throw;
> +        if (!emitIteratorClose(bce, completionKind)) // ITER ... EXCEPTION

Just inline CompletionKind::Throw, since the stack comment already doesn't line up.

@@ +2074,5 @@
> +                           CompletionKind completionKind = CompletionKind::Normal) {
> +        return bce->emitIteratorClose(allowSelfHosted_, completionKind);
> +    }
> +
> +    bool emitPrepareForNonLocalJump(BytecodeEmitter* bce, bool isTarget = false) {

No need to make isTarget have a default argument, passing in true/false explicitly with a /* isTarget = */ comment is clearer.

@@ +2720,2 @@
>  
>      // IteratorClose is handled specially in the exception unwinder. For

Stale comment, we can delete this sentence now.

@@ +5239,5 @@
>          return false;
>  
> +    if (completionKind == CompletionKind::Throw) {
> +        // For throw case, we should check if the RET is callable outside of
> +        // the try-catch below.

Great catch.

Could you copy-paste your explanation from the bug comment into this comment here? Namely, that with a normal CompletionKind, where the error is thrown for the return method being non-callable is non-observable.
Attachment #8838913 - Flags: review?(shu) → review+
Comment on attachment 8838916 [details] [diff] [review]
Part 7.5: Add BytecodeEmitter::emitDotGenerator and make yield/await nodes unary.

Review of attachment 8838916 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8274,5 @@
>      return true;
>  }
>  
>  bool
> +BytecodeEmitter::emitDotGenerator()

Nit: rename this to emitGetDotGenerator.
Attachment #8838916 - Flags: review?(shu) → review+
Comment on attachment 8838917 [details] [diff] [review]
Part 8.1: Disable test262 test to check the non-existence of async generators.

Review of attachment 8838917 [details] [diff] [review]:
-----------------------------------------------------------------

Just a heads up, this will conflict with bug 1340583, where :anba changes how negative tests are handled (from using the -n suffix in the filename to error: in the reftest line, similar to jit-tests).
Attachment #8838917 - Flags: review?(shu) → review+
Rebased.

Now for-await-of uses ForOfLoopControl.
Attachment #8838920 - Attachment is obsolete: true
Attachment #8839840 - Flags: review?(shu)
Comment on attachment 8839840 [details] [diff] [review]
Part 12: Implement for-await-of.

Review of attachment 8839840 [details] [diff] [review]:
-----------------------------------------------------------------

So much cleaner with a unified approach to IteratorClose. :)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2020,3 @@
>    public:
> +    ForOfLoopControl(BytecodeEmitter* bce, int32_t iterDepth, bool allowSelfHosted,
> +                     IteratorKind iterKind)

Nit: put iterKind before allowSelfHosted in the constructor and in the fields above.

@@ +2077,5 @@
>      }
>  
>      bool emitIteratorClose(BytecodeEmitter* bce,
>                             CompletionKind completionKind = CompletionKind::Normal) {
> +        return bce->emitIteratorClose(allowSelfHosted_, iterKind_, completionKind);

Nit: should've noticed this earlier; please put allowSelfHosted as the last argument to any function that needs it.

@@ +6978,5 @@
> +    unsigned iflags = forOfLoop->pn_iflags;
> +    IteratorKind iterKind = (iflags & JSITER_FORAWAITOF)
> +                            ? IteratorKind::Async
> +                            : IteratorKind::Sync;
> +    MOZ_ASSERT_IF(iterKind == IteratorKind::Async, sc->asFunctionBox());

Assuming it's a typo for sc->isFunctionBox(), no need for assert, since asFunctionBox() below will assert it.
Attachment #8839840 - Flags: review?(shu) → review+
JSOP_CHECKISCALLABLE is added by Part 0.1.

that does the following at once (the error message is customizable), just like JSOP_CHECKISOBJ:

  if (!IsCallable(obj))
    ThrowTypeError(JSMSG_RETURN_NOT_CALLABLE)

this opcode is used only in IteratorClose with abrupt completion (like when for-of loop body throws), so it won't be so much hot.

those opcodes just calls CheckIsCallable VMFunction, nothing is inlined.
we could optimize this more by reusing MIsCallable, but I'd like to go with simpler way as a first step.
Attachment #8838914 - Attachment is obsolete: true
Attachment #8840143 - Flags: review?(jdemooij)
JSOP_TOASYNCGEN does almost same thing as JSOP_TOASYNC, but calls js::WrapAsyncGenerator instead of js::WrapAsyncFunction.
Attachment #8836283 - Attachment is obsolete: true
Attachment #8840146 - Flags: review?(jdemooij)
JSOP_TOASYNCITER is added in Part 10, that does CreateAsyncFromSyncIterator(syncIterator) in step 3.b.iii there
https://tc39.github.io/proposal-async-iteration/#sec-getiterator
>  iii. Return ? CreateAsyncFromSyncIterator(syncIterator).

JSOP_TOASYNCITER just calls js::CreateAsyncFromSyncIterator, like other opcodes above.
Attachment #8836285 - Attachment is obsolete: true
Attachment #8840152 - Flags: review?(jdemooij)
Blocks: 1342049
arai, could you land just the ITERCLOSE to try-catch patches?
I'll land them in bug 1342553.
Depends on: 1342553
Comment on attachment 8840143 [details] [diff] [review]
Part 0.2: Support JSOP_CHECKISCALLABLE in JIT.

Review of attachment 8840143 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing since I'd like these parts landed before the rest of the async iter patches.

We already have a boolean returning (instead of throwing) version called {M,L}IsCallable. Might not make sense to refactor the MIR/LIR because of the extra checkKind. We need to at least refactor the codegen, as LIsCallable has inlined the fast paths of the checks.

::: js/src/jit/VMFunctions.cpp
@@ +1474,5 @@
>  
> +bool
> +CheckIsCallable(JSContext* cx, HandleValue v, CheckIsCallableKind kind)
> +{
> +    if (!v.isObject() || !v.toObject().isCallable())

Please use !IsCallable(v)
Attachment #8840143 - Flags: review?(jdemooij)
Comment on attachment 8838913 [details] [diff] [review]
Part 0.1: Use try-catch for IteratorClose in for-of.

moved to bug 1342553
Attachment #8838913 - Attachment is obsolete: true
Comment on attachment 8840143 [details] [diff] [review]
Part 0.2: Support JSOP_CHECKISCALLABLE in JIT.

moved to bug 1342553
Attachment #8840143 - Attachment is obsolete: true
Comment on attachment 8840146 [details] [diff] [review]
Part 9: Support JSOP_TOASYNCGEN in JIT.

Review of attachment 8840146 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.
Attachment #8840146 - Flags: review?(jdemooij) → review+
Attachment #8840152 - Flags: review?(jdemooij) → review+
To test non-release-or-beta-only test, I need to skip it on release-or-beta.
added release_or_beta boolean variable to reftest condition.
Attachment #8841806 - Flags: review?(jmaher)
Enabled async-generator tests on non-release-or-beta,
except expression-yield-as-statement.js that our local copy is obsolete.
Attachment #8841808 - Flags: review?(andrebargull)
also, supported new opcodes in decompiler (for stack dump)
we could improve "await" decompilation after changing await to not-using result object.
Attachment #8841810 - Flags: review?(shu)
Comment on attachment 8841808 [details] [diff] [review]
Part 8.3: Enable test262 tests for async generators.

Review of attachment 8841808 [details] [diff] [review]:
-----------------------------------------------------------------

If you need to exclude a complete test directory, you should also be able to use "skip-if(release_or_beta) include test262/language/expressions/async-generators/jstests.list".
Attachment #8841808 - Flags: review?(andrebargull) → review+
Attachment #8841810 - Flags: review?(shu) → review+
Depends on: 1343481
Comment on attachment 8832998 [details] [diff] [review]
Part 1: Remove {JSFunction,JSScript,LazyScript}.isGenerator() method.

I'll land Part 1-6 and 7.5 in bug 1316098.
Attachment #8832998 - Attachment is obsolete: true
Attachment #8832999 - Attachment is obsolete: true
Attachment #8833001 - Attachment is obsolete: true
Attachment #8833003 - Attachment is obsolete: true
Attachment #8833004 - Attachment is obsolete: true
Attachment #8833005 - Attachment is obsolete: true
Attachment #8838916 - Attachment is obsolete: true
(In reply to Tooru Fujisawa [:arai] from comment #66)
> Comment on attachment 8832998 [details] [diff] [review]
> Part 1: Remove {JSFunction,JSScript,LazyScript}.isGenerator() method.
> 
> I'll land Part 1-6 and 7.5 in bug 1316098.

wrong bug number :P

it's bug 1343481
We could optimize away result object allocation for yield/return in async generator, just like await case (bug 1316098),
since the result object is not exposed to script.
I'll file another bug for it after landing this.
depends on the bug in the proposal (the first one there)
https://github.com/tc39/proposal-async-iteration/issues/86

I implemented that part to immediately throw.
looks like fixed by https://github.com/tc39/proposal-async-iteration/pull/87.
and now "done" property is accessed only once.
the patch and testcase in test262 needs fix
another non-critical issue around property access order.
https://github.com/tc39/proposal-async-iteration/issues/88
one more issue https://github.com/tc39/proposal-async-iteration/issues/89
(affects only spec steps, no effect for implementation)
Removed access for "done" property in AsyncGeneratorResume.
step 10.a there is totally unnecessary now.
Attachment #8844829 - Flags: review?(shu)
actually, that fix is not sufficient.
we should perform |if received.[[Type]] is throw| path once more for abrupt case.
filed https://github.com/tc39/proposal-async-iteration/issues/90
my patch doesn't match to current proposal, but I think it's the proposal's bug, not mine ;)
Comment on attachment 8844829 [details] [diff] [review]
Part 11 (previous Part 14) : Fix property access in yield* to follow the change in proposal.

Review of attachment 8844829 [details] [diff] [review]:
-----------------------------------------------------------------

Could just fold this one into the previous patches.
Attachment #8844829 - Flags: review?(shu) → review+
arai, let's land what we have when you're back. I see your issue 90 has been resolved. :)

I'm afraid of this bitrotting. Leo from test262 also said that having this in Nightly would help test262 test async iter to help find possible spec bugs.

We can draft something to dev-platform that says what we have an initial implementation for testing purposes, but please do not use yet due to unsettled spec semantic issues.
Flags: needinfo?(arai.unmht)
okay, hopefully I could land them this weekend, after renumbering the spec steps in the comments
maybe we should make this nightly only then?
Keywords: leave-open
now testing again and I hit the assertion failure on cgc build, but it's not reproducible locally.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bddc92512e86e0edf0d71a3a93eccb0a784b3a4

may take some more time.
Depends on: 1350762
while waiting for bug 1350762, here's the change from https://github.com/tc39/proposal-async-iteration/pull/92
now yield* awaits on the last value (done==true)
Attachment #8851524 - Flags: review?(shu)
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbde2a580e0
Part 1: Add Symbol.asyncIterator. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/0705d5b86ad7
Part 2: Implement Async Generator except yield*. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6e6211270b
Part 3: Disable test262 test to check the non-existence of async generators. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/61e0eb18ba8c
Part 4: Add release_or_beta variable to reftest condition. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/8edcc256976d
Part 5: Enable test262 tests for async generators. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/72750b6e9e2a
Part 6: Support JSOP_TOASYNCGEN in JIT. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d39acbc0922
Part 7: Implement Async Generator yield*. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/831d289bb6cc
Part 8: Support JSOP_TOASYNCITER in JIT. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/3701b28662ac
Part 9: Implement for-await-of. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0c51ce33e8
Part 10: Support JSOP_TOASYNCGEN, JSOP_TOASYNCITER, and JSOP_AWAIT in decompiler. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbde2a580e0d846247703ce3903500e30b83236
Bug 1331092 - Part 1: Add Symbol.asyncIterator. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/0705d5b86ad726d12ebfb73b2ac79dffddbee6a4
Bug 1331092 - Part 2: Implement Async Generator except yield*. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6e6211270ba84591b867a7ff554be14ecbc729
Bug 1331092 - Part 3: Disable test262 test to check the non-existence of async generators. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/61e0eb18ba8cf1d9278a6420aad9317c1a69f06d
Bug 1331092 - Part 4: Add release_or_beta variable to reftest condition. r=jmaher

https://hg.mozilla.org/integration/mozilla-inbound/rev/8edcc256976d217f744ebd5a7072b21a7261e042
Bug 1331092 - Part 5: Enable test262 tests for async generators. r=anba

https://hg.mozilla.org/integration/mozilla-inbound/rev/72750b6e9e2a69f417257326a8af6f2b611592fd
Bug 1331092 - Part 6: Support JSOP_TOASYNCGEN in JIT. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d39acbc09227c0e8187c9acbc1abde04201b09e
Bug 1331092 - Part 7: Implement Async Generator yield*. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/831d289bb6ccac19db04eca0bfba9ccef0699d19
Bug 1331092 - Part 8: Support JSOP_TOASYNCITER in JIT. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/3701b28662ac43a7d2d2bc347eddaee6a3d65a11
Bug 1331092 - Part 9: Implement for-await-of. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0c51ce33e8a5bfd25e06cca7d70164893b5113
Bug 1331092 - Part 10: Support JSOP_TOASYNCGEN, JSOP_TOASYNCITER, and JSOP_AWAIT in decompiler. r=shu
Backed out bug 1331092 and bug 1350762 for breaking js plain shell tests on Windows:

bug 1350762:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e4bb0a73edb848bb93af8ca63a3b6c7d85cc42

bug 1331092:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32f2480cd5aeed2a5d44fe918418d66b9f897b60
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ac3610d777849e9d5cd8c71295bc17253f6dfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c3c0f2b7183d31cfe13f4edb2ef8f1eb0cbad6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ba8b772e65a74441edf8d88a67c0873aa76aa3
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0484f2722f37109453196112b0b1bfe5560e11c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d00d1e5a0f6a8ad28921c562e0fd52b9c4555e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e4c5ed3ed5fea355d42a6767d4eb686ce171a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/649797f1174f3e03b3b04feb54cf0cb2a660047b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1977f337bc2db8db43c00d42d22b0636dc0eb66f
https://hg.mozilla.org/integration/mozilla-inbound/rev/97e7ac639161dc6dc30ae89646c34d156e2ae36e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9b0c51ce33e8a5bfd25e06cca7d70164893b5113&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86668970&repo=mozilla-inbound

c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/vs2015u3/VC/bin/amd64_x86/cl.exe -FoUnified_cpp_js_src4.obj -c   -DNDEBUG=1 -DTRIMMED=1 -D_CRT_RAND_S -DENABLE_BINARYDATA -DENABLE_SIMD -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/js/src -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/js/src  -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/dist/include  -Ic:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/dist/include/nspr         -MD -FI c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/js/src/js-confdefs.h -DMOZILLA_CLIENT   -arch:SSE2 -TP -nologo -utf-8 -wd4800 -wd4595 -D_CRT_SECURE_NO_WARNINGS -wd5026 -wd5027 -Zc:sizedDealloc- -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -Gw -wd4244 -wd4267 -wd4251 -we4553 -GR-  -Zi -O2 -Oy- -WX -wd4805 -wd4661 -we4067 -we4258 -we4275 -wd4146 -wd4577 -wd4312 -Fdgenerated.pdb -FS  c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/obj-spider/js/src/Unified_cpp_js_src4.cpp
Unified_cpp_js_src37.cpp
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(352): error C2589: '(': illegal token on right side of '::'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(342): note: while compiling class template member function 'bool mozilla::BufferList<js::SystemAllocPolicy>::WriteBytes(const char *,std::size_t)'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\js/StructuredClone.h(193): note: see reference to class template instantiation 'mozilla::BufferList<js::SystemAllocPolicy>' being compiled
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(352): error C2059: syntax error: '::'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(362): error C2589: '(': illegal token on right side of '::'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(362): error C2059: syntax error: '::'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(384): error C2589: '(': illegal token on right side of '::'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(380): note: while compiling class template member function 'bool mozilla::BufferList<js::SystemAllocPolicy>::ReadBytes(mozilla::BufferList<js::SystemAllocPolicy>::IterImpl &,char *,std::size_t) const'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(384): error C2059: syntax error: '::'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(459): error C2589: '(': illegal token on right side of '::'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(451): note: while compiling class template member function 'mozilla::BufferList<js::SystemAllocPolicy> mozilla::BufferList<js::SystemAllocPolicy>::Extract(mozilla::BufferList<js::SystemAllocPolicy>::IterImpl &,std::size_t,bool *)'
c:\builds\moz2_slave\m-in_w32_sm-plain-000000000000\src\obj-spider\dist\include\mozilla/BufferList.h(459): error C2059: syntax error: '::'
c:/builds/moz2_slave/m-in_w32_sm-plain-000000000000/src/config/rules.mk:1011: recipe for target 'Unified_cpp_js_src37.obj' failed
mozmake[3]: *** [Unified_cpp_js_src37.obj] Error 2
mozmake[3]: *** Waiting for unfinished jobs....
Added ParserBase::asyncIterationSupported that returns false for chrome/addon and release/beta.
and replaced RELEASE_OR_BETA with it.
Attachment #8851852 - Flags: review?(shu)
workaround for the windows plain build bustage, that is caused by min/max macros in windows.h,
that can be suppressed by NOMINMAX.
Attachment #8851853 - Flags: review?(shu)
Attachment #8851524 - Flags: review?(shu) → review+
Comment on attachment 8851852 [details] [diff] [review]
Part ??: Disable Async Iteration in chrome or add-ons code.

Review of attachment 8851852 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!
Attachment #8851852 - Flags: review?(shu) → review+
Attachment #8851853 - Flags: review?(shu) → review+
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68d774d6bb73
Part 0: Define NOMINMAX to avoid compile error from min/max macro on windows. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1c1a6c478cb
Part 1: Add Symbol.asyncIterator. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/28bce039877f
Part 2: Implement Async Generator except yield*. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/dee4db84bae0
Part 3: Disable test262 test to check the non-existence of async generators. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bfcb40a68b8
Part 4: Add release_or_beta variable to reftest condition. r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ddc9bbcfe35
Part 5: Enable test262 tests for async generators. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/a51f54fd72ff
Part 6: Support JSOP_TOASYNCGEN in JIT. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a506fa42cdc
Part 7: Implement Async Generator yield*. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f68da99a98c
Part 8: Support JSOP_TOASYNCITER in JIT. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b11639e4085
Part 9: Implement for-await-of. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/6564ccfdc704
Part 10: Support JSOP_TOASYNCGEN, JSOP_TOASYNCITER, and JSOP_AWAIT in decompiler. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8464c6a70c0
Part 11: Await on the innerResult.value when innerResult.done is true in yield*. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e15fb883887
Part 12: Disable Async Iteration in chrome or add-ons code. r=shu
Flags: needinfo?(arai.unmht)
Per IRC discussion with arai, let's move follow-up work into a new bug for simpler tracking.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Summary: Implement Async Iteration → Implement Async Iteration for web content on Nightly builds
Blocks: 1352312
Depends on: 1352627
Depends on: 1366399
Depends on: 1390082
Depends on: 1391807
Depends on: 1391781
Depends on: 1454285
Depends on: 1462288
You need to log in before you can comment on or make changes to this bug.