Skip to content

gh-133371: Don't optimize LOAD_FAST instructions whose local is killed by DELETE_FAST #133383

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

Merged
merged 4 commits into from
May 5, 2025

Conversation

mpage
Copy link
Contributor

@mpage mpage commented May 4, 2025

In certain cases it's possible for locals loaded by LOAD_FAST instructions to be on the stack when the local is killed by DELETE_FAST. These LOAD_FAST instructions should not be optimized into LOAD_FAST_BORROW as the strong reference in the frame is killed while there is still a reference on the stack.

…_FAST`

In certain cases it's possible for locals loaded by `LOAD_FAST` instructions
to be on the stack when the local is killed by `DEL_FAST`. These `LOAD_FAST`
instructions should not be optimized into `LOAD_FAST_BORROW` as the strong
reference in the frame is killed while there is still a reference on the stack.
@mpage mpage added the skip news label May 4, 2025
@mpage mpage changed the title gh-133371: Don't optimize LOAD_FAST instructions whose local is killed by DEL_FAST gh-133371: Don't optimize LOAD_FAST instructions whose local is killed by DELETE_FAST May 4, 2025
@mpage mpage requested review from colesbury and Yhg1s May 4, 2025 17:00
@mpage mpage marked this pull request as ready for review May 4, 2025 17:00
Comment on lines +2798 to +2802
case DELETE_FAST: {
kill_local(instr_flags, &refs, oparg);
break;
}

Copy link
Member

Choose a reason for hiding this comment

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

FWIW: This change by itself is not enough to make the crash in test_asyncio go away.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Doesn't seem to fix the test_asyncio crash.

@bedevere-app
Copy link

bedevere-app bot commented May 4, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@mpage
Copy link
Contributor Author

mpage commented May 5, 2025

@gvanrossum - This fixes both of the repros in the linked issue. Even if this doesn't fix the original issue, it's worth merging by itself.

@mpage
Copy link
Contributor Author

mpage commented May 5, 2025

@gvanrossum - I forgot to bump the magic number. I suspect that you have some cached bytecode. I've bumped the magic number, double checked that this fixes both of the repros on the linked issue, and applied this on top of the affected PR and verified that it fixes the crash in test_asyncio.test_eager_task_factory.

@mpage
Copy link
Contributor Author

mpage commented May 5, 2025

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented May 5, 2025

Thanks for making the requested changes!

@Yhg1s, @gvanrossum, @Fidget-Spinner: please review the changes made to this pull request.

@mpage
Copy link
Contributor Author

mpage commented May 5, 2025

I think the hypothesis failure is unrelated to this PR. It looks like test_external_inspection.TestGetStackTrace.test_async_remote_stack_trace might be flaky (e.g. it fails in the same way on this unrelated job).

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I can no longer repro the crash (probably because I am now using a different machine) but your hypothesis that I kept seeing it because of the magic number makes sense. (A little confused still -- does optimized bytecode now end up in .pyc files?)

@mpage
Copy link
Contributor Author

mpage commented May 5, 2025

A little confused still -- does optimized bytecode now end up in .pyc files?

Yes. This optimization happens statically during bytecode compilation. Maybe you are thinking of the specializing interpreter, which optimizes the compiled bytecode at runtime?

@gvanrossum
Copy link
Member

Go ahead and merge then!

@mpage mpage merged commit 78adb63 into python:main May 5, 2025
41 checks passed
@mpage mpage deleted the gh-133371-handle-del-fast branch May 5, 2025 04:00
@gvanrossum
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants