gh-144540: Add _MAKE_HEAP_SAFE uop to eliminate unnecessary refcount operations in RETURN_VALUE and YIELD_VALUE #144414
Conversation
cocolato
requested review from
Fidget-Spinner,
markshannon,
savannahostrowski and
tomasr8
as code owners
markshannon
changed the title
_RETURN_VALUE_HEAP_SAFE to avoid refcount operation_RETURN_VALUE_HEAP_SAFE to avoid refcount operation
|
@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. |
|
The removal of the "heap safe" check can be done for any return and for yields as well. |
cocolato
changed the title
_RETURN_VALUE_HEAP_SAFE to avoid refcount operation
cocolato
changed the title
cocolato
changed the title
cocolato
changed the title
Sorry about that, and thanks for letting me know! I've updated the code. |
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
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.
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) || |
There was a problem hiding this comment.
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.
|
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 |
In this case, because If this does not meet our expectations, I can add more tests later. |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
2 participants
Split
RETURN_VALUEandYIELD_VALUEinto macro form with a new_MAKE_HEAP_SAFEmicro-op, so the tier2 optimizer can eliminate unnecessaryPyStackRef_MakeHeapSafe()calls when the value is already heap-safe (owned or immortal).