-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix and Enable Jitting Generators #6264
Conversation
EDIT: additional commit I've stumbled to another bug with this not covered by current tests and which I cannot currently fix - the following code snippet will hit a failfast in BuildBailInSymbolList. Disabling GlobOpt avoids this but obviously that is not desirable. Notably if the FailFast is removed the code runs to completion safely - though I assume that the FailFast is there for a reason. function* foo()
{
const temp2 = undefined;
while(true)
{
yield temp2;
}
}
const gen = foo();
for (let i = 0; i < 2000; ++i)
{
gen.next();
} |
47d6868
to
7c9da9f
Compare
I tried updating to latest and found that #6232 appears to have broken this - I will investigate and endeavour to fix but struggling to work out the cause of the breakage at the moment. |
Thanks, @rhuanjl - I'll taking a look at it as well next week. |
7c9da9f
to
17401d5
Compare
So this looks safe to land, updated to latest and disabled AsyncGenerators, though could perhaps do with some further stress testing/fuzzing of some kind. It enables jitting Generators and Async functions in x64 builds only. I have two TODOs to go beyond this that I'd like help/advice with or someone else to pick up:
function* gf()
{
while (true)
{
yield Math.fround(Math.PI);
}
}
const gen = gf();
gen.next();
gen.next();
gen.next();
gen.next();
|
Note: Test failure is "Agent lost communication with the server" and not an actual test failure. Assume there will be some further changes from forthcoming review so tests can be re-run then. |
@rhuanjl Right, there's an issue with our Win7 test runner at the moment. |
17401d5
to
fa695d7
Compare
Also don't unnecessarily `generateWriteBarrier` when nulling out interpreter frame (fixes Encoder issue on Mac/Linux)
Generator functions with non-simple parameter list (e.g: arguments with default values that have some complex expressions) will need to evaluate those expressions when the generator object is created. This result in us emitting a first dummy yield to evaluate those. For jitted generator functions, this would mean every function would have a bailout at the beginning which is fairly expensive for small functions. So to mitigate this, only emit dummy yields when we really need to do so.
fa695d7
to
1d0f6c5
Compare
I've dropped the As above this is still only enabling jitting for generators and async functions NOT async generators. AND it's still x64 only. |
I'd like to take a look at the x86 issue before we merge this. |
I spent a bit of time trying to dig into the x86 issue but couldn't work out what the cause for it was the crash occurs in an x86 build only when at runtime the engine attempts to yield the return value of Math.fround that has been lowered with GenerateFastInlineMathFround - but that was as far as I could get. |
Closing as much of this is incompatible with #6312 |
@rhuanjl I just tried to repro the x86 issue using your POC on master and did not get a failure. Does it still repro for you? |
Just tested and it doesn't repro for me on master either. It must have been brought in by something else on this branch, possibly the OP_ResumeYield fast path or the removal of the dummy/startup yield. |
It could also have been due to that bug we recently fixed. Thanks! |
This PR incorporates the work done by @nhat-nguyen in #6245 and #6251 as well as a couple of fixes so it should work correctly and pass all tests:
@zenparsing please could you take a look at this?