1352312 - Update Async Iteration to follow the proposal change and let it ride the train

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tooru Fujisawa [:arai]]

followup for bug 1331092.

Async Iteration is now enabled only on web content in non-release.
There's some issues in proposal that will change the semantics.
  https://github.com/tc39/proposal-async-iteration/issues/93
  https://github.com/tc39/proposal-async-iteration/issues/88

we'll need to update our implementation after the issue gets closed,
and wen the proposal gets stable (maybe when it gets stage 4?), we can enable it on chrome/addons and let it ride the train.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: WIP : Await on the value before yielding inside async generator.

Almost implements https://github.com/tc39/proposal-async-iteration/pull/102

there's one difference between the PR that,
generator.[[AsyncGeneratorState]] is set to suspendedYield *after* awaiting on the value.
(I don't think it has some effect tho...)

Comment 2

[Tooru Fujisawa [:arai]]

https://github.com/tc39/proposal-async-iteration/pull/102/files
now we need to do await for return, with AwaitIfReturn, inside AsyncGeneratorResumeNext and AsyncGeneratorYield, they're now implemented in C++.

in order to perform await there, we should do either of the following:
  A. split it and all callers into 2 parts, and connect them with Promise or direct-call
  B. move most part of those functions into bytecode (not sure where to put it tho)

for AsyncGeneratorYield, we could just move it to bytecode tho,
AsyncGeneratorResumeNext is a bit hard.

Comment 3

[Tooru Fujisawa [:arai]]

AwaitIfReturn is removed and modified version could be implemented mostly in straightforward way,
the only problem is the await after AsyncGenerator#return.

>      1. Set the code evaluation state of _genContext_ such that when evaluation is resumed with a Completion _resumptionValue_ the following steps will be performed:
> +      1. If _resumptionValue_.[[Type]] is not ~return~, return Completion(_resumptionValue_).
> +      1. Let _awaited_ be Await(_resumptionValue_.[[Value]]).
> +      1. If _awaited_.[[Type]] is ~throw~, return Completion(_awaited_).
> +      1. Assert: _awaited_.[[Type]] is ~normal~.
> +      1. Return Completion{[[Type]]: ~return~, [[Value]]: _awaited_.[[Value]], [[Target]]: ~empty~}.
> +      1. NOTE: When one of the above steps returns, it returns to the evaluation of the |YieldExpression| production that originally called this abstract operation.

I'm wondering where to perform that steps.
maybe we need dedicated function than StarGeneratorReturn for this case,
or perhaps perform it in AsyncGeneratorResume.

Comment 4

[Tooru Fujisawa [:arai]]

simplest solution would be, emitting those steps as bytecode and jump to there when resuming from yield with return.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: WIP : Await on the value before yielding or returning inside async generator.

spec-wise changes:
 * added await to yield
 * removed await in yield* with done==true case
 * added await to syntactic return
 * added await to AsyncGenerator#return (before initial yield, yield, after closed)

impl-wise changes:
 * added InternalAwait that encapsulates NewPromiseCapability+resolve+NewPromiseCapability+PerformPromiseThen
 * updated spec sections and steps
 * generalized promise handler jobs for AsyncFunction and AsyncGenerator, from await to everything

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #4)
> simplest solution would be, emitting those steps as bytecode and jump to
> there when resuming from yield with return.

in WIP patch, await is performed in C++ side, with InternalAwait.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: WIP : Await on the value before yielding or returning inside async generator.

updated #include and comments.

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8879755 [details] [diff] [review]
WIP : Await on the value before yielding or returning inside async generator.

Since this bug is for letting AsyncIteration ride the train, moving single particular change into other bug.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Can this now get enabled in chrome and ride trains?

According to https://github.com/tc39/proposal-async-iteration/issues/109 Domenic is waiting for two implementations to ship it and if I understand https://chromium-review.googlesource.com/c/523844 V8 already does.

Comment 10

[Tooru Fujisawa [:arai]]

IMO, it can, but not sure about the policy here.
at least enabling in chrome code on nightly only should be fine.

also, even if it gets enabled on chrome code, we'd better wait for a few cycles before using it in chrome code.
(same as async function, that we started using it only in testcase at first, and then in chrome code after maybe 2 cycles)

till, can I have your opinion about when to let it ride the train?

Comment 11

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa (almost away 08/11-08-16) [:arai] from comment #10)
> till, can I have your opinion about when to let it ride the train?

So the current situation is that our implementation matches the latest spec, and the spec is at stage 3, right? If both of those hold, we should let this ride the train, for both chrome and content.

We should probably still hold off for a bit on using this in chrome code, lest we get into annoying situations because we have to disable it on beta or something.

Comment 12

[Tooru Fujisawa [:arai]]

Thanks :)

I'll work on this maybe this weekend (if my laptop become IP reachable)
if anyone want, feel free to take over.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Enable Async Iteration.

removed asyncIterationSupported condition, and also enabled tests for async-generator on all branches, and removed a testcase that checks it's disabled on chrome.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bdd8be8275768b7db5c3c294e6b2c056b9fb92d

Comment 14

[Till Schneidereit [:till]]

Comment on attachment 8897370 [details] [diff] [review]
Enable Async Iteration.

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

\o/, excellent!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff910d235e4
Enable Async Iteration. r=till

Comment 16

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff910d235e4b7e3908a2d18b09ac21de7cb5480
Bug 1352312 - Enable Async Iteration. r=till

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ff910d235e4

Comment 18

[Domenic Denicola]

I don't see this in https://developer.mozilla.org/en-US/Firefox/Releases/57; was that an omission, or did it end up not making it into 57 after all?

Comment 19

[Tooru Fujisawa [:arai]]

(In reply to Domenic Denicola from comment #18)
> I don't see this in https://developer.mozilla.org/en-US/Firefox/Releases/57;
> was that an omission, or did it end up not making it into 57 after all?

I just forgot to update the documentation.
I'll update shortly.
Thanks!

Comment 20

[Tooru Fujisawa [:arai]]

for now just listed them.
documentation for each syntax and prototype isn't yet ready.
  https://developer.mozilla.org/en-US/Firefox/Releases/57

Comment 21

[rektide]

Hello. It would be wonderful to have an entry on platform-status for this. This is how websites such as caniuse.com traditionally link to a browser's support for platform capabilities.