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
iritkatriel merged 6 commits into from
Oct 25, 2023
Merged

gh-100762: Fix optimization in gen_close #111069

iritkatriel merged 6 commits into from
Oct 25, 2023

Conversation

@iritkatriel
Copy link
Member

@iritkatrieliritkatriel 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-appbedevere-appbot mentioned this pull request Oct 19, 2023
Closed
@iritkatrieliritkatriel 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
@iritkatrieliritkatriel changed the title gh-100762: fix refleak in gen_closegh-100762: assert that yf is NULL in gen_close in the cases where it is not DECREFedOct 19, 2023
@iritkatrieliritkatriel changed the title gh-100762: assert that yf is NULL in gen_close in the cases where it is not DECREFedgh-100762: Fix optimization in gen_close and assert that yf is NULL in the cases where it is not DECREFedOct 19, 2023
iritkatriel
iritkatriel commented Oct 19, 2023
View reviewed changes
@iritkatrieliritkatriel mentioned this pull request Oct 19, 2023
Merged
@iritkatrieliritkatriel mentioned this pull request Oct 20, 2023
Closed
@iritkatrieliritkatriel 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.

@iritkatrieliritkatriel marked this pull request as ready for review October 24, 2023 11:11
@iritkatrieliritkatriel added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Oct 24, 2023
@iritkatrieliritkatriel changed the title gh-100762: Fix optimization in gen_close and assert that yf is NULL in the cases where it is not DECREFedgh-100762: Fix optimization in gen_close Oct 24, 2023
@iritkatrieliritkatriel 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
@iritkatrieliritkatriel merged commit 0db2517 into python:mainOct 25, 2023
@markshannonmarkshannon 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

@cdce8pcdce8p mentioned this pull request Feb 22, 2024
Closed
@iritkatriel
Copy link
MemberAuthor

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.

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

miss-islington-appbot 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-appbot commented Feb 22, 2024

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

@bedevere-appbedevere-appbot 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.
@iritkatrieliritkatriel 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
@gaogaotiantiangaogaotiantian mentioned this pull request Jun 12, 2025
Closed
@markshannonmarkshannon mentioned this pull request Oct 21, 2025
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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