Skip to content

gh-100762: Fix optimization in gen_close #111069

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.

Sign up for GitHub

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
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
+5 −8

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Oct 19, 2023 •
edited
Loading

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 only security fixes needs backport to 3.12 only security fixes and removed 3.12 only security fixes type-bug An unexpected behavior, bug, or error needs backport to 3.12 only 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
@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 only security fixes needs backport to 3.12 only 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 only security fixes and removed needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Oct 24, 2023
@iritkatriel iritkatriel merged commit 0db2517 into python:main Oct 25, 2023
@markshannon markshannon mentioned this pull request Jan 5, 2024
Closed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@iritkatriel @aisk
pythongh-100762: Fix optimization in gen_close (python#111069)
3baff26
@cdce8p
Copy link
Contributor

cdce8p commented Feb 22, 2024

Would it be possible / make sense to cherry-pick this change onto 3.12?

For pylint / astroid we're seeing ValueError: generator already executing errors since 3.12.0b1. We haven't been able to create a standalone reproducer - which is also the reason I haven't opened a bug report yet. However testing either 3.13.0a2 or 3.12 with this change cherry-picked on top does seem to resolve it as best as I can tell.

pylint-dev/pylint#9138

@cdce8p cdce8p mentioned this pull request Feb 22, 2024
Open
@iritkatriel
Copy link
Member Author

Would it be possible / make sense to cherry-pick this change onto 3.12?

I think so. I'll make a PR.

@iritkatriel iritkatriel added the needs backport to 3.12 only security fixes label Feb 22, 2024
@miss-islington-app
Copy link

Thanks @iritkatriel for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 22, 2024
@iritkatriel @miss-islington
pythongh-100762: Fix optimization in gen_close (pythonGH-111069)
fdbb567 
(cherry picked from commit 0db2517)

Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Feb 22, 2024

GH-115818 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Feb 22, 2024
iritkatriel added a commit that referenced this pull request Feb 22, 2024
@miss-islington @iritkatriel
[3.12] gh-100762: Fix optimization in gen_close (GH-111069) (#115818)
eb4774d 
gh-100762: Fix optimization in gen_close  (GH-111069)
(cherry picked from commit 0db2517)

Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
iritkatriel added a commit to iritkatriel/cpython that referenced this pull request Jun 13, 2024
@iritkatriel
Revert "[3.12] pythongh-100762: Fix optimization in gen_close (python…
47c7a30 
…GH-111069) (python#115818)"

This reverts commit eb4774d.
@iritkatriel iritkatriel mentioned this pull request Jun 13, 2024
Merged
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@iritkatriel @Glyphack
pythongh-100762: Fix optimization in gen_close (python#111069)
bba1850
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.
3 participants
@iritkatriel @markshannon @cdce8p