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

Rework how captured values are written to GeneratorBailInInstrs #6293

Merged
merged 1 commit into from
Oct 18, 2019

Conversation

zenparsing
Copy link
Contributor

@zenparsing zenparsing commented Sep 24, 2019

This PR fixes an existing bug in the generator JIT implementation found by @rhuanjl.

In order to generate bail-in code, the backward pass copies "captured value" information from a yield's BailOutInfo into the associated GeneratorBailInInstr. The current implementation was copying bailOutInfo->capturedValues->constantValues on every pass over the yield instr. However, in a loop the yield instr is passed over twice, and each time entries are moved from bailOutInfo->capturedValues into bailOutInfo->usedCapturedValues. On the second pass over the instr, the current implementation is overwriting the constant values list with an empty list.

With this change, the constant values are transferred to the bailin instr directly in the code that processes bailout constant values and copy prop syms.

@zenparsing zenparsing force-pushed the bailin-captured-values branch 3 times, most recently from fb8c3b2 to a0cdafa Compare September 24, 2019 19:23
@rhuanjl
Copy link
Collaborator

rhuanjl commented Sep 26, 2019

So there was a legitimate problem, not just an over zealous failfast? Good to see it fixed.

@zenparsing
Copy link
Contributor Author

@MikeHolman would you mind taking a look at this PR for me? This bug is getting in the way of the other generator-related work that I'm doing.

}

Assert(instr->m_next && instr->m_next->m_next);
return instr->m_next->m_next->AsGeneratorBailInInstr();
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is asking for trouble, maybe we want to guard this with AssertOrFailFast(IsGeneratorBailInInstr()), or even just do an early return if it isn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree the m_next->m_next thing is icky. What would be a better way?

Clearly every OpCode::Yield must have a following GeneratorBailIn right? So it's just a matter of where in the forward list it is. If there's isn't one in the forward list (before the next Yield) then we have to fail fast.

So we could do this:

for (auto* next = instr->m_next; next; next = next->m_instr)
{
    if (next->m_opcode === Js::OpCode::Yield) 
        break;
    else if (next->IsGeneratorBailInInstr())
        return next->AsGeneratorBailInInstr();
}
AssertOrFailFastMsg(false, "Yield not followed by GeneratorBailInInstr");

Do you think that would be better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you could alternatively hit a return instead of another yield - so maybe break to the failfast for a return or a yield?

Copy link
Contributor

@MikeHolman MikeHolman left a comment

Choose a reason for hiding this comment

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

:shipit:

chakrabot pushed a commit that referenced this pull request Oct 18, 2019
…eneratorBailInInstrs

Merge pull request #6293 from zenparsing:bailin-captured-values

This PR fixes an existing bug in the generator JIT implementation found by @rhuanjl.

In order to generate bail-in code, the backward pass copies "captured value" information from a yield's `BailOutInfo` into the associated `GeneratorBailInInstr`. The current implementation was copying `bailOutInfo->capturedValues->constantValues` on every pass over the yield instr. However, in a loop the yield instr is passed over twice, and each time entries are moved from `bailOutInfo->capturedValues` into `bailOutInfo->usedCapturedValues`. On the second pass over the instr, the current implementation is overwriting the constant values list with an empty list.

With this change, the constant values are transferred to the bailin instr directly in the code that processes bailout constant values and copy prop syms.
@chakrabot chakrabot merged commit 8a4ff27 into chakra-core:master Oct 18, 2019
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.

4 participants