Closed Bug 1352312 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: arai, Assigned: arai)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 1355399
Depends on: 1352627
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...)
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
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.
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.
simplest solution would be, emitting those steps as bytecode and jump to there when resuming from yield with return.
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
Attachment #8874088 - Attachment is obsolete: true
(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.
updated #include and comments.
Attachment #8879487 - Attachment is obsolete: true
Depends on: 1379525
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.
Attachment #8879755 - Attachment is obsolete: true
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.
Flags: needinfo?(arai.unmht)
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?
Flags: needinfo?(arai.unmht) → needinfo?(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.
Flags: needinfo?(till) → needinfo?(arai.unmht)
Thanks :)

I'll work on this maybe this weekend (if my laptop become IP reachable)
if anyone want, feel free to take over.
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
Flags: needinfo?(arai.unmht)
Attachment #8897370 - Flags: review?(till)
Comment on attachment 8897370 [details] [diff] [review]
Enable Async Iteration.

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

\o/, excellent!
Attachment #8897370 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/4ff910d235e4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1390730
Depends on: 1391807
Depends on: 1391781
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?
(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!
for now just listed them.
documentation for each syntax and prototype isn't yet ready.
  https://developer.mozilla.org/en-US/Firefox/Releases/57
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: