gh-128942: make arraymodule.c free-thread safe (lock-free) - #130771
gh-128942: make arraymodule.c free-thread safe (lock-free)#130771tom-pytel wants to merge 85 commits into
Conversation
Sorry, something went wrong.
ZeroIntensity
left a comment
There was a problem hiding this comment.
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.
|
Note, this is not ready to go, there is the memory issue which needs resolving. |
Sorry, something went wrong.
|
@ZeroIntensity you can remove the do-not-merge, its not an arraymodule issue, its a QSBR issue, see #130794. |
Sorry, something went wrong.
|
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.
colesbury
left a comment
There was a problem hiding this comment.
I haven't gotten a chance to look through arraymodule.c yet. I'll review that later this week.
Sorry, something went wrong.
colesbury
left a comment
There was a problem hiding this comment.
The overall approach here seems good. A few comments below.
Sorry, something went wrong.
|
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:
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.
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())
$ ./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.
|
I'd like test_array added to TSAN_TESTS, not TSAN_PARALLEL_TESTS.
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. |
Sorry, something went wrong.
|
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.
|
Ping @AA-Turner, I've removed arraymodule from Setup.bootstrap.in, can you mark the changes you requested as done? |
Sorry, something went wrong.
…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.
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".