Refactor await and yield implementation and enable async iteration #6312

zenparsing · 2019-10-24T19:45:15Z

Overview

The goals of this change are to:

Enable async generators
Streamline and simplify the relationship between generators, async functions, and async generators.
Make async functions and generators more JIT-friendly by reducing the number and complexity of supporting helpers.
Improve code readability and maintainability.

The primary architectural change is to perform yield resume logic and branching directly in bytecode, rather than relying on the ResumeYield helper, and to desugar all async functions and async generators to normal generators. This allows us to eliminate the special-case GeneratorReturnException exception type.

In order to perform yield resume logic in bytecode, we resume the generator by creating a dynamic object with "value" (an arbitrary var) and "kind" (tagged int) fields. Branch instructions are inserted to deal with the various "kind" cases: "normal", "return", and "throw".

To desugar async functions and async generators to normal generators, we introduce a new type ID to represent "await results". Async generators then test for this type ID when determining whether their internal coroutine is "awaiting" or "yielding".

Changes

Architectural

Remove ResumeYieldData. Instead, resume the generator with a "resume yield object" containing "value" and "kind" properties.
Remove GeneratorReturnException. The bytecode is now responsible for returning based on the resume yield kind.
Remove opcodes ResumeYield, Await, AsyncYield, AsyncYieldStar, AsyncYieldIsReturn.
Add new opcode and helper for NewAwaitObject. This object type is yielded when awaiting and is uniquely distinguished by type ID from "normal" yield results.
Remove ResumeYield and ResumeYieldStar opcodes and helpers. When generators are resumed, the resume yield object is loaded into the yield register and the resume logic is performed in bytecode.
Make async generators a subclass of JavascriptGenerator.
Enable flag ES2018AsyncIteration by default.

JIT

Remove unused GeneratorResumeYieldLabel.
Rename GeneratorLoadResumeYieldData to GeneratorResumeYield.
Remove support for ResumeYield. GeneratorResumeYield is inserted immediately after the bail-in instr and takes care of loading the resume yield object.

ByteCode

Rename EmitDummyYield to EmitStartupYield.
Remove unnecessary wrapping of async function body in try/catch. Exception handling is performed by library code that wraps the coroutine.
Move startup yield after function declaration hoisting to unify module and generator code paths.
Implement iterator next method caching to match spec.
Replace tryScopeRecordsList and loop "parent node linking" with a single mechanism for performing cleanup in both the branch-in-loop cases and yield cases.
Implement yield star evaluation logic in bytecode.
Implement yield resume branching in bytecode.
Await return operand in async generators to match spec.
Add new error message for when yield * inner iterator does not have a "throw" method.

Parser

Remove ParseNodeLoop. The only purpose of this node type was to store an outer statement reference used when emitting jump cleanup. This information is now maintained in the bytecode generator's jumpCleanupList.

Library

Move JavascriptAsyncGeneratorFunction into a new file.
Add type ID for async generators.
Remove unnecessary JavascriptPromiseAsyncSpawnExecutorFunction.
Use more readable names for CreateIteratorResultObject functions.
Use more readable names for async function support classes.
Move JavascriptAsyncFunction into a new file (previously code was split between JavascriptPromise and JavascriptGeneratorFunction).
Move JavascriptAsyncGeneratorFunction into a new file.
Create locked library types for await objects and resume yield objects.
Move "copy arguments to heap" logic into JavascriptGenerator::New.

Performance

This change results in a small performance increase on the ARES-6 benchmark.

Scenario	Average Time	Change over baseline
ARES-6/Basic (No JIT)	3671ms	3.96% faster
ARES-6/Basic (JIT)	2502ms	2.56% faster

rhuanjl · 2019-10-28T15:51:56Z

It's nice to see the horrible OP_ResumeYield logic replaced.

A little sad to see that much of my implementation of AsyncGenerators is removed/re-written here BUT I can see that this way of doing it is a lot neater.

NOTE: I have not reviewed this, I've just had a general read of the code.

A couple of things stand out to me by way of future improvements:

The startup yield is only needed when the generator has complex parameters - there was code to put a relevant condition around it in the PR to enable jitting generators - obviously that PR as it stands is dead with this change but that enhancement seems worth bringing across to me.
In my asyncGenerator implementation I cached the continuation methods for the async generator rather than creating them every time obviously doing that has some memory cost but I figure it should be worth it for performance and the same could/should be done for async functions.
Similarly the await object should only need to be created once and could be cached in an internal slot
The caching of the iterator Next property is missing in some other places not just generators

zenparsing · 2019-10-28T18:24:56Z

Hi @rhuanjl - I was a bit nervous about posting this PR given there were so many changes to what you had done, which was awesome work given the existing generators implementation. But particularly with JITing coming I want to take this opportunity to clean up the existing oddness. Apologies!

In my asyncGenerator implementation I cached the continuation methods for the async generator rather than creating them every time obviously doing that has some memory cost but I figure it should be worth it for performance and the same could/should be done for async functions.

Oh right, I had forgotten about that, and it was a good idea. Hopefully this change will make it a bit easier to apply the same optimization to async functions (since there are fewer continuation functions to worry about).

Similarly the await object should only need to be created once and could be cached in an internal slot

Yes! I was trying to figure out whether it would be better to store it in a C++ field or create a register for it in bytecode, but I didn't try to implement it either way yet.

The caching of the iterator Next property is missing in some other places not just generators

Right. My idea was to go ahead and take care of the bytecode part here, and then use your existing PR for the remaining changes to the library code. I think I got all of the bytecode here?

rhuanjl · 2019-10-29T17:59:52Z

From talking about it thought I'd have a stab at it, commits available for cherry-picking if wanted though I really just did this for fun so won't mind if they're not used:

Cache the await object (done in a register in bytecode to minimise helper calls): rhuanjl@27fb68c
Cache the AsyncGenerator continuation methods:
rhuanjl@9ecc2f6
Re-work AsyncFunction logic (to simplify it) and cache the continuation methods This one currently breaks async function TTD support, I've tried unsuccessfully to fix it, hopefully something obvious/easy that I'm missing - I've never touched the TTD code before
rhuanjl@67f7691

On the Iterator Next point it does indeed look like you hit all of the Bytecode changes for it; so yeah just a case of taking my existing PR and dropping the bytecode changes and fixing the conflicts on JavascriptOperators.cpp and should fully fix the issue.

zenparsing · 2019-10-29T19:53:20Z

Those changes look great! We can bring them in after this one gets landed.

For the TTD stuff, I don't really know it either but I think @boingoing could help when we get there. I'm also trying to consider what we want to do with TTD longer-term.

nhat-nguyen · 2019-10-29T22:09:16Z

Awesome!! 😊😊

fatcerberus · 2019-11-04T06:52:05Z

Remove unnecessary wrapping of async function body in try/catch. Exception handling is performed by library code that wraps the coroutine.

Ooh! Will that fix #4630, I wonder...? 🤞

rhuanjl · 2019-11-04T09:06:27Z

Remove unnecessary wrapping of async function body in try/catch. Exception handling is performed by library code that wraps the coroutine.

Ooh! Will that fix #4630, I wonder...? 🤞

It doesn't - BUT it lays the groundwork for an easy fix of #4630 that I have ready to submit after this lands (if someone doesn't beat me to it).

boingoing

It looks good to me. I like the cleanups in ptree.h, bytecode generator, and splitting of async code from JavascriptPromise. Couple questions but overall I think it seems right.

boingoing · 2019-11-19T00:44:38Z

lib/Parser/Parse.cpp

@@ -7322,7 +7322,6 @@ void Parser::ParseExpressionLambdaBody(ParseNodeFnc * pnodeLambda, bool fAllowIn
        // Pushing a statement node with PushStmt<>() normally does this initialization
        // but do it here manually since we know there is no outer statement node.
        pnodeRet->grfnop = 0;
-        pnodeRet->pnodeOuter = nullptr;


Is this a drive-by cleanup of pnodeOuter because it isn't used?

It was only used for the EmitJumpCleanup routine in the byte code generator. After joining the "jump cleanup" paths with "generator yield cleanup" paths, we don't need this state in the parse tree anymore.

boingoing · 2019-11-19T00:46:23Z

lib/Parser/ptree.h

@@ -863,21 +859,8 @@ class ParseNodeJump : public ParseNodeStmt
    DISABLE_SELF_CAST(ParseNodeJump);
 };

-// base for loop nodes
-class ParseNodeLoop : public ParseNodeStmt


Wow. Hard to believe this hasn't been cleaned-up before.

boingoing · 2019-11-19T00:47:52Z

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

+        // (that is when the function is called). This yield opcode is to mark the  begining of the function body.
+        // TODO: Inserting a yield should have almost no impact on perf as it is a direct return from the function. But this needs
+        // to be verified. Ideally if the function has simple parameter list then we can avoid inserting the opcode and the additional call.
+        if (pnodeFnc->IsGenerator())


boingoing · 2019-11-19T00:49:23Z

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

-void EmitIteratorComplete(Js::RegSlot doneLocation, Js::RegSlot iteratorResultLocation, ByteCodeGenerator* byteCodeGenerator, FuncInfo* funcInfo);
-void EmitIteratorValue(Js::RegSlot valueLocation, Js::RegSlot iteratorResultLocation, ByteCodeGenerator* byteCodeGenerator, FuncInfo* funcInfo);
+
+void EmitFunctionCall(


Are these functions just moved up here?

There was quite a bit of code movement here in order to better support calling iterator methods from bytecode. For example, we needed to cache the "next" method of iterators and use that method on subsequent calls in for-of and yield star. To that end, EmitIteratorNext and EmitIteratorValue were removed in favor of the more general EmitFunctionCall and EmitGetObjectProperty.

boingoing · 2019-11-19T00:50:30Z

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

-    {
-        byteCodeGenerator->tryScopeRecordsList.UnlinkFromEnd();
-    }
+    byteCodeGenerator->PopJumpCleanup();


I like the cleanups related to this list - good stuff.

boingoing · 2019-11-19T00:58:40Z

lib/Runtime/ByteCode/ByteCodeGenerator.cpp

+{
+    // On resuming from a yield, we branch based on the ResumeYieldKind
+    // integer value
+    byteCodeGenerator->EnregisterConstant((uint)Js::ResumeYieldKind::Normal);


I'm a little confused how this constant is used. We're sticking it in the constant table. Is this pulled-out when we're doing the resume? Is there other generator state here or is it usually on the stack frame?

Thanks for asking about this.

These constants are used to determine how the generator was resumed. If the generator was resumed with a "normal completion", then the resume object will have a kind property whose value is (uint)Js::ResumeYieldKind::Normal. (Similarly for "throw" and "return".) Bytecode is generated that checks the "kind" property against these constant values.

Do you think this is a good approach?

So the point of registering these constants is just to pass the values so we can compare them to some other value in a register or such when executing the bytecode?

boingoing · 2019-11-19T01:37:51Z

lib/Runtime/Library/JavascriptGenerator.cpp

-        // 4. Let queue be generator.[[AsyncGeneratorQueue]].
-        // 5. Let request be AsyncGeneratorRequest { [[Completion]]: completion, [[Capability]]: promiseCapability }.
-        AsyncGeneratorRequest* request = RecyclerNew(scriptContext->GetRecycler(), AsyncGeneratorRequest, input, exceptionObj, promise);
+    // TODO: BUGBUG - figure out how to determine what the prototype was


Is this ok? Is TTD broken with this?

@boingoing I believe TDD has always been broken on this path. I remember seeing this same comment but didn't have enough context to understand it further.

Yes. All of this, including the TTD stuff (which I've got my eye on for possible simplification) was just copied over.

pleath · 2019-12-03T23:23:24Z

Note that the *_ReuseLoc variants should not be used the first time a code sequence writes to a register, only the 2nd and subsequent times. So:
R3 = LdFld R4.x
R3 = LdFld_ReuseLoc R5.x
Not:
R3 = LdFld_ReuseLoc R4.x
R3 = LdFld_ReuseLoc R5.x

The reason is that _ReuseLoc tells the JIT not to start a new lifetime. So in the above example, each _ReuseLoc op extends an existing lifetime of R3.

zenparsing · 2019-12-03T23:41:45Z

Thanks, @pleath I was wondering about that. I'll update with a fix.

rhuanjl · 2020-04-08T20:28:37Z

@zenparsing do you think you'll be able to update this so we can land it? Or should I pull it down, update for the comments above and open a new PR?

zenparsing · 2020-04-10T20:22:24Z

@rhuanjl This PR was already updated per @pleath's comments above. I'll try to rebase, regenerate bytecode, and give it another review within a couple of days.

rhuanjl · 2020-05-14T08:26:08Z

@zenparsing Please could you rebase this and regen the bytecode? I'll give it another review - I think I'm familiar enough now with most of this logic.

(I've got some follow up PRs I want to open)

Refactor await and yield implementation and enable async iteration See #6312 Co-authored-by: Kevin Smith <Kevin.Smith@microsoft.com>

rhuanjl · 2020-06-04T22:31:15Z

Merged as c848d4d

zenparsing changed the title ~~[WIP] Refactor await and yield implementation~~ Refactor await and yield implementation and enable async iteration Oct 25, 2019

rhuanjl mentioned this pull request Oct 29, 2019

Fix and Enable Jitting Generators #6264

Closed

fatcerberus mentioned this pull request Nov 4, 2019

Assertion failure in InlineCache.cpp #6323

Open

boingoing approved these changes Nov 19, 2019

View reviewed changes

zenparsing force-pushed the async-rework branch from c94da69 to b186ded Compare November 23, 2019 03:33

This was referenced Nov 24, 2019

Implement Top-Level Await (Stage 3 proposal) #6262

Closed

Review Implementation of Async/Await #5293

Open

zenparsing force-pushed the async-rework branch from b186ded to 1fb2a00 Compare December 4, 2019 01:00

Refactor await and yield implementation and enable async iteration

c65dd32

zenparsing force-pushed the async-rework branch from 1fb2a00 to c65dd32 Compare December 5, 2019 18:51

rhuanjl mentioned this pull request Mar 26, 2020

ChakraCore Transition #6384

Open

rhuanjl mentioned this pull request Apr 13, 2020

Refactor module code to keep dependency specifiers in source code order #6296

Open

rhuanjl mentioned this pull request Jun 4, 2020

Refactor await & yield & enable async iteration #6456

Merged

rhuanjl added a commit that referenced this pull request Jun 4, 2020

Refactor await & yield & enable async iteration (#6456)

c848d4d

Refactor await and yield implementation and enable async iteration See #6312 Co-authored-by: Kevin Smith <Kevin.Smith@microsoft.com>

rhuanjl closed this Jun 4, 2020

rhuanjl mentioned this pull request Jun 4, 2020

Cache iterator next method fix:#5860 #6164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor await and yield implementation and enable async iteration #6312

Refactor await and yield implementation and enable async iteration #6312

zenparsing commented Oct 24, 2019 •

edited

Loading

rhuanjl commented Oct 28, 2019 •

edited

Loading

zenparsing commented Oct 28, 2019

rhuanjl commented Oct 29, 2019 •

edited

Loading

zenparsing commented Oct 29, 2019

nhat-nguyen commented Oct 29, 2019

fatcerberus commented Nov 4, 2019

rhuanjl commented Nov 4, 2019

boingoing left a comment

boingoing Nov 19, 2019

zenparsing Nov 23, 2019

boingoing Nov 19, 2019

boingoing Nov 19, 2019

boingoing Nov 19, 2019

zenparsing Nov 23, 2019

boingoing Nov 19, 2019

boingoing Nov 19, 2019

zenparsing Nov 23, 2019 •

edited

Loading

boingoing Jan 3, 2020

boingoing Nov 19, 2019

nhat-nguyen Nov 19, 2019

zenparsing Nov 23, 2019

pleath commented Dec 3, 2019

zenparsing commented Dec 3, 2019

rhuanjl commented Apr 8, 2020

zenparsing commented Apr 10, 2020

rhuanjl commented May 14, 2020

rhuanjl commented Jun 4, 2020

Refactor await and yield implementation and enable async iteration #6312

Refactor await and yield implementation and enable async iteration #6312

Conversation

zenparsing commented Oct 24, 2019 • edited Loading

Overview

Changes

Architectural

JIT

ByteCode

Parser

Library

Performance

rhuanjl commented Oct 28, 2019 • edited Loading

zenparsing commented Oct 28, 2019

rhuanjl commented Oct 29, 2019 • edited Loading

zenparsing commented Oct 29, 2019

nhat-nguyen commented Oct 29, 2019

fatcerberus commented Nov 4, 2019

rhuanjl commented Nov 4, 2019

boingoing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenparsing Nov 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleath commented Dec 3, 2019

zenparsing commented Dec 3, 2019

rhuanjl commented Apr 8, 2020

zenparsing commented Apr 10, 2020

rhuanjl commented May 14, 2020

rhuanjl commented Jun 4, 2020

zenparsing commented Oct 24, 2019 •

edited

Loading

rhuanjl commented Oct 28, 2019 •

edited

Loading

rhuanjl commented Oct 29, 2019 •

edited

Loading

zenparsing Nov 23, 2019 •

edited

Loading