Skip to content

gh-100762: Fix optimization in gen_close - #111069

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

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

Conversation

@iritkatriel

@iritkatrieliritkatriel commented Oct 19, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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
Comment threadObjects/genobject.c Outdated
@iritkatrieliritkatriel mentioned this pull request Oct 19, 2023
Merged
@iritkatrieliritkatriel mentioned this pull request Oct 20, 2023
Closed
@iritkatriel
iritkatriel marked this pull request as draft October 20, 2023 12:48
@markshannon

markshannon commented Oct 24, 2023

Copy link
Copy Markdown
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

markshannon commented Oct 24, 2023

Copy link
Copy Markdown
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
@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
@iritkatriel
iritkatriel 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

cdce8p commented Feb 22, 2024

Copy link
Copy Markdown
Contributor

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

iritkatriel commented Feb 22, 2024

Copy link
Copy Markdown
MemberAuthor

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

miss-islington-appBot commented Feb 22, 2024

Copy link
Copy Markdown

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

bedevere-appBot commented Feb 22, 2024

Copy link
Copy Markdown

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