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 28 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.
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
markshannon
left a comment
There was a problem hiding this comment.
I think we should be very conservative in the analysis, and only track references on the evaluation stack. It simplifies the code a bit, and means we don't need to worry about escapes.
FTR, this optimizations is going to be rendered obsolete by #120619 which will eliminate many temporary tuples entirely.
BUILD_TUPLE 2; _RETURN_VALUE; UNPACK 2 -> _RETURN_PAIR
Python/optimizer_bytecodes.c
Outdated
| } | ||
|
|
||
| op(_UNPACK_SEQUENCE_TWO_TUPLE, (seq -- val1, val0)) { | ||
| if (PyJitRef_IsUnique(seq)) { |
There was a problem hiding this comment.
Do the length check here, and not in the uop at runtime. If we have tracked the uniqueness of the reference, we most likely know the size of the tuple
Python/bytecodes.c
Outdated
| assert(oparg == 2); | ||
| PyObject *seq_o = PyStackRef_AsPyObjectSteal(seq); | ||
| assert(PyTuple_CheckExact(seq_o)); | ||
| DEOPT_IF(PyTuple_GET_SIZE(seq_o) != 2); |
There was a problem hiding this comment.
Do this check in the optimizer
Python/bytecodes.c
Outdated
| PyObject *seq_o = PyStackRef_AsPyObjectBorrow(seq); | ||
| assert(PyTuple_CheckExact(seq_o)); | ||
| assert(PyTuple_GET_SIZE(seq_o) == oparg); | ||
| DEOPT_IF(!_PyObject_IsUniquelyReferenced(seq_o)); |
There was a problem hiding this comment.
If we only track references on the evaluation stack, then there is no need for this check as the reference cannot escape.
As soon as a value is stored in a local, it is unlikely to only have one reference to it when we actually use it.
Only tracking references on the stack means we don't need to worry about the various forms of LOAD_FAST, just STORE_FAST.
|
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. |
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