FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-144540: Add _MAKE_HEAP_SAFE uop to eliminate unnecessary refcount operations in RETURN_VALUE and YIELD_VALUE by cocolato · Pull Request #144414 · python/cpython · GitHub

/ cpython Public

gh-144540: Add _MAKE_HEAP_SAFE uop to eliminate unnecessary refcount operations in RETURN_VALUE and YIELD_VALUE - #144414

Merged
markshannon merged 10 commits into
python:mainfrom
cocolato:optimize/add_RETURN_VALUE_HEAP_SAFE
Mar 11, 2026
ConversationCommits (10)ChecksFiles changed
Merged

gh-144540: Add _MAKE_HEAP_SAFE uop to eliminate unnecessary refcount operations in RETURN_VALUE and YIELD_VALUE#144414
markshannon merged 10 commits into
python:mainfrom
cocolato:optimize/add_RETURN_VALUE_HEAP_SAFE

Conversation

cocolato commented Feb 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Split RETURN_VALUE and YIELD_VALUE into macro form with a new _MAKE_HEAP_SAFE micro-op, so the tier2 optimizer can eliminate unnecessary PyStackRef_MakeHeapSafe() calls when the value is already heap-safe (owned or immortal).

bedevere-app Bot mentioned this pull request Feb 3, 2026
Open
markshannon changed the title gh-144388: Add new uop _RETURN_VALUE_HEAP_SAFE to avoid refcount operation gh-144540: Add new uop _RETURN_VALUE_HEAP_SAFE to avoid refcount operation Feb 6, 2026
bedevere-app Bot mentioned this pull request Feb 6, 2026
Open

markshannon commented Feb 6, 2026

Copy link
Copy Markdown
Member

@cocolato in future can you avoid referencing the meta-issue in PRs. Make a new issue if you need to. I've done that for this PR already.

markshannon commented Feb 6, 2026

Copy link
Copy Markdown
Member

The removal of the "heap safe" check can be done for any return and for yields as well.
If we split the instructions into micro-ops it makes the optimizations simpler and more effective.
#144540

cocolato changed the title gh-144540: Add new uop _RETURN_VALUE_HEAP_SAFE to avoid refcount operation gh-144540: Split RETURN_VALUE/YIELD_VALUE into _MAKE_HEAP_SAFE + micro-ops Feb 6, 2026
cocolato changed the title gh-144540: Split RETURN_VALUE/YIELD_VALUE into _MAKE_HEAP_SAFE + micro-ops Add uop to eliminate unnecessary refcount operations in RETURN_VALUE and YIELD_VALUE Feb 6, 2026
cocolato changed the title Add uop to eliminate unnecessary refcount operations in RETURN_VALUE and YIELD_VALUE gh-144540: Add uop to eliminate unnecessary refcount operations in RETURN_VALUE and YIELD_VALUE Feb 6, 2026
cocolato changed the title gh-144540: Add uop to eliminate unnecessary refcount operations in RETURN_VALUE and YIELD_VALUE gh-144540: Add _MAKE_HEAP_SAFE uop to eliminate unnecessary refcount operations in RETURN_VALUE and YIELD_VALUE Feb 6, 2026

cocolato commented Feb 6, 2026

Copy link
Copy Markdown
Member Author

@cocolato in future can you avoid referencing the meta-issue in PRs. Make a new issue if you need to. I've done that for this PR already.

Sorry about that, and thanks for letting me know! I've updated the code.

markshannon requested changes Feb 9, 2026
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Take care with the negation of predicates. The opposite of "knowing x is true", is "not knowing that x is true", not "knowing that x is false".

Can you add a test case that fails for this PR as written.
Something like:

cond = False
def f(a):
    if cond:
        del a
    return a
def g():
    x = 1
    return f(x)
for _ in range(THRESHOLD):
    g()

g will pass a borrowed reference to f which will then incorrectly return a borrowed reference.

Comment thread Python/optimizer_bytecodes.c Outdated

op(_MAKE_HEAP_SAFE, (value -- value)) {
// If the value is not borrowed, or is immortal, it's heap-safe.
if (!PyJitRef_IsBorrowed(value) ||

markshannon Feb 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think this is right. PyJitRef_IsBorrowed(value) means that we know that the reference value is borrowed, but !PyJitRef_IsBorrowed(value) merely means that we don't know that it is borrowed, not that we know it isn't borrowed.

I think we can only apply this to immortal values.

bedevere-app Bot commented Feb 9, 2026

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

cocolato commented Feb 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Can you add a test case that fails for this PR as written. Something like:

cond = False
def f(a):
    if cond:
        del a
    return a
def g():
    x = 1
    return f(x)
for _ in range(THRESHOLD):
    g()

In this case, because a is a small int, so it will be optimized by immortal check, I wrote other simple fail test after we only check immortal return values:

def returns_owned(x):
       return x + 1

If this does not meet our expectations, I can add more tests later.

cocolato commented Feb 10, 2026

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

bedevere-app Bot commented Feb 10, 2026

Copy link
Copy Markdown

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

bedevere-app Bot requested a review from markshannon February 10, 2026 04:28
markshannon approved these changes Mar 10, 2026
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks good now, thanks.
(Sorry for taking so long to get back to this)

cocolato commented Mar 10, 2026

Copy link
Copy Markdown
Member Author

Thanks for reply and review! Updated.

cocolato commented Mar 11, 2026

Copy link
Copy Markdown
Member Author

@markshannon Gentle ping

markshannon merged commit f062014 into python:main Mar 11, 2026
73 checks passed

markshannon commented Mar 13, 2026

Copy link
Copy Markdown
Member

@cocolato @Fidget-Spinner
It looks like this may have broken something.

With PYTHON_JIT_STRESS=1 the tests now fail (for me) in test_dataclasses on main, and bisecting leads to this commit.

I don't see anything wrong with this PR, so it might have exposed a pre-existing bug.

Fidget-Spinner commented Mar 13, 2026

Copy link
Copy Markdown
Member

@markshannon I found the bug and will have a fix up shortly.

Fidget-Spinner mentioned this pull request Mar 13, 2026
Closed
cocolato deleted the optimize/add_RETURN_VALUE_HEAP_SAFE branch March 17, 2026 08:11
cocolato mentioned this pull request Mar 23, 2026
Draft
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
pythongh-144540: Add _MAKE_HEAP_SAFE uop to eliminate unnecessary ref…
f72b48b 
…count operations in RETURN_VALUE and YIELD_VALUE (pythonGH-144414)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

markshannon markshannon approved these changes
tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner
Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner
savannahostrowski Awaiting requested review from savannahostrowski savannahostrowski is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL