gh-129069: make list ass_slice and memory_repeat safe in free-threading

[tom-pytel]

This PR exists because of #129069 and Tools/tsan/suppressions_free_threading.txt:

# List resizing happens through different paths ending in memcpy or memmove # (for efficiency), which will probably need to rewritten as explicit loops # of ptr-sized copies to be thread-safe. 

It seems to hit the requirements: performance, atomicity assured (per pointer), TSAN shuts up. Pretty sure can remove the list_ass_slice_lock_held and list_inplace_repeat_lock_held suppressions, and if not yet then should be able to add the atomic memcpy to list_resize where needed to be able to do so.

The "atomic memcpy" (atomic per ptr, not whole memcpy) functions are in the atomic wrappers header because they can be reused, if want otherwise though can move into listobject.c, or somewhere else. Can also make non-inline callable real functions. Or the inline funcs could go into pyatomic.h?

@colesbury, you may recognize this, I did something similar for the lock-free array module PR, but this is simpler. PyObject pointers are a bit more fragile than pure values though so I assume you want this here?

Here is an example of what the inner loop a FT_ATOMIC_MEMMOVE_PTR_RELAXED compiles to, its no rep movsq or other specialized move instructions, but its fast:

.L2971:# ./Include/cpython/pyatomic_gcc.h:387:{return (void *)__atomic_load_n((void * const *)obj, __ATOMIC_RELAXED)}movrax, QWORD PTR [rdi] # _160,* s# ./Include/internal/pycore_pyatomic_ft_wrappers.h:143: for (void **d = (void **)dest + n,**s = (void **)src + n,**e = (void **)dest -1; d != e; d--, s--){subrsi,64 # d,# ./Include/internal/pycore_pyatomic_ft_wrappers.h:143: for (void **d = (void **)dest + n,**s = (void **)src + n,**e = (void **)dest -1; d != e; d--, s--){subrdi,64 # s,# ./Include/cpython/pyatomic_gcc.h:509:{__atomic_store_n((void **)obj, value, __ATOMIC_RELAXED)}mov QWORD PTR 64[rsi],rax #,, _160movrax, QWORD PTR 56[rdi] # _160,mov QWORD PTR 56[rsi],rax #,, _160# ./Include/cpython/pyatomic_gcc.h:387:{return (void *)__atomic_load_n((void * const *)obj, __ATOMIC_RELAXED)}movrax, QWORD PTR 48[rdi] # _160,mov QWORD PTR 48[rsi],rax #,, _160# ./Include/cpython/pyatomic_gcc.h:387:{return (void *)__atomic_load_n((void * const *)obj, __ATOMIC_RELAXED)}movrax, QWORD PTR 40[rdi] # _160,mov QWORD PTR 40[rsi],rax #,, _160# ./Include/cpython/pyatomic_gcc.h:387:{return (void *)__atomic_load_n((void * const *)obj, __ATOMIC_RELAXED)}movrax, QWORD PTR 32[rdi] # _160,mov QWORD PTR 32[rsi],rax #,, _160# ./Include/cpython/pyatomic_gcc.h:387:{return (void *)__atomic_load_n((void * const *)obj, __ATOMIC_RELAXED)}movrax, QWORD PTR 24[rdi] # _160,mov QWORD PTR 24[rsi],rax #,, _160# ./Include/cpython/pyatomic_gcc.h:387:{return (void *)__atomic_load_n((void * const *)obj, __ATOMIC_RELAXED)}movrax, QWORD PTR 16[rdi] # _160,mov QWORD PTR 16[rsi],rax #,, _160# ./Include/cpython/pyatomic_gcc.h:387:{return (void *)__atomic_load_n((void * const *)obj, __ATOMIC_RELAXED)}movrax, QWORD PTR 8[rdi] # _160,mov QWORD PTR 8[rsi],rax #,, _160cmp QWORD PTR 32[rsp],rsi # %sfp, djne .L2971 #,

Simple benchmark. Note the decrementing address copy case in FT_ATOMIC_MEMMOVE_PTR_RELAXED seems to hurt a bit cache-wise in the 'tins' simple bench, but it doesn't seem to make a difference in the large move or overall in pyperformance. I've had this one jump around from parity with current to this (which is the worst case so I put it here). The difference disappears if the real memmove is used in this case but the difference seems to come from the real memmove using special instructions (which are not atomic), a la: https://codebrowser.dev/glibc/glibc/sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S.html. The other two simple (tdel and tins big) are solid where they are. Test script:

fromtimeitimporttimeitfromtimeimporttimetdel=timeit('del dst[:1024]', 'dst = [None] * 4096', number=10000000) print(f't del: {tdel}') tins=timeit('dst[:0] = src', 'dst = [None] * 4096; dst.pop(); src = [None]', number=100000) # pop() to make sure no realloc in timingprint(f't ins: {tins}') dst= [None] *0x10000000src= [None] dst.pop() t0=time() dst[:0] =srct1=time() print(f't ins big: {t1-t0}')

Times, average of 10 runs each:

current main: ------------- t del: 0.2191115931245804 t ins: 0.3050231862498549 t ins big: 0.0799584686756134 fix-129069: ----------- t del: 0.21582267370067712 t ins: 1.072859044800134 t ins big: 0.08154952526092529 

pyperformance benchmark. Difference in average performance is essentially nonexistent though individual tests can vary a bit (AMD 7950x running in VirtualBox):

 current main fix-129069 norm diff async_generators 250 254 -0.0157 asyncio_tcp 248 247 0.0040 asyncio_tcp_ssl 1.08 1.06 0.0189 asyncio_websockets 344 342 0.0058 chaos 42.3 42.4 -0.0024 comprehensions 11.1 11 0.0091 bench_mp_pool 17.5 17.4 0.0057 bench_thread_pool 1.16 1.09 0.0642 coroutines 12.8 12.8 0.0000 coverage 57 54.5 0.0459 crypto_pyaes 51.9 51.6 0.0058 deepcopy 182 180 0.0111 deepcopy_reduce 1.91 1.94 -0.0155 deepcopy_memo 20.2 21.7 -0.0691 deltablue 2.3 2.3 0.0000 django_template 25.2 25 0.0080 docutils 1.61 1.58 0.0190 dulwich_log 26.3 25.8 0.0194 fannkuch 260 257 0.0117 float 42.4 43.5 -0.0253 create_gc_cycles 494 512 -0.0352 gc_traversal 1.17 1.27 -0.0787 generators 20.9 21.4 -0.0234 genshi_text 16.6 16.7 -0.0060 genshi_xml 34.3 34.9 -0.0172 go 80.3 80.8 -0.0062 hexiom 4.17 4.22 -0.0118 html5lib 33.2 34 -0.0235 json_dumps 7.43 7.31 0.0164 json_loads 14.2 13.9 0.0216 logging_format 4.58 4.61 -0.0065 logging_silent 64.8 63.7 0.0173 logging_simple 4.2 4.22 -0.0047 mako 7.95 7.97 -0.0025 mdp 1.78 1.98 -0.1010 meteor_contest 68.2 69.7 -0.0215 nbody 82.6 78.8 0.0482 nqueens 58.6 61.1 -0.0409 pathlib 13.8 13.9 -0.0072 pickle 6.12 6.13 -0.0016 pickle_dict 15.3 15.2 0.0066 pickle_list 2.3 2.18 0.0550 pickle_pure_python 204 203 0.0049 pidigits 116 117 -0.0085 pprint_safe_repr 499 513 -0.0273 pprint_pformat 1.02 1.05 -0.0286 pyflate 296 304 -0.0263 python_startup 9.29 9.29 0.0000 python_startup_no_site 7.78 7.62 0.0210 raytrace 192 193 -0.0052 regex_compile 74.4 74.1 0.0040 regex_dna 111 110 0.0091 regex_effbot 1.69 1.7 -0.0059 regex_v8 13.8 13.7 0.0073 richards 32.1 32.4 -0.0093 richards_super 36.8 37.3 -0.0134 scimark_fft 232 231 0.0043 scimark_lu 81 80.7 0.0037 scimark_monte_carlo 45.9 47.9 -0.0418 scimark_sor 77.9 76.3 0.0210 scimark_sparse_mat_mult 3.83 3.78 0.0132 spectral_norm 68.5 67 0.0224 sqlglot_normalize 197 196 0.0051 sqlglot_optimize 36.8 36.9 -0.0027 sqlglot_parse 887 888 -0.0011 sqlglot_transpile 1.1 1.1 0.0000 sqlite_synth 1.43 1.44 -0.0069 sympy_expand 277 276 0.0036 sympy_integrate 12.8 12.7 0.0079 sympy_sum 88.1 85.3 0.0328 sympy_str 164 162 0.0123 telco 5.47 5.39 0.0148 tomli_loads 1.37 1.37 0.0000 typing_runtime_protocols 112 112 0.0000 unpack_sequence 28.7 28.6 0.0035 unpickle 8 8.18 -0.0220 unpickle_list 2.87 2.75 0.0436 unpickle_pure_python 144 140 0.0286 xml_etree_parse 74 72.6 0.0193 xml_etree_iterparse 49.6 47.8 0.0377 xml_etree_generate 56.2 56.9 -0.0123 xml_etree_process 40.4 40.3 0.0025 AVERAGE -0.0001 

• Issue: Race in concurrent list mutation and item retrieval #129069

[tom-pytel]

Ping @colesbury , @Yhg1s , Windows doesn't like size_t as a parameter, will fix, but would like a thumbs up or down on this PR in general?

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[tom-pytel]

Thank you @mhsmith for the analysis, it led to the solution.

Specifically:

cpython/configure

Line 30963 in 78acd8e

#include "pyatomic.h"

needs to be accomodated on ARM Android build (but apparently nowhere else, and I swear it compiled fine a couple weeks ago).

@kumaraditya303 I had to revert #131882 (comment) because _Py_IS_ALIGNED is not available in the above case. In fact I had to add #include <assert.h> to pyatomic.h in order to accomodate even direct alignment validation. Which leads me to a question, should we even bother with those assertions here (which necessitates that include)?

[mhsmith]

As I explained in my previous comment, this is a macOS problem, not an Android one. If the PR was merged in its previous state, it would have failed on the macOS buildbots, unless they use the configure cache in the same unsafe way as GitHub Actions does.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 9c4debd 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131882%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[tom-pytel]

WHY???

 extra_compile_args: [..., '-Werror=declaration-after-statement', '-DMODULE_NAME=_test_c99_cext', '-std=c99'] /Users/buildbot/buildarea/pull_request.pablogsal-macos-m1.macos-with-brew/build/Include/cpython/pyatomic.h:590:16: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement] 590 | void **dest_ = (void **)dest + n; | ^ 1 error generated. 

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 32f9604 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131882%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[colesbury]

Hi @tom-pytel, thanks for your work on this and sorry for letting the PR languish. I've taken a slightly different approach in #142957 for the same issue.

[tom-pytel]

Hi @tom-pytel, thanks for your work on this and sorry for letting the PR languish. I've taken a slightly different approach in #142957 for the same issue.

No worries, I'll close this out. But what should I do with #130771, close or is it still viable?