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

Runtime improvements for generators #6251

Closed
wants to merge 5 commits into from

Conversation

nhat-nguyen
Copy link
Contributor

  • Cache generator type
  • Remove redundant prototype creation
  • Remove extra dummy yield at the beginning of generator functions (used to evaluate default arguments, disabled by default, currently breaks async functions)

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.
@rhuanjl
Copy link
Collaborator

rhuanjl commented Aug 17, 2019

As the consistent test failure relates to AsyncGenerators I took a quick look.

Unfortunately I found 2 separate bugs neither of which I currently have a fix for.

1. Error causing the current crash:

async function* agf () {
    await (function() {})();
}

agf().next();

Run with: -ES2018AsyncIteration -mic:1 -off:simplejit

This results in:
a) agf() call leads to InitialiseAsyncGenerator and the DummyYield - agf has been called (interpreted) once
b) next() call leads to Jit attempt on agf
c) Upon execution hits BailOnNoProfile
d) Drops into interpreter and reaches OP_Await BUT ResumeYieldData is a nullptr (assumedly lost in the Jit and bail out process)

I would be surprised if this result cannot be reproduced without the Dummy Yield as I assume there are other ways to hit a BailOnNoProfile before an OP_Await (Or OP_AwaitYield which has the same problem)

2. Other error

async function* agf () {
    await 5;
}

agf().next();

Run with: -ES2018AsyncIteration -RemoveDummyYield

This appears to be an error introduced by removing the Dummy Yield and is not related to Jitting the code.

  1. agf() call creates generator object and calls InitialiseAsyncGenerator BUT does not lead to execution as DummyYield removed
  2. next() call leads to execution BUT on reaching OP_Await the ResumeYieldData is a pointer to JS undefined instead of the ResumeYieldData
  3. Promise reactions are given invalid handlers as undefined doesn't have them

I'd like to look into this further particularly point 2 where I can see what's happening:
ResumeYieldData is set to undefined when the DummyYield is disabled and OP_Await (and OP_AwaitYield) can't work with that - out of time for now but will see if I can pick this up later.

}
else
{
this->Writer()->Reg1(Js::OpCode::LdUndef, funcInfo->yieldRegister);
Copy link
Collaborator

@rhuanjl rhuanjl Aug 17, 2019

Choose a reason for hiding this comment

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

In AsyncGenerators this results in writing undefined to what should be the location of the ResumeYieldData - unfortunately removing it just leaves it as a nullptr which isn't right either - in any event I don't think this instruction is useful.

Within the EmitDummyYield undefined is loaded into the yieldRegister then yielded (as you need to yield something) - the Resume that follows results in ResumeYieldData then being set here.

@nhat-nguyen
Copy link
Contributor Author

nhat-nguyen commented Aug 17, 2019

@rhuanjl Yes you are right. This improvement is a work-in-progress, and unfortunately I didn't have time to fix this before my last day. Essentially the problem is that async generator assumes that the ResumeYieldData has already been loaded to the yield register. Removing the first yield now makes that assumption invalid because we only set ResumeYieldData when we call EntryNext on the generator.

Following is what I have tried to address this problem, but I wasn't sure if it was the best way.

So there is code that handles setting this ResumeYieldData if the interpreter frame already exists (!= nullptr) (a grep for GetYieldRegister should give you where this code is), what we simply need is to do this for both cases. This is because before removing yield, we already have the interpreter frame for every "real" entry into the generator.

The above fix would expose us to another problem in the jit where the symbol defined by the yield register doesn't exist before uses by OP_Await (or some other opcode, I don't quite remember). So what we need to do next is, in this else block, emit some opcode that looks like it's writing to the yield register. Currently I have this LoadResumeYieldDataForGenerator that can be modified a little so that it can be a no-op in interpreter mode.

So with the 2 fixes, I think we are back to having the first case that you describe where the ResumeYieldData isn't restored during bailout. I didn't have time to investigate this further though.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Aug 29, 2019

I think I've solved the BailOut issue, took ages to track it down but very simple in the end - posted in the other PR as relevant there.

I've hit all sorts of weird errors I don't understand trying to solve the RemoveDummyYield, I will continue digging a little with it.

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.

2 participants