Implement ES2018 Async Iteration (Experimental)

[rhuanjl]

This PR implements ES2018 Async Iteration (Async Generator Functions and for await):
UPDATE: This is now a complete implementation except:

1. It needs more tests (help appreciated here)
2. I haven't looked at profiler support
3. I haven't looked at TTD support
4. This is placed behind a flag, ES2018AsyncIteration set to default to disabled (hoping it can be enabled soon but feels too big to enable straight away)

Implementation notes:

1. AsyncGenerators are implemented by extending the existing Generator type
2. Handling await, yield and yield* in async generators has involved adding 4 new op codes - wanted it only to be 3 but couldn't find a way to handle the interaction between .return() and yield* without an extra op
3. AsyncFromSyncIterator (required per spec for async iteration of a synchronous object) has been added as a whole new type

Test262 results

1. I've run this against the test 262 tests in the following folders, I used the ES6FunctionNameFull and ES2018ObjectRestSpread flags when running the tests as some of these tests require those features:
   https://github.com/tc39/test262/tree/master/test/built-ins/AsyncFromSyncIteratorPrototype
   https://github.com/tc39/test262/tree/master/test/built-ins/AsyncGeneratorFunction
   https://github.com/tc39/test262/tree/master/test/built-ins/AsyncIteratorPrototype/Symbol.asyncIterator
   https://github.com/tc39/test262/tree/master/test/language/statements/async-generator
   https://github.com/tc39/test262/tree/master/test/language/statements/for-await-of

2. Most tests pass, fails relate to existing issues:
   a. Next isn't cached - issue exists with synch iterators BUG: GetIterator should cache next method #5860
   b. the async keyword is not allowed to contain unicode escapes (affects normal async functions as well as async generators)
   c. of in a loop header is not allowed to contain a unicode escape
   d. 12 other issues to do with destructuring syntax and loop headers

ref: #2720

[pleath]

Thanks, @rhuanjl . You mention that you're adding runtime checks. Do those checks determine which context await/yield are being employed in, and/or to distinguish between await and yield?

[rhuanjl]

The runtime checks this contains which could clearly be removed by refactoring (likely with more code in total) are:

1. await vs yield vs yield*
2. AsyncGenerator vs Generator (when calling the next/throw/return methods have to check type twice as first check gives same answer for both as it's the same class)

The other painful point performance wise with await and yield is that currently the bytecode is set to create an object and attach the awaited or yielded value as the .value property of the object. In an async generator even with yield you have to await the value so in both cases i’m discarding the created object.

[pleath]

I see. Yes, you're right, we want to be able to resolve static properties of the code without using runtime checks. A separate opcode for yield in different contexts is a good place to start and something we anticipated having to do to implement the feature. I'll be glad to help you do that. We can handle the JIT after getting things working in the interpreter. You'll need to:

• add your new opcode to runtime/bytecode/opcodes.h
• make the byte code generator emit your opcode in the proper context (runtime/bytecode/bytecodeemitter.cpp)
• add a handler for your opcode to the interpreter, which will likely just route to a runtime helper.

[rhuanjl]

Whilst this version should work as is certainly agree that it could do with improvements, I'll aim to submit an update with additional opcodes by the end of this week or start of next.

[rhuanjl]

Changes made:
I've made the following changes from the original version:

1. fixed some errors I'd made around incorrect handling of returns
2. Introduced OpCodes for Await, AsyncYield and AsyncYield*
3. Implemented the OpCodes as runtime function calls
4. added the OpCodes as helper calls in the Jit (I think I have this right...)
5. Removed most of the temporary objects my initial version needed (including pre-creating and holding onto continuation functions for await and yield as with most async generators these will likely be needed multiple times)
6. Significantly reduced the number of runtime checks/branches required
7. Updated my handling of await for a normative PR that's currently open to the spec (it already has consensus so should in theory be being merged soon)

OtherNotes

1. Hopefully the assertion I'm hitting is an easy fix though I'd appreciate help with that - I've had a look but can't figure it out. EDIT: assertion fixed
2. If/when this is merged I'd like to follow up with an edit to the Async function implementation to use much of this code - will be able to reduce a lot of duplication as well as streamlining async functions
3. Pending points from my first comment remain the same
4. The original yield opcode is still used for breaking execution of the async generator - but in async generators that's now all it does - it has to set a return value but this is a throwaway - I assumed that removing the unneeded return value was more effort than it was worth.

[rhuanjl]

Small update - I'm about half way there offline with the remaining pieces needed to meet the spec i.e. for-await-of and async from sync iterator. It's going to be 500+ more lines of code when done so may be easier if this part is reviewed first before I add the rest?

[pleath]

Thanks, @rhuanjl . Are you still hitting the assert? If so, can you give me some detail about it?

[rhuanjl]

@pleath I've fixed the assert issue, thanks.

Question - Should I leave this PR as is or update it with my further work - I've implemented for-await-of and AsyncFromSyncIterator (the remaining parts of the feature) offline but they're another 600 lines and I haven't written full tests for them yet. (worried about the sheer size of the diff)

[fatcerberus]

@rhuanjl I mentioned this in private already but it seems that this feature currently can be enabled only with a flags-enabled build of ch (using the command-line option). JsRuntimeAttributeEnableExperimentalFeatures doesn’t enable it so there’s no way to use the feature at all in a Release build.

[rhuanjl]

UPDATE: see edited first post for details - this is now a complete implementation of ES2018 Async Iteration (though could do with more tests)

Please could people find some time to begin reviewing this?

[pleath]

Awesome. Thanks, @rhuanjl . I'll give this a close review this week.

[zenparsing]

Thanks @rhuanjl and @pleath - it will be really awesome to get this in!

[pleath]

FYI, I'm about half way through the review, and so far everything looks nice and clean. I want to walk through a couple of use cases before I sign off.

[fatcerberus]

This still is not usable with JsRuntimeAttributeEnableExperimentalFeatures, which is the only way to get access to experimental stuff in the release build - can that be fixed?

[rhuanjl]

@pleath thanks for the prelim feedback - I've done a re-read/check and noticed a couple of things, not going to update the PR whilst you're mid review, notes for ref:

1. I realised that the JIT handlers I included in lower.cpp are untested as by default jilting generators is disabled (controlled by flag JitES6Generators) BUT - it's disabled as it doesn't work - jitting sync generators causes a crash - async ones crash the same way - so don't know if my code is ok and it's just the existing problem that's an issue or if my code is wrong.

2. Some of my spec text comments have the wrong numbering (oops, I'll fix this in a separate commit after you've reviewed)

3. to @fatcerberus point on JsRuntimeAttributeEnableExperimentalFeatures it seems this enables flags that use the macro FLAGPR_REGOVR_EXP() and not FLAGPR() - not sure which this should be

4. Not sure what to do for better test coverage - the basic cases I've included clearly aren't enough - test262 has ~2000 tests for this; it might take a long time to create full coverage.

5. Also realised I hadn't mentioned above - I took a cue from Make Async-from-Sync iterator object inaccessible to ECMAScript code tc39/ecma262#1172 and followed v8/jsc&sm in not allowing a script to obtain a reference to an AsyncFromSyncIterator object - whilst technically per spec it should be possible by overwriting AsyncIteratorPrototype[Symbol.asyncIterator].

[rhuanjl]

@pleath Any update on this - as fixing the hit piece is out of scope per discussion in #5877 only other points I'm aware of are that it probably needs more tests and a couple of my comments are wrong - didn't want to add anything when you were mid-review though

(Also really expected your review to find some errors in my code this is the largest piece of c++ I've ever written)

[pleath]

Sorry for the holdup, @rhuanjl. Many fire drills the last couple of weeks. I’ll wrap this up today. Feel free to correct your comments; it won’t interfere.

[fatcerberus]

Uh oh, looks like Candle Jack got hi

[pleath]

(Hah. My previous comment was mangled, but I’ve fixed it.)

[pleath]

Nice job, @rhuanjl . I have just a couple of comments and questions. I would go ahead and make the changes you mentioned that you were holding on to, and we can probably go ahead with getting this in.

[rhuanjl]

Thanks for the review, I've:

1. Squashed my previous commits
2. Rebased to latest master and regen'd byte code (to fix merge conflict)
3. Updated the comment error + points from @pleath's review as the new commit "fixes"

Note this PR still adds this as a disabled feature - partly as no one has told me to enable it since I opened the PR and partly as I'm worried my test coverage is insufficient for a shipping feature this big.

[fatcerberus]

Keep in mind that if the feature is fully disabled, including with the EnableExperimentalFeatures runtime attribute, then nobody can test it unless they have a flags-enabled build of CC. That's not likely to be the case for embedders shipping an application, so in practice the feature will get no real-world testing.

[rhuanjl]

@fatcerberus I'm hoping this can be enabled soon but leaving that decision to the CC team - as an additional note node-chrakracore uses EnableExperimentalFeatures so if I switched it to an experimental feature of that kind it would be on in node-chakracore - it should really have TTD support before then I assume; and I don't know how to do TTD support.

[fatcerberus]

I'm hoping this can be enabled soon but leaving that decision to the CC team

Yes, I was aware - I was simply bringing the above up as a discussion point.

[rhuanjl]

Would it be possible to move forward with this or is it parked indefinitely?

Is there anything specific I can do to move this towards being mergeable?

[zenparsing]

Hi @rhuanjl - Thanks for pinging on this! I intend to pick this up within the next few days. In the meantime, would you mind doing a rebase?

[rhuanjl]

@zenparsing I saw it got out of date, and had bytecode conflicts so I've just regenerated the bytecode and rebased again. I also squashed the fixes commit for neatness.

Please let me know if you'd like me to do anything else on this.

[bathos]

Suggested change

	name : "Async Generaotr function instances have the correct prototype object",
	name : "Async Generator function instances have the correct prototype object",

[rhuanjl]

I've fixed this typo in the follow up PR #6172 thanks.

[rhuanjl]

Thanks for merging this @zenparsing

As noted above whilst this is a full implementation it's disabled by default - please let me know if there's anything I can contribute to help get towards it being enabled by default.