gh-129069: make list ass_slice and memory_repeat safe in free-threading #131882
Conversation
|
Ping @colesbury , @Yhg1s , Windows doesn't like |
tom-pytel
changed the title
Misc/NEWS.d/next/Core_and_Builtins/2025-03-29-20-12-28.gh-issue-129069.QwbbWV.rst
Outdated
Show resolved
Hide resolved
|
All commit authors signed the Contributor License Agreement. |
Include/cpython/pyatomic.h
Outdated
| } | ||
|
|
||
| static inline void * | ||
| _Py_atomic_memmove_ptr_store_relaxed(void *dest, void *src, Py_ssize_t n) |
There was a problem hiding this comment.
I think these two functions should take size_t like memcpy does
There was a problem hiding this comment.
I'm pretty sure I was told at some point to use Py_ssize_t for passing sizes exclusively, hence the cast in FT_ATOMIC_MEMMOVE_PTR_STORE_RELAXED and cast back in _Py_atomic_memmove_ptr_store_relaxed, but technically it really doesn't matter, will change.
There was a problem hiding this comment.
Would you add tests for these in this PR as well? Then we can remove the suppressions for these as well in this PR.
There was a problem hiding this comment.
Sorry if I wasn't clear but I meant to add thread safety tests of list which uses these methods.
You can do something like slice list in one thread while it is being mutated in another thread.
There was a problem hiding this comment.
Sending up test that SHOULD work but can't test it because for some reason can't build with TSAN anymore. This works:
./configure --prefix=`realpath -e ../local` --with-pydebug
...
checking whether year with century should be normalized for strftime... yes
checking whether C99-compatible strftime specifiers are supported... yes
But this craps out, any idea? Something changed?
./configure --prefix=`realpath -e ../local` --with-pydebug --disable-gil --with-thread-sanitizer --disable-ipv6
...
checking whether year with century should be normalized for strftime... no
checking whether C99-compatible strftime specifiers are supported... configure: error: Python requires C99-compatible strftime specifiers
Should add test_list.py to TSAN_TESTS?
There was a problem hiding this comment.
Can you try building with a clean checkout? Also what platform are you using?
There was a problem hiding this comment.
Ok, weird, it wasn't working Linux native but rebooted to run the same partition in a VM and works fine. So its something with my native setup, nm.
Anyways, fixed the test and noticed you said a safety, not a tsan test, so removed the tsan requirement from it. But the weird thing is I couldn't trigger TSAN warnings when reverted to non-atomic memcpy and memmove. Didn't try too hard though.
This PR exists because of #129069 and
Tools/tsan/suppressions_free_threading.txt:It seems to hit the requirements: performance, atomicity assured (per pointer), TSAN shuts up. Pretty sure can remove the
list_ass_slice_lock_heldandlist_inplace_repeat_lock_heldsuppressions, and if not yet then should be able to add the atomic memcpy tolist_resizewhere 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 intopyatomic.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_RELAXEDcompiles to, its norep movsqor other specialized move instructions, but its fast:Simple benchmark. Note the decrementing address copy case in
FT_ATOMIC_MEMMOVE_PTR_RELAXEDseems 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 inpyperformance. 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 realmemmoveis used in this case but the difference seems to come from the realmemmoveusing 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 (tdelandtins big) are solid where they are. Test script:Times, average of 10 runs each:
pyperformancebenchmark. Difference in average performance is essentially nonexistent though individual tests can vary a bit (AMD 7950x running in VirtualBox):