gh-143414: Implement unique reference tracking for JIT, optimize unpacking of such tuples#144300
gh-143414: Implement unique reference tracking for JIT, optimize unpacking of such tuples#144300reidenong wants to merge 31 commits intopython:mainfrom
Conversation
|
Discussed with @Fidget-Spinner, will reopen the PR after implementing |
|
Windows Ci failure looks possibly related, can you please look into it? |
Fidget-Spinner
left a comment
There was a problem hiding this comment.
This is excellent. Just 2 comments, then I think it's safe to merge.
Python/optimizer_bytecodes.c
Outdated
|
|
||
| op(_COPY, (bottom, unused[oparg-1] -- bottom, unused[oparg-1], top)) { | ||
| assert(oparg > 0); | ||
| bottom = PyJitRef_IsUnique(bottom) ? PyJitRef_StripReferenceInfo(bottom) : bottom; |
There was a problem hiding this comment.
We only need to strip unique info here, not the borrow info too. This way we still get refcount elimination with the borrow.
There was a problem hiding this comment.
I think this is consistent with the code?
If the ref is unique, we remove the unique tag, otherwise we leave it as is. Since a ref cannot be both unique and borrowed at the same time, borrowed/invalid refs will be maintained.
|
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 |
|
Thanks for the explanation on only tracking references on the stack. Since we remove uniqueness on 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. |
Probably not. |
|
@markshannon Hi hope you're well, I’ve implemented the suggested changes, could you please let me know if everything looks good, or if there’s anything further I should adjust? Specifically I suspect Thanks! |
markshannon
left a comment
There was a problem hiding this comment.
Just two places where uniqueness removals should be asserts, as the ref should not be unique at that point. Otherwise, this looks good.
Python/optimizer_bytecodes.c
Outdated
| if (sym_is_null(value)) { | ||
| ctx->done = true; | ||
| } | ||
| value = PyJitRef_RemoveUnique(value); |
There was a problem hiding this comment.
| value = PyJitRef_RemoveUnique(value); | |
| assert(!PyJitRef_IsUnique(value)); |
Any ref stored in a local should not be treated as unique
Python/optimizer_bytecodes.c
Outdated
|
|
||
| op(_LOAD_FAST, (-- value)) { | ||
| value = GETLOCAL(oparg); | ||
| value = PyJitRef_RemoveUnique(value); |
|
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. Thanks! |
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
|
@markshannon Hi mark, please take a look at this PR when you have the time. Thanks! |
Some notes:
_BUILD_TUPLE, to be extended to other objects in the future_LOAD_FAST_*and_COPY.Would appreciate any feedback.
Thanks
unique reference trackingin Tier 2 for reference count optimizations #143414