gh-134584: Eliminate redundant refcounting from _UNPACK_SEQUENCE_TWO_TUPLE

[cocolato]

• Issue: Eliminate redundant refcounting in the JIT #134584

[Fidget-Spinner]

I need to discuss with Mark for this one. As it might cause a stack overflow if the stack bounds are exceeded.

[markshannon]

@cocolato Thanks for the contribution.

Unfortunately, I don't think that this transformation is going to be useful.
Most unpacking is of new references, typically a, b = foo(), so we wouldn't be able to eliminate the reference counting operation.

There is potentially an optimization we can do with _UNPACK_SEQUENCE but it is considerably more complex.
If we can track which references are unique references to a new object, we can eliminate even more refcount operations.
For example: a, b = foo(), where foo ends return x, y, once specialized and traced gives us:

... _BUILD_TUPLE 2 _RETURN_VALUE _GUARD_TOS_TUPLE _UNPACK_SEQUENCE_TWO_TUPLE 

If we know that the reference to the tuple in _UNPACK_SEQUENCE_TWO_TUPLE is unique we could extract the contents and free the tuple without any reference counting operations.

[cocolato]

Thanks for the explanation! I now see that it doesn’t reduce refcount overhead for the common case of consuming new references (return values), so I’ll convert this PR to a draft. I might dig into how the Tier 2 optimizer could track such uniqueness.

[Fidget-Spinner]

@cocolato please reduce this PR to just _UNPACK_SEQUENCE_TWO_TUPLE. I will merge that one as it's actually worth it.

[cocolato]

@cocolato please reduce this PR to just _UNPACK_SEQUENCE_TWO_TUPLE. I will merge that one as it's actually worth it.

I have reduced this PR to _UNPACK_SEQUENCE_TWO_TUPLE. Regarding _UNPACK_SEQUENCE_TUPLE and _UNPACK_SEQUENCE_LIST, should I proceed with similar changes, or should these wait until we can track unique references to new objects as Mark suggested? I’ve been studying the Tier 2 tracing implementation recently to see how we might achieve that.

[Fidget-Spinner]

I have reduced this PR to _UNPACK_SEQUENCE_TWO_TUPLE. Regarding _UNPACK_SEQUENCE_TUPLE and _UNPACK_SEQUENCE_LIST, should I proceed with similar changes, or should these wait until we can track unique references to new objects as Mark suggested? I’ve been studying the Tier 2 tracing implementation recently to see how we might achieve that.

Cool! I think you can skip those for now and wait till we can do what Mark suggested. Let me know if you need any help understanding the implementation.

[Fidget-Spinner]

Hmm thinking about this further, I think Mark is right. I'm really sorry for asking you to reduce the PR! We need to track unique references to get the most benefit.

[cocolato]

I have reduced this PR to _UNPACK_SEQUENCE_TWO_TUPLE. Regarding _UNPACK_SEQUENCE_TUPLE and _UNPACK_SEQUENCE_LIST, should I proceed with similar changes, or should these wait until we can track unique references to new objects as Mark suggested? I’ve been studying the Tier 2 tracing implementation recently to see how we might achieve that.

Cool! I think you can skip those for now and wait till we can do what Mark suggested. Let me know if you need any help understanding the implementation.

Hi, I want to confirm: if we implement "Unique Reference Tracking", should we track only the current UNPACK_TUPLE family now or track a broader range of uops? I'm currently preparing to add the REF_IS_UNIQUE bit (value 2) to JitOptRef for marking unique references. Do you have any other modification suggestions?

cpython/Include/internal/pycore_optimizer.h

Lines 196 to 204 in 9609574

	#defineREF_IS_BORROWED 1
	#defineJIT_BITS_TO_PTR_MASKED(REF) ((JitOptSymbol *)(((REF).bits) & (~REF_IS_BORROWED)))
	staticinlineJitOptSymbol*
	PyJitRef_Unwrap(JitOptRefref)
	{
	returnJIT_BITS_TO_PTR_MASKED(ref);
	}

[cocolato]

Or should we open a new issue?

[Fidget-Spinner]

@cocolato open a new issue for this please, thank you.

You can start small with just tuple ops for now, then we can expand in the future.

[Fidget-Spinner]

REF_IS_UNIQUE sounds like a good way to do it. You need to change the PyJitRef_StripReferenceInfo to only strip reference info for REF_IS_BORROWED and now REF_IS_UNIQUE.