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

Jump to bottom

gh-100762: Fix optimization in gen_close #111069

Merged
merged 6 commits into from Oct 25, 2023
Merged

gh-100762: Fix optimization in gen_close #111069

merged 6 commits into from Oct 25, 2023

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Oct 19, 2023 •
edited

In f02fa64 lines 383-389 were added, with returns that do not decref yf. I think this is ok because yf is always NULL in these cases. Adding the assertions.

Also, the optimization for the case of exception_handler_depth== 1 is not working because op.code is checked instead of op.arg.

@bedevere-app bedevere-app bot mentioned this pull request Oct 19, 2023
Open
@iritkatriel iritkatriel added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.12 bugs and security fixes needs backport to 3.12 bug and security fixes and removed 3.12 bugs and security fixes type-bug An unexpected behavior, bug, or error needs backport to 3.12 bug and security fixes labels Oct 19, 2023
@iritkatriel iritkatriel changed the title gh-100762: fix refleak in gen_close gh-100762: assert that yf is NULL in gen_close in the cases where it is not DECREFed Oct 19, 2023
@iritkatriel iritkatriel changed the title gh-100762: assert that yf is NULL in gen_close in the cases where it is not DECREFed gh-100762: Fix optimization in gen_close and assert that yf is NULL in the cases where it is not DECREFed Oct 19, 2023
iritkatriel
iritkatriel commented Oct 19, 2023
View reviewed changes
Objects/genobject.c Outdated Show resolved Hide resolved
@iritkatriel iritkatriel mentioned this pull request Oct 19, 2023
Merged
@iritkatriel iritkatriel reopened this Oct 20, 2023
@iritkatriel iritkatriel mentioned this pull request Oct 20, 2023
Closed
@iritkatriel iritkatriel marked this pull request as draft October 20, 2023 12:48
@markshannon
Copy link
Member

This looks good.
The failure is in a new test, that didn't exist when the unintentional change from oparg to opcode was made. https://github.com/python/cpython/pull/101912/files#diff-2a0c449b68605ebd0872fd232e60ce7e838a77782a6d2e364764f99514fb508aR376

Just change the test.

@markshannon
Copy link
Member

One minor efficiency improvement that could be made is to not call _PyGen_yf(gen) until after the checks for FRAME_CREATED and FRAME_COMPLETED.

@iritkatriel iritkatriel marked this pull request as ready for review October 24, 2023 11:11
@iritkatriel iritkatriel added needs backport to 3.11 bug and security fixes needs backport to 3.12 bug and security fixes labels Oct 24, 2023
@iritkatriel iritkatriel changed the title gh-100762: Fix optimization in gen_close and assert that yf is NULL in the cases where it is not DECREFed gh-100762: Fix optimization in gen_close Oct 24, 2023
@iritkatriel iritkatriel added needs backport to 3.12 bug and security fixes and removed needs backport to 3.11 bug and security fixes needs backport to 3.12 bug and security fixes labels Oct 24, 2023
@iritkatriel iritkatriel merged commit 0db2517 into python:main Oct 25, 2023
34 checks passed
@markshannon markshannon mentioned this pull request Jan 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon markshannon approved these changes

Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@iritkatriel @markshannon