GeezerVison is showing github.com — gh-128942: make arraymodule.c free-thread safe (lock-free) by tom-pytel · Pull Request #130771 · python/cpython
Conversation
tom-pytel
commented
Mar 2, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
I added lock-free single element reads and writes by mostly copying the list object's homework. TL;DR: pyperformance scimark seems to be back to about what it was without the free-thread safe stuff (pending confirmation of course). Tried a few other things but the list strategy seems good enough (except for the negative index thing I mentioned in #130744, if that is an issue).
Timings, the relevant ones are "OLD" - non free-thread safe arraymodule, "SLOW" - the previous slower PR and the last two "LFREERW".
### scimark_fft ###
WITHGIL: Mean +- std dev: 200 ms +- 6 ms
OLD: Mean +- std dev: 238 ms +- 7 ms
SLOW: Mean +- std dev: 274 ms +- 4 ms
ITEMFIX: Mean +- std dev: 272 ms +- 14 ms
NOLOCK: Mean +- std dev: 240 ms +- 4 ms
TEST1: Mean +- std dev: 267 ms +- 4 ms
TEST2: Mean +- std dev: 255 ms +- 6 ms
LFREERD: Mean +- std dev: 256 ms +- 7 ms # left in standard Py_SIZE() in getarrayitem
LFREERD: Mean +- std dev: 262 ms +- 7 ms # atomic read of size in getarrayitem
LFREERD: Mean +- std dev: 270 ms +- 7 ms # build and run again
LFREERD: Mean +- std dev: 255 ms +- 10 ms # orthodox
LFREERD: Mean +- std dev: 259 ms +- 8 ms # build and run again
LFREERW: Mean +- std dev: 239 ms +- 7 ms # lockfree write single element
LFREERW: Mean +- std dev: 242 ms +- 6 ms # build and run again
STATICL: Mean +- std dev: 223 ms +- 6 ms # statically linked
STATICL: Mean +- std dev: 223 ms +- 5 ms # build and run again
CLEANUP: Mean +- std dev: 224 ms +- 5 ms # including atomic get/set
AMEMCPY: Mean +- std dev: 226 ms +- 6 ms # atomic item copy on resize (instead of memcpy)
AMEMCPY: Mean +- std dev: 222 ms +- 5 ms # build and run again
LOCKWR: Mean +- std dev: 226 ms +- 4 ms # locked writes again, but much better with static linking
LOCKWR: Mean +- std dev: 226 ms +- 5 ms # build and run again
AMCAGG: Mean +- std dev: 222 ms +- 7 ms # aggregate memcpy (just using best build)
### scimark_lu ###
WITHGIL: Mean +- std dev: 64.6 ms +- 1.9 ms
OLD: Mean +- std dev: 89.0 ms +- 2.1 ms
SLOW: Mean +- std dev: 95.4 ms +- 3.1 ms
ITEMFIX: Mean +- std dev: 92.0 ms +- 2.1 ms
NOLOCK: Mean +- std dev: 88.5 ms +- 2.5 ms
TEST1: Mean +- std dev: 89.7 ms +- 2.1 ms
TEST2: Mean +- std dev: 90.5 ms +- 2.4 ms
LFREERD: Mean +- std dev: 89.9 ms +- 2.7 ms
LFREERD: Mean +- std dev: 90.9 ms +- 2.6 ms
LFREERD: Mean +- std dev: 93.4 ms +- 4.0 ms
LFREERD: Mean +- std dev: 91.9 ms +- 3.7 ms
LFREERD: Mean +- std dev: 91.0 ms +- 5.0 ms
LFREERW: Mean +- std dev: 89.2 ms +- 2.8 ms
LFREERW: Mean +- std dev: 88.8 ms +- 2.5 ms
STATICL: Mean +- std dev: 86.9 ms +- 1.9 ms
STATICL: Mean +- std dev: 87.1 ms +- 2.2 ms
CLEANUP: Mean +- std dev: 88.9 ms +- 2.5 ms
AMEMCPY: Mean +- std dev: 87.9 ms +- 1.9 ms
AMEMCPY: Mean +- std dev: 88.7 ms +- 4.8 ms
LOCKWR: Mean +- std dev: 88.0 ms +- 2.4 ms
LOCKWR: Mean +- std dev: 88.5 ms +- 3.5 ms
AMCAGG: Mean +- std dev: 88.5 ms +- 3.5 ms
### scimark_monte_carlo ###
WITHGIL: Mean +- std dev: 40.1 ms +- 2.3 ms
OLD: Mean +- std dev: 53.4 ms +- 1.0 ms
SLOW: Mean +- std dev: 56.8 ms +- 1.4 ms
ITEMFIX: Mean +- std dev: 55.9 ms +- 0.7 ms
NOLOCK: Mean +- std dev: 54.4 ms +- 1.3 ms
TEST1: Mean +- std dev: 54.1 ms +- 0.6 ms
TEST2: Mean +- std dev: 54.1 ms +- 0.6 ms
LFREERD: Mean +- std dev: 54.3 ms +- 0.9 ms
LFREERD: Mean +- std dev: 55.9 ms +- 0.9 ms
LFREERD: Mean +- std dev: 55.4 ms +- 1.1 ms
LFREERD: Mean +- std dev: 55.4 ms +- 1.1 ms
LFREERD: Mean +- std dev: 55.2 ms +- 1.5 ms
LFREERW: Mean +- std dev: 53.2 ms +- 1.1 ms
LFREERW: Mean +- std dev: 53.9 ms +- 1.5 ms
STATICL: Mean +- std dev: 55.0 ms +- 1.9 ms
STATICL: Mean +- std dev: 53.3 ms +- 1.0 ms
CLEANUP: Mean +- std dev: 52.6 ms +- 0.5 ms
AMEMCPY: Mean +- std dev: 53.3 ms +- 0.6 ms
AMEMCPY: Mean +- std dev: 53.4 ms +- 1.2 ms
LOCKWR: Mean +- std dev: 55.4 ms +- 4.0 ms
LOCKWR: Mean +- std dev: 54.8 ms +- 1.8 ms
AMCAGG: Mean +- std dev: 53.1 ms +- 1.0 ms
### scimark_sor ###
WITHGIL: Mean +- std dev: 70.3 ms +- 3.3 ms
OLD: Mean +- std dev: 98.1 ms +- 1.8 ms
SLOW: Mean +- std dev: 99.9 ms +- 1.1 ms
ITEMFIX: Mean +- std dev: 100 ms +- 1 ms
NOLOCK: Mean +- std dev: 97.3 ms +- 1.4 ms
TEST1: Mean +- std dev: 97.5 ms +- 0.7 ms
TEST2: Mean +- std dev: 98.5 ms +- 1.5 ms
LFREERD: Mean +- std dev: 97.0 ms +- 2.0 ms
LFREERD: Mean +- std dev: 98.2 ms +- 1.4 ms
LFREERD: Mean +- std dev: 98.4 ms +- 2.7 ms
LFREERD: Mean +- std dev: 97.6 ms +- 1.5 ms
LFREERD: Mean +- std dev: 97.9 ms +- 1.8 ms
LFREERW: Mean +- std dev: 97.2 ms +- 1.6 ms
LFREERW: Mean +- std dev: 96.9 ms +- 2.0 ms
STATICL: Mean +- std dev: 97.8 ms +- 1.9 ms
STATICL: Mean +- std dev: 97.2 ms +- 1.7 ms
CLEANUP: Mean +- std dev: 97.7 ms +- 1.2 ms
AMEMCPY: Mean +- std dev: 96.0 ms +- 2.0 ms
AMEMCPY: Mean +- std dev: 96.4 ms +- 1.5 ms
LOCKWR: Mean +- std dev: 97.7 ms +- 1.5 ms
LOCKWR: Mean +- std dev: 96.8 ms +- 2.5 ms
AMCAGG: Mean +- std dev: 95.6 ms +- 0.5 ms
### scimark_sparse_mat_mult ###
WITHGIL: Mean +- std dev: 2.89 ms +- 0.11 ms
OLD: Mean +- std dev: 3.94 ms +- 0.15 ms
SLOW: Mean +- std dev: 4.68 ms +- 0.07 ms
ITEMFIX: Mean +- std dev: 4.34 ms +- 0.07 ms
NOLOCK: Mean +- std dev: 3.94 ms +- 0.58 ms
TEST1: Mean +- std dev: 3.99 ms +- 0.12 ms
TEST2: Mean +- std dev: 3.80 ms +- 0.12 ms
LFREERD: Mean +- std dev: 3.84 ms +- 0.17 ms
LFREERD: Mean +- std dev: 3.83 ms +- 0.15 ms
LFREERD: Mean +- std dev: 3.94 ms +- 0.12 ms
LFREERD: Mean +- std dev: 3.74 ms +- 0.12 ms
LFREERD: Mean +- std dev: 3.83 ms +- 0.19 ms
LFREERW: Mean +- std dev: 3.84 ms +- 0.19 ms
LFREERW: Mean +- std dev: 3.87 ms +- 0.17 ms
STATICL: Mean +- std dev: 3.37 ms +- 0.08 ms
STATICL: Mean +- std dev: 3.49 ms +- 0.12 ms
CLEANUP: Mean +- std dev: 3.45 ms +- 0.14 ms
AMEMCPY: Mean +- std dev: 3.44 ms +- 0.13 ms
AMEMCPY: Mean +- std dev: 3.46 ms +- 0.14 ms
LOCKWR: Mean +- std dev: 3.52 ms +- 0.47 ms
LOCKWR: Mean +- std dev: 3.44 ms +- 0.10 ms
AMCAGG: Mean +- std dev: 3.38 ms +- 0.09 ms
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Disclaimer: I'm not an expert on the FT list implementation, so take some of my comments with a grain of salt.
Seeing good single-threaded performance is nice, but what about multi-threaded scaling? The number of locks that are still here scare me a little--it would be nice if this scaled well for concurrent use as well, especially for operations that don't require concurrent writes (e.g., comparisons and copies).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
stonebig reacted with heart emoji
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Note, this is not ready to go, there is the memory issue which needs resolving.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@ZeroIntensity you can remove the do-not-merge, its not an arraymodule issue, its a QSBR issue, see #130794.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
The main thing here for acceptance is a benchmark run which I am not able to start (I only did local pyperformance check against main), so someone with access will have to initiate that to compare with main.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't gotten a chance to look through arraymodule.c yet. I'll review that later this week.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall approach here seems good. A few comments below.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
tom-pytel
commented
Mar 5, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
The actual arraymodule cleanup is fine, just one thing you should pay attention to when looking at this:
data->items is not a deref but pointer arithmetic so is safe to use if data is NULL in memcopies and _PyBytes_Repeat when associated size is 0. But there are places where this might not be safe which are accomodated, specifically:
array_array_buffer_info_implarray_array_tobytes_implarray_array_fromunicode_implarray_array_tounicode_implarray_buffer_getbuf_lock_held
Are there any other places where this needs to take place?
Its the test and trying to run it with parallel-threads with tsan that's problematic.
warnings doesn't play nice with --parallel-threads, found a least evil solution but its not wonderful. This still gets "env changed" but at least can run. Is there a way to check for parallel-threads other than this?
def setUp(self):
if (not support.Py_GIL_DISABLED or
any('"parallel_threads": null' in a for a in sys.argv) or
all('parallel_threads' not in a for a in sys.argv)):
self.enterContext(warnings.catch_warnings())
-
--parallel-threads doesn't play well with the threaded FreeThreadingTest, get dangling threads sometimes.
-
./Include/cpython/pyatomic_gcc.h:615:3: warning: ‘atomic_thread_fence’ is not supported with ‘-fsanitize=thread’ [-Wtsan]
Do we care?
-
And worst of all, cleaned up data races in arraymodule (as far as I can see). I used to be able to run test stuff setting supperssions without problems but with test_array I get:
$ ./python -m test --parallel-threads=4 -j4 test_array
Using random seed: 2354862211
0:00:00 load avg: 0.55 Run 1 test in parallel using 1 worker process
0:00:30 load avg: 0.80 running (1): test_array (30.1 sec)
0:00:41 load avg: 0.75 [1/1/1] test_array worker non-zero exit code (Exit code 66)
==================
WARNING: ThreadSanitizer: data race (pid=1075807)
Read of size 8 at 0x7f58e22c0098 by thread T4:
#0 _Py_TYPE Include/object.h:268 (python+0x22a21a)
#1 Py_IS_TYPE Include/object.h:291 (python+0x22a21a)
#2 compare_unicode_unicode_threadsafe Objects/dictobject.c:1413 (python+0x22a21a)
#3 do_lookup Objects/dictobject.c:1010 (python+0x22bb38)
#4 unicodekeys_lookup_unicode_threadsafe Objects/dictobject.c:1438 (python+0x22bd9b)
#5 _Py_dict_lookup_threadsafe Objects/dictobject.c:1493 (python+0x232e50)
#6 _PyDict_GetItemRef_KnownHash Objects/dictobject.c:2342 (python+0x233f39)
#7 PyDict_GetItemRef Objects/dictobject.c:2378 (python+0x234070)
#8 cache_struct_converter Modules/_struct.c:2524 (_struct.cpython-314td-x86_64-linux-gnu.so+0xc916)
#9 calcsize Modules/clinic/_struct.c.h:249 (_struct.cpython-314td-x86_64-linux-gnu.so+0xceb0)
#10 cfunction_vectorcall_O Objects/methodobject.c:523 (python+0x25961c)
#11 _PyObject_VectorcallTstate Include/internal/pycore_call.h:167 (python+0x1896b3)
#12 PyObject_Vectorcall Objects/call.c:327 (python+0x189810)
#13 _PyEval_EvalFrameDefault Python/generated_cases.c.h:1371 (python+0x42310d)
...
Previous write of size 8 at 0x7f58e22c0098 by thread T1:
#0 memset ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:799 (libtsan.so.0+0x614cb)
#1 memset ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:797 (libtsan.so.0+0x614cb)
#2 memset /usr/include/x86_64-linux-gnu/bits/string_fortified.h:59 (python+0x274169)
#3 fill_mem_debug Objects/obmalloc.c:2739 (python+0x274169)
#4 _PyMem_DebugRawAlloc Objects/obmalloc.c:2823 (python+0x27429d)
#5 _PyMem_DebugRawMalloc Objects/obmalloc.c:2839 (python+0x2742d6)
#6 _PyMem_DebugMalloc Objects/obmalloc.c:3004 (python+0x274331)
#7 PyObject_Malloc Objects/obmalloc.c:1412 (python+0x296e92)
#8 PyUnicode_New Objects/unicodeobject.c:1407 (python+0x31fd7d)
#9 PyUnicode_Concat Objects/unicodeobject.c:11647 (python+0x335e53)
#10 PyNumber_Add Objects/abstract.c:1135 (python+0x14e11c)
#11 _PyEval_EvalFrameDefault Python/generated_cases.c.h:62 (python+0x41b60b)
...
WARNING: ThreadSanitizer: data race (pid=1079529)
Write of size 8 at 0x7f0050cbd0f0 by thread T17:
#0 descr_get_qualname Objects/descrobject.c:624 (python+0x1ac687)
#1 getset_get Objects/descrobject.c:193 (python+0x1aaf41)
#2 _PyObject_GenericGetAttrWithDict Objects/object.c:1694 (python+0x268a4e)
...
Previous write of size 8 at 0x7f0050cbd0f0 by thread T20:
#0 descr_get_qualname Objects/descrobject.c:624 (python+0x1ac687)
#1 getset_get Objects/descrobject.c:193 (python+0x1aaf41)
#2 _PyObject_GenericGetAttrWithDict Objects/object.c:1694 (python+0x268a4e)
...
Which is not arraymodule stuff and not suppressed. So as of now its not runnable clean under tsan (at least on my system how I am running it).
Left the bad setUp() in test_array for now so u can see but it will be getting removed (or fixed).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
colesbury
commented
Mar 5, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
I'd like test_array added to TSAN_TESTS, not TSAN_PARALLEL_TESTS.
warnings doesn't play nice with --parallel-threads
Yes, warnings is not therad-safe (even with the GIL). Neil is work on making it thread-safe, but for now we shouldn't add the test to TSAN_PARALLEL_TESTS for now. It's not worth trying to work around the warnings stuff.
|
tom-pytel reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ping @AA-Turner, I've removed arraymodule from Setup.bootstrap.in, can you mark the changes you requested as done?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…hon into fix-issue-128942-lockfree
|
This PR is stale because it has been open for 90 days with no activity.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|
ZeroIntensity
left a comment
colesbury
left a comment