Skip to content

gh-100762: Fix optimization in gen_close #111069

Merged
iritkatriel merged 6 commits intopython:mainfrom
iritkatriel:refleak
Oct 25, 2023
Merged

gh-100762: Fix optimization in gen_close #111069
iritkatriel merged 6 commits intopython:mainfrom
iritkatriel:refleak

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
Closed
@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

markshannon commented Oct 24, 2023

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

markshannon commented Oct 24, 2023

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
Closed
@iritkatriel
Copy link
Member Author

iritkatriel commented Feb 22, 2024

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

miss-islington-app bot commented Feb 22, 2024

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
@gaogaotiantian gaogaotiantian mentioned this pull request Jun 12, 2025
Closed
@markshannon markshannon mentioned this pull request Oct 21, 2025
Closed
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