GeezerVison is showing github.com — gh-128942: make arraymodule.c free-thread safe by tom-pytel · Pull Request #128943 · python/cpython
Conversation
tom-pytel
commented
Jan 17, 2025
•
edited by bedevere-app
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
This PR is a work in progress but it currently fixes the crashes mentioned in the related issue so I am sending it up now to elicit feedback to make sure I am on the right path. So far the changes have been made functional but as minimal as possible to facilitate review. The critical sections are fairly broad so optimization may follow if functionality is confirmed. The changes are mostly additions of critical sections to publicly visible methods and slots gotten from the two structures listed below (as well as a few internal functions):
static PyMethodDef array_methods[] = {
ARRAY_ARRAY_APPEND_METHODDEF
ARRAY_ARRAY_BUFFER_INFO_METHODDEF
ARRAY_ARRAY_BYTESWAP_METHODDEF
ARRAY_ARRAY_CLEAR_METHODDEF
ARRAY_ARRAY___COPY___METHODDEF
ARRAY_ARRAY_COUNT_METHODDEF
ARRAY_ARRAY___DEEPCOPY___METHODDEF
ARRAY_ARRAY_EXTEND_METHODDEF
ARRAY_ARRAY_FROMFILE_METHODDEF
ARRAY_ARRAY_FROMLIST_METHODDEF
ARRAY_ARRAY_FROMBYTES_METHODDEF
ARRAY_ARRAY_FROMUNICODE_METHODDEF
ARRAY_ARRAY_INDEX_METHODDEF
ARRAY_ARRAY_INSERT_METHODDEF
ARRAY_ARRAY_POP_METHODDEF
ARRAY_ARRAY___REDUCE_EX___METHODDEF
ARRAY_ARRAY_REMOVE_METHODDEF
ARRAY_ARRAY_REVERSE_METHODDEF
ARRAY_ARRAY_TOFILE_METHODDEF
ARRAY_ARRAY_TOLIST_METHODDEF
ARRAY_ARRAY_TOBYTES_METHODDEF
ARRAY_ARRAY_TOUNICODE_METHODDEF
ARRAY_ARRAY___SIZEOF___METHODDEF
{"__class_getitem__", Py_GenericAlias, METH_O|METH_CLASS, PyDoc_STR("See PEP 585")},
{NULL, NULL} /* sentinel */
};
static PyType_Slot array_slots[] = {
{Py_tp_dealloc, array_dealloc},
{Py_tp_repr, array_repr},
{Py_tp_getattro, PyObject_GenericGetAttr},
{Py_tp_doc, (void *)arraytype_doc},
{Py_tp_richcompare, array_richcompare},
{Py_tp_iter, array_iter},
{Py_tp_methods, array_methods},
{Py_tp_members, array_members},
{Py_tp_getset, array_getsets},
{Py_tp_alloc, PyType_GenericAlloc},
{Py_tp_new, array_new},
{Py_tp_traverse, array_tp_traverse},
/* as sequence */
{Py_sq_length, array_length},
{Py_sq_concat, array_concat},
{Py_sq_repeat, array_repeat},
{Py_sq_item, array_item},
{Py_sq_ass_item, array_ass_item},
{Py_sq_contains, array_contains},
{Py_sq_inplace_concat, array_inplace_concat},
{Py_sq_inplace_repeat, array_inplace_repeat},
/* as mapping */
{Py_mp_length, array_length},
{Py_mp_subscript, array_subscr},
{Py_mp_ass_subscript, array_ass_subscr},
/* as buffer */
{Py_bf_getbuffer, array_buffer_getbuf},
{Py_bf_releasebuffer, array_buffer_relbuf},
{0, NULL},
};The following public methods/slots have been modified directly:
* array_array_buffer_info = ARRAY_ARRAY_BUFFER_INFO_METHODDEF
* array_array_byteswap = ARRAY_ARRAY_BYTESWAP_METHODDEF
* array_array_clear = ARRAY_ARRAY_CLEAR_METHODDEF
* array_array_count = ARRAY_ARRAY_COUNT_METHODDEF
* array_array_fromlist = ARRAY_ARRAY_FROMLIST_METHODDEF / CRITICAL2
* array_array_fromunicode = ARRAY_ARRAY_FROMUNICODE_METHODDEF
* array_array_index = ARRAY_ARRAY_INDEX_METHODDEF
* array_array_pop = ARRAY_ARRAY_POP_METHODDEF
* array_array_remove = ARRAY_ARRAY_REMOVE_METHODDEF
* array_array_reverse = ARRAY_ARRAY_REVERSE_METHODDEF
* array_array_tofile = ARRAY_ARRAY_TOFILE_METHODDEF
* array_array_tolist = ARRAY_ARRAY_TOLIST_METHODDEF
* array_array_tobytes = ARRAY_ARRAY_TOBYTES_METHODDEF
* array_array_tounicode = ARRAY_ARRAY_TOUNICODE_METHODDEF
* array_array___sizeof__ = ARRAY_ARRAY___SIZEOF___METHODDEF
* array_repr = Py_tp_repr
* array_richcompare = Py_tp_richcompare / CRITICAL2
* array_new = Py_tp_new
* array_length = Py_sq_length
* array_concat = Py_sq_concat / CRITICAL2
* array_repeat = Py_sq_repeat
* array_item = Py_sq_item
* array_ass_item = Py_sq_ass_item
* array_contains = Py_sq_contains
* array_inplace_repeat = Py_sq_inplace_repeat
* array_length = Py_mp_length
* array_subscr = Py_mp_subscript -> array_item
* array_ass_subscr = Py_mp_ass_subscript / CRITICAL2
* array_buffer_getbuf = Py_bf_getbuffer
* array_buffer_relbuf = Py_bf_releasebuffer
The following have not been modified but depend for safety on the functions they call:
+ array_array_append = ARRAY_ARRAY_APPEND_METHODDEF -> ins
+ array_array___copy__ = ARRAY_ARRAY___COPY___METHODDEF -> array_slice
+ array_array___deepcopy__ = ARRAY_ARRAY___DEEPCOPY___METHODDEF -> array_array___copy__
+ array_array_extend = ARRAY_ARRAY_EXTEND_METHODDEF -> array_do_extend / CRITICAL2
+ array_array_frombytes = ARRAY_ARRAY_FROMBYTES_METHODDEF -> frombytes / CRITICAL2
+ array_array_insert = ARRAY_ARRAY_INSERT_METHODDEF -> ins
+ array_array___reduce_ex__ = ARRAY_ARRAY___REDUCE_EX___METHODDEF -> array_array_tolist_impl, array_array_tobytes_impl
+ array_inplace_concat = Py_sq_inplace_concat -> array_do_extend / CRITICAL2
The following look safe as is:
- array_array_fromfile = ARRAY_ARRAY_FROMFILE_METHODDEF -> array_array_frombytes
- array_dealloc = Py_tp_dealloc
- array_iter = Py_tp_iter
- array_tp_traverse = Py_tp_traverse
And the following non-public utility functions have been made safe:
* array_slice
* array_do_extend / CRITICAL2
* ins
* frombytes / CRITICAL2
Here are a few questions needing guidance:
-
Changed clinic sig of array_array_frombytes() so I could lock the original source object during op (maybe its a writeable bytearray). Want to prevent modification of data during the operation but not sure this will even do this so not sure is necessary or even desired. Should I remove this and leave just the PyObject_GetBuffer() for protection? Also for this added a forward declaration of array_array_frombytes() for array_array_fromfile_impl() instead of moving stuff around for minimal impact.
-
In array_new there may or may not be an initializer object, if there is not then the args tuple is used as a stand-in for the critical section lock. Is there a better way to do this?
Misc:
-
array_richcompare was incrementing refcount of Py_True and Py_False, removed that as they are immortal.
-
The following were larger / slightly more complicated functions to check so should double check array_array_frombytes, array_richcompare, array_ass_subscr and array_new.
More work is still needed, like the array iterator and more testing and verification.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
|
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.
Thanks for doing this! At a glance, don't use goto with critical sections--it's messier and can be error-prone (IIRC there's problems with it compiling on MSVC too). Instead, create a wrapper function like this:
static PyObject *
array_something(arrayobject *self, PyObject *arg)
{
PyObject *res;
Py_BEGIN_CRITICAL_SECTION(self);
res = array_something_lock_held(); // the actual implementation function
Py_END_CRITICAL_SECTION();
return res;
}For functions that are defined with the Argument Clinic, you can use @critical_section to do this even faster. (See what I did for the ssl module.)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
tom-pytel
commented
Jan 17, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Made requested changes, also made arrayiter safe.
Also found a good old-fashioned non-free-threaded bug which probably goes back a long way - if try to arrayiter.__setstate__() when it is exhausted then segfault, fixed here obviously.
Question remaining about need to lock bytes source object in frombytes() or if holding the buffer is sufficient?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Also found a good old-fashioned non-free-threaded bug which probably goes back a long way - if try to arrayiter.__setstate__() when it is exhausted then segfault, fixed here obviously.
Could you separate that out from this PR and submit a dedicated PR for this bug? We might want to backport it.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Regarding the style nits: please make sure all new code (and in most cases, also changed code) follow PEP-7.
|
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.
|
Still would like to know if array_array_frombytes() really needs to lock the bytes object or if just holding the buffer is sufficient. The idea being trying to prevent the CONTENTS of the buffer changing during the operation. Just holding buffer is not enough if is writable but not sure locking object would help that anyway.
|
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.
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.
We're getting there! Now, we're overlocking things a bit. This isn't always the rule, but in general you only need to lock something if you want to avoid inconsistent state (making it a "critical section"!).
That, and keep in mind that things that have a _Py or Py prefix are generally thread safe themselves and will lock if they need to. You don't need to lock for every call to Python.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
❯ ./python -m test test_array -R 3:3
Using random seed: 1919100667
0:00:00 load avg: 36.22 Run 1 test sequentially in a single process
0:00:00 load avg: 36.22 [1/1] test_array
beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:456
XX. ...
0:00:16 load avg: 28.56 [1/1] test_array passed
== Tests result: SUCCESS ==
1 test OK.
Total duration: 16.1 sec
Total tests: run=886 skipped=1
Total test files: run=1/1
Result: SUCCESS No refleaks.
|
ZeroIntensity and tom-pytel reacted with hooray emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
ZeroIntensity reacted with heart emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
The PR regressed free threading performance substantially on the scimark benchmarks from pyperformance, so I'm going to revert the PR for now. We'll need to figure out a way of making arraymodule.c thread-safe without regressing perf.
|
tom-pytel and ZeroIntensity reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
colesbury
added a commit
to colesbury/cpython
that referenced
this pull request
Feb 28, 2025
…)"
The change regressed performance on scimark benchmarks from the
pyperformance benchmark suite.
This reverts commit 8ba0d7b.
The PR regressed free threading performance substantially on the scimark benchmarks from pyperformance, so I'm going to revert the PR for now. We'll need to figure out a way of making arraymodule.c thread-safe without regressing perf.
Will play with it checking perf along the way to see if a lesser standard of thread-safe can work (ignore data integrity and just make sure nothing crashes).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
colesbury
commented
Feb 28, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
I looked at it briefly under Linux's perf record and noticed a few things:
- The most important functions are
array_subscr, array_item, and array_ass_subscr in that order. Probably nothing else matters much.
- The critical section code is less efficient because it's in a shared library so there's a bunch of non-inlined calls to
_PyThreadState_GetCurrent().
We can consider using a similar strategy as PyListObject where we avoid locking during most read-only single-element accesses. It definitely will complicate the implementation, though.
We can also consider moving arraymodule.c out of a shared library. I'm not really sure what the considerations are for that.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
We can consider using a similar strategy as PyListObject where we avoid locking during most read-only single-element accesses. It definitely will complicate the implementation, though.
List deals with objects with their own PyObject headers, array is just a blob of memory so not sure how much of that strategy translates.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
colesbury
added a commit
that referenced
this pull request
Feb 28, 2025
The change regressed performance on scimark benchmarks from the
pyperformance benchmark suite.
This reverts commit 8ba0d7b.
|
I guess it'll be a bit simpler than PyListObject then. The primary thread-safety hazard is attempting to read or write an entry in ob_item while the arrayobject is concurrently resized and ob_item is freed and re-allocated.
So if we adapt the list strategy it might look like:
- Lock the
arrayobject during resize and multi-element accesses (e.g., array_subscr with slices)
- Mark the
arrayobject as shared if it's accessed by a non-owning thread (requires locking)
- When resizing the
arrayobject, use QSBR to delay the free of ob_item if the arrayobject is shared
- Single-element
array_subscr and array_ass_subscr can generally avoid locking
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
I guess it'll be a bit simpler than PyListObject then. The primary thread-safety hazard is attempting to read or write an entry in ob_item while the arrayobject is concurrently resized and ob_item is freed and re-allocated.
Simplest worst case scenario thing depending on performance could be a spinlock to make access to ob_item and ob_size shared atomic. Might lose integrity but keep speed and avoid crashes, put responsibility for the data validity on user in multithreaded scenarios.
In any case, got some crude numbers for impact of just array_item, array_subscr and ass_subscr by removing their locks to see rough best case scenario. For reference the order is: OLD array freethread main, NEW slow array freethread, NOLOCK slow array freethread without locking on array_item, array_subscr or array_ass_subscr:
### scimark_fft ###
OLD: Mean +- std dev: 238 ms +- 8 ms
NEW: Mean +- std dev: 282 ms +- 11 ms
NOLOCK: Mean +- std dev: 247 ms +- 4 ms
### scimark_lu ###
OLD: Mean +- std dev: 88.7 ms +- 3.9 ms
NEW: Mean +- std dev: 95.0 ms +- 1.5 ms
NOLOCK: Mean +- std dev: 88.5 ms +- 1.8 ms
### scimark_monte_carlo ###
OLD: Mean +- std dev: 53.4 ms +- 0.5 ms
NEW: Mean +- std dev: 56.0 ms +- 0.5 ms
NOLOCK: Mean +- std dev: 53.9 ms +- 0.9 ms
### scimark_sor ###
OLD: Mean +- std dev: 98.1 ms +- 1.7 ms
NEW: Mean +- std dev: 100 ms +- 1 ms
NOLOCK: Mean +- std dev: 97.7 ms +- 1.3 ms
### scimark_sparse_mat_mult ###
OLD: Mean +- std dev: 3.84 ms +- 0.14 ms
NEW: Mean +- std dev: 4.71 ms +- 0.07 ms
NOLOCK: Mean +- std dev: 3.87 ms +- 0.13 ms
I have all weekend to play with this so will try various things and what you mentioned.
|
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.
Reviewers
picnixz
picnixz left review comments
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