gh-115999: Add free-threaded specialization for FOR_ITER

[Yhg1s]

Add free-threaded versions of existing specialization for FOR_ITER (list, tuples, fast range iterators and generators), without significantly affecting their thread-safety. (Iterating over shared lists/tuples/ranges should be fine like before. Reusing iterators between threads is not fine, like before. Sharing generators between threads is a recipe for significant crashes, like before.)

• Issue: Make the specializing interpreter thread-safe in --disable-gil builds #115999

[Yhg1s]

I still need to add tests for the specialization (rather than the thread-safety, which are in #128637), but otherwise the PR is good enough to review I think.

[mpage]

Why not use the approach that you suggested in discord where we only specialize for the case where the iterator is uniquely referenced by the current thread? It seems like that should cover the overwhelmingly common case, would be simpler to implement, and the resulting instructions would be faster.

[Yhg1s]

I realised that approach wouldn't work for the list referenced by the list iterator (which is the bulk of the work for list iteration) because we're not doing any refcount operations on it (on purpose) and _PyObject_IsUniquelyReferenced() will only do the right thing if all threads hold strong references... and then I think I forgot we should still be able to use that approach for the iterators themselves. I'll see if I can poke holes into that idea tomorrow.

[Yhg1s]

I've added a test that exercises the deopt paths, although for it to pass with ThreadSanitizer relies on #128637 going in first. The test is a little wacky because for list/range/tuple iterators it's not easy to leak the uniquely referenced iterators. (For generators it could just use a weakref.) I wrote the test to make sure the deopt paths were actually safe, but I'm not sure if long-term those tests make sense. We could just keep them around while we fiddle with specialization, but drop them once we're sure the iterators can't leak (or we change the entire approach to iterator specialization.)

[mpage]

It doesn't seem like this comment applies to the tuple specialization. Can you delete it if not?

[Yhg1s]

Done.

[mpage]

Can you expand a little bit on why this doesn't make sense? It'd be nice to keep the structure of the ops the same between the two builds.

[Yhg1s]

Done.

[markshannon]

Why does this need the HAS_ESCAPES_FLAG? How does it escape?

[Yhg1s]

The mechanism for fetching an item from a shared list includes calling _Py_TryIncrefCompare, which has a path which DECREFs.

[markshannon]

Which path? I'm confused as why an incref would need to decref.

[Yhg1s]

When the reference is removed from the array that backs the list between the time it's retrieved and it's returned.

cpython/Include/internal/pycore_object.h

Lines 595 to 610 in 180ee43

	/* Tries to incref the object op and ensures that *src still points to it. */
	staticinlineint
	_Py_TryIncrefCompare(PyObject**src, PyObject*op)
	{
	if (_Py_TryIncrefFast(op)){
	return1;
	}
	if (!_Py_TryIncRefShared(op)){
	return0;
	}
	if (op!=_Py_atomic_load_ptr(src)){
	Py_DECREF(op);
	return0;
	}
	return1;
	}

Added in #114512.

[markshannon]

I don't see how that works. What happens if *src is modified after op != _Py_atomic_load_ptr(src) but before the function returns?

Regardless, we want to keep FOR_ITER_LIST non-escaping in the default build.

[Yhg1s]

@colesbury do you remember why lists use _Py_TryXGetRef (in list_get_item_ref) or why it matters that the src ptr still refers to the same item? Can the object be invalid in some way that still requires us to DECREF it?

[colesbury]

There are two hazards that _Py_TryXGetRef handles:

1. The object op may be deallocated between the initial load and the incref. The try-incref, in cooperation with the allocator, handles this case. It returns zero if op has zero refcount.
2. The memory block for op may be deallocated and reallocated for a different object in between the initial load and incref. In that case the try-incref succeeds, but the subsequent check fails and we need to decref op.

It does not matter if *src is modified after op != _Py_atomic_load_ptr(src). op will still be a valid reference to an object that was the next element in the list at some point during the operation. It's always been possible for the list to be concurrently modified between the execution of FOR_ITER and subsequent code that uses the result.

See https://peps.python.org/pep-0703/#optimistically-avoiding-locking

Regardless, we want to keep FOR_ITER_LIST non-escaping in the default build

Is there any performance reason to do so?

[brandtbucher]

Regardless, we want to keep FOR_ITER_LIST non-escaping in the default build

Is there any performance reason to do so?

Just to follow up: this does make JIT code slightly worse for traces covering for loops over lists. Since the _ITER_NEXT_LIST at the top of the loop is now escaping, this will add a new _CHECK_VALIDITY uop to the start of each loop body where none exists currently.

Consider this loop:

deff(): x=0foriinlist(range(10_000)): x+=i

This change increases the number of validity checks per loop from 2 to 3. Before:

_MAKE_WARM _SET_IP _CHECK_PERIODIC _CHECK_VALIDITY <------------- OLD _ITER_CHECK_LIST _GUARD_NOT_EXHAUSTED_LIST _ITER_NEXT_LIST _SET_IP _STORE_FAST_1 _CHECK_VALIDITY <------------- OLD _LOAD_FAST_0 _LOAD_FAST_1 _GUARD_BOTH_INT _BINARY_OP_ADD_INT _SET_IP _STORE_FAST_0 _JUMP_TO_TOP 

After:

_MAKE_WARM _SET_IP _CHECK_PERIODIC _CHECK_VALIDITY <------------- OLD _ITER_CHECK_LIST _GUARD_NOT_EXHAUSTED_LIST _ITER_NEXT_LIST _CHECK_VALIDITY_AND_SET_IP <-- NEW _STORE_FAST_1 _CHECK_VALIDITY <------------- OLD _LOAD_FAST_0 _LOAD_FAST_1 _GUARD_BOTH_INT _BINARY_OP_ADD_INT _SET_IP _STORE_FAST_0 _JUMP_TO_TOP 

[markshannon]

Just to clarify:
FOR_ITER_LIST is escaping because _PyList_GetItemRefNoLock is escaping, and _PyList_GetItemRefNoLock is escaping because _Py_TryIncrefCompareStackRef is escaping.

Either we need to find an alternative approach or make _Py_TryIncrefCompareStackRef non-escaping.

[Yhg1s]

@markshannon This is the FOR_ITER specialization you wanted to take another look at.

[markshannon]

It's a shame about FOR_ITER_LIST becoming escaping.

We need to make _Py_TryIncrefCompareStackRef non-escaping, but not in this PR.
Otherwise we'll end up with everything escaping, as more code uses _Py_TryIncrefCompareStackRef.

[markshannon]

Just to clarify:
FOR_ITER_LIST is escaping because _PyList_GetItemRefNoLock is escaping, and _PyList_GetItemRefNoLock is escaping because _Py_TryIncrefCompareStackRef is escaping.

Either we need to find an alternative approach or make _Py_TryIncrefCompareStackRef non-escaping.

[Yhg1s]

We need to make _Py_TryIncrefCompareStackRef non-escaping, but not in this PR. Otherwise we'll end up with everything escaping, as more code uses _Py_TryIncrefCompareStackRef.

I think we discussed this in the weekly meeting a month or two ago... I think the only way to avoid it is to delay DECREF'ing objects we've accidentally incorrectly INCREF'ed -- that is to say, objects that were newly allocated from the same memory as the object we were trying to INCREF but we lost the INCREF/DECREF race with another thread on. I think in this case that might not be too bad: if the INCREF fails we deopt anyway, and we could delay that DECREF until that deopt. For other cases where we need to TryIncref, I'm not sure of the impact of delaying (plus, we'd need some mechanism for the delay).

[Yhg1s]

I think the only way to avoid it is to delay DECREF'ing objects we've accidentally incorrectly INCREF'ed -- that is to say, objects that were newly allocated from the same memory as the object we were trying to INCREF but we lost the INCREF/DECREF race with another thread on. I think in this case that might not be too bad: if the INCREF fails we deopt anyway, and we could delay that DECREF until that deopt.

@markshannon let me know if you want that to happen in this PR (but I'd prefer it not).

[markshannon]

No need to do it in this PR.