gh-120608: Make reversed iterator work with free-threading

[eendebakpt]

In this PR we make the reversed iterator thread-safe. In the free-threading build the index ro->index needs to be loaded and updated atomically. Also we need to prevent the pointer ro->seq from being released in one the thread while one of the other threads is still using it.

Update: based on review comments this PR makes the reversed work under free-threading, but it is not guaranteed to be thread-safe

• There seems to be no dedicated file for the reversed iterator, so a new one was added,

• Issue: reversed is not thread-safe #120608

[colesbury]

Like we discussed in #120496, I do not think that we should make reversed thread safe.

[corona10]

How impact for the single thread performance? I am +0 with this change.
(If the change is not that big but basically concur the Sam's opinion)

[eendebakpt]

Like we discussed in #120496, I do not think that we should make reversed thread safe.

Maybe I misunderstood or the description of the PR is not clear enough. Without this PR the interpreter can lock or freeze in the free-threaded build. As a side effect of this PR the reversed would be thread-safe in the sense of returning expected results, but that is not the main goal.

[eendebakpt]

Performance impact (bit noisy on my system):

list(reversed(range10)): Mean +- std dev: [ft_main] 257 ns +- 10 ns -> [ft_atomics_pr] 240 ns +- 10 ns: 1.07x faster
list(reversed(range1000)): Mean +- std dev: [ft_main] 10.3 us +- 0.3 us -> [ft_atomics_pr] 10.7 us +- 0.5 us: 1.04x slower
list(reversed(list100)): Mean +- std dev: [ft_main] 591 ns +- 15 ns -> [ft_atomics_pr] 572 ns +- 25 ns: 1.03x faster
list(reversed(tuple100)): Mean +- std dev: [ft_main] 606 ns +- 19 ns -> [ft_atomics_pr] 977 ns +- 40 ns: 1.61x slower
list(reversed(userlist100)): Mean +- std dev: [ft_main] 24.2 us +- 0.9 us -> [ft_atomics_pr] 22.9 us +- 0.9 us: 1.06x faster

Benchmark hidden because not significant (1): reversed(range10)

Geometric mean: 1.06x slower

Summary: reversed on range or list is unaffected since they have a __reversed__ method, reversed on a tuple becomes slower, reversed on a custom type is unchanged (overhead of the atomics is small)

Update: we could perhaps remove the atomics from this PR and keep the part where we do not clear ro->seq. That should give the same performance, but avoid corruption of the interpreter.

[corona10]

The change requires some performance overhead, I think that we don't need to make reversed iterator thread-safe as same opinion as Sam if we need to make more implementation effort.

[colesbury]

Without this PR the interpreter can lock or freeze in the free-threaded build

Ok, thanks I missed that.

The relaxed atomics loads and stores are free, but the _Py_atomic_add_ssize is relatively expensive. We want to avoid that, even at the cost of potentially returning the same item multiple times in a race condition.

• Use relaxed atomics to load/store ro->index. (Or you can use the FT_ macros)
• Update the NULL checks for ro->seq to instead check if ro->index is negative (in reversed_len and reversed_reduce)

[eendebakpt]

Should we also update the reversed_setstate method? With this PR we have a behaviour change even when running with just a single thread. Minimal example:

r = reversed((1,2,3))
list(r) # exhaust
r.__setstate__(1)
print(list(r)) # returns [] on normal build, returns [2, 1] on free-threaded build

I believe the behaviour on the free-threaded build makes more sense and that the behaviour on current main is a workaround for the case where ro->seq is cleared. I also doubt that people will actually run into this change in actual code, but it would be nice to have some confirmation on that.

[colesbury]

Yes, I think both reversed_reduce and reversed_setstate should be updated.

I believe the behaviour on the free-threaded build makes more sense...

I would prefer to avoid changing the behavior, even if it doesn't make as much sense.

[colesbury]

Looks good. Two minor suggestions.

[colesbury]

I think the preference I've heard from other core developers is to use for loops instead of using list comprehensions for their side-effects.

[ambv]

The merge with main helped with the "All green" check issue so I'm reverting my shot-in-the-dark trailing comma removal.