Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jit Generators and asyncs + minor enhancements #6465

Closed

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented Jun 10, 2020

Enable the JIT for Generators and Async functions (including Async Generators).

This builds on work done by @zenparsing and @nhat-nguyen to finally enable jitting functions containing yield or await.

I've also included a couple of minor improvements that I'd written a while ago and been waiting to merge:

  1. cache the await object - this was being allocated and freed for every await it's now allocated at the top of an async function and freed at the end.
  2. cache async generator continuation methods - these were being allocated and freed for every yield or await inside an async generator, now allocated at the start of the function and freed at the end.

@ppenzin not a significant diff by line count but as this enabling a lot of other code that has not been enabled before may need some careful thought.

Fixes: #5877

@rhuanjl rhuanjl force-pushed the generatorAsyncEnhancements branch 3 times, most recently from 90e1e13 to c8a0ead Compare June 10, 2020 18:48
@rhuanjl rhuanjl closed this Jun 10, 2020
@rhuanjl rhuanjl reopened this Jun 10, 2020
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jun 10, 2020

The lone test failure is very strange - it's hitting an assertion using time travel debugging and the NoNative flag, it does not occur if you instead use the -off:backend flag which is functionally equivalent to NoNative so clearly there is something odd happening here.

Also weird that a change to enable jitting things not previously jitted would hit an assertion when using a flag that disables the jit.

@ppenzin
Copy link
Member

ppenzin commented Jun 11, 2020

I am not immediately able to reproduce it from the CI log, re-running the test suite on my machine.

Edit: Reproduced with local test suite; with default test options it is a JIT command (with -nonative it still fails the same way though):

> ch.exe -DumpOnCrash -WERExceptionSupport -ExtendedErrorStackForTestHost -BaselineMode -maxInterpretCount:1 -maxSimpleJitRunCount:1 -bgjit- -dynamicprofilecache:profile.dpl.UnnamedTest3079  -TTReplay=asyncAwait3Test -TTDStartEvent=5 C:\Users\popen\devel\ChakraCore\test\TTBasic\ttdSentinal.js

I am not sure whether JIT or interpreter mode makes a substantial impact, as the test is entirely replay logs, JS file is empty. The error is due to getting an Object instead of AwaitObject - I am wondering if something needs to be regenerated for this test.

@nhat-nguyen
Copy link
Contributor

nhat-nguyen commented Jun 11, 2020

The lone test failure is very strange - it's hitting an assertion using time travel debugging and the NoNative flag, it does not occur if you instead use the -off:backend flag which is functionally equivalent to NoNative so clearly there is something odd happening here.

Also weird that a change to enable jitting things not previously jitted would hit an assertion when using a flag that disables the jit.

I could be wrong, but if my memory serves me correctly, -off:backend doesn't turn off the jit completely (maybe simplejit is still on). So -off:backend isn't what it suggests . -nonative is the actual interpreter only mode.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jun 11, 2020

I had a bit of a dig - as far as I can see the issue is that TTD doesn't have explicit support for an await object. It still restores the right information but doesn't set the type. Currently the only impact of this is hitting that assertion the code runs to completion correctly with the assertion removed.

I'm unsure why playing around with jit/nojit mode hits this but it might be to do with the point in the code that the snapshot is taken for the TTD.

AsyncGenerators - which would be impacted more significantly by this - do not support TTD at all, but I'm not planning on fixing that for now hence I've added a change to stop the assertion firing when in TTDReplay mode.

@nhat-nguyen thanks for the comment - I had a dig through the code and I can't find a path that nonative blocks that -off:backend doesn't also block but may be missing something

@ppenzin one of the earlier cases in the same file generates the log for the replay - running offline I was regenerating the log first. The input JS file is asyncAwait3.js and you run it with -TTRecord=asyncAwait3Test -TTSnapInterval=0 to create the log in your CWD then have to run the test in the same CWD to use the log.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jun 11, 2020

Going back over the code I found a bug that wasn't being tested:
Generator.prototype.next.call(asyncGenerator) should throw a TypeError but was running to completion - in the context of jitted AsyncGenerators that could easily produce undefined behaviour so adding a fix for that.

Also noticed that AsyncGenerator.prototype.next.call(invalid) was throwing the slightly odd error message: AsyncGenerator.prototype.next 'this' is not a %s object absolutely minor but adding a fix for that at the same time - as well as tests of that code path as it was untested.

@@ -231,7 +231,7 @@ Var JavascriptGenerator::EntryNext(RecyclableObject* function, CallInfo callInfo

AUTO_TAG_NATIVE_LIBRARY_ENTRY(function, callInfo, _u("Generator.prototype.next"));

if (!VarIs<JavascriptGenerator>(args[0]))
if (!VarIs<DynamicObject>(args[0]) || JavascriptOperators::GetTypeId(args[0]) != TypeIds_Generator)
Copy link
Collaborator Author

@rhuanjl rhuanjl Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VarIs<JavascriptGenerator> returns true for AsyncGenerators (which are a subclass of Generators) hence had to change these conditions to use an explicit check for Generators instead.

@rhuanjl rhuanjl closed this Jun 11, 2020
@rhuanjl rhuanjl reopened this Jun 11, 2020
@rhuanjl rhuanjl force-pushed the generatorAsyncEnhancements branch 2 times, most recently from 804d79d to a329470 Compare June 11, 2020 15:19
@rhuanjl rhuanjl force-pushed the generatorAsyncEnhancements branch from a329470 to 1e20533 Compare June 12, 2020 06:38
@rhuanjl rhuanjl closed this Jun 12, 2020
@rhuanjl rhuanjl reopened this Jun 12, 2020
@rhuanjl rhuanjl closed this Jun 12, 2020
@rhuanjl rhuanjl reopened this Jun 12, 2020
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jun 12, 2020

So due to the way the switch was setup my original PR wasn't actually enabling the JIT - having fixed that I've uncovered 2 significant bugs - I've fixed one but still trying to track through the second.

Currently if a generator function hits a BailoutOnNoProfile it restores with the wrong variable values - which results in inconsistent observable behaviour, closing this until I can fix this issue.

@rhuanjl rhuanjl closed this Jun 12, 2020
@rhuanjl rhuanjl reopened this Jun 13, 2020
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jun 13, 2020

Well, two bugs down and found another one, the bytecode offsets sometimes go adrift, so when yielding from a generator and dropping back via the interpreter it can try to restore in an invalid location.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Dec 5, 2020

I'll try and have another look at this in the next two weeks - really want to get it working BUT ugh it is not simple...

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Dec 14, 2020

Closing this - I've moved the parts that are ready to land into #6533 going to keep working on the other parts.

@rhuanjl rhuanjl closed this Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generators and Async functions are not jitted
3 participants