gh-128942: make arraymodule.c free-thread safe (lock-free) #130771
Conversation
|
ping @colesbury |
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).
|
Note, this is not ready to go, there is the memory issue which needs resolving. |
|
@ZeroIntensity you can remove the do-not-merge, its not an |
|
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. |
|
The actual
Are there any other places where this needs to take place? Its the test and trying to run it with
Which is not Left the bad |
|
I'd like
Yes, |
|
Excessive QSBR memory usage: I ran across this while profiling memory usage here. The results are similar for both array and list objects which use QSBR to free memory, so this is a QSBR thing. Memory usage numbers for script (provided below) using both array and list ( Script: from queue import Queue
def thrdfunc(queue):
while True:
l = queue.get()
l.append(0) # force resize in non-parent thread which will free using _PyMem_ProcessDelayed()
queue = Queue(maxsize=2)
threading.Thread(target=thrdfunc, args=(queue,)).start()
while True:
l = array('i', [0] * int(3840*2160*3/4)) # using int instead of byte for reasons
# l = [None] * int(3840*2160*3/8) # sys.getsizeof(l) ~= 3840*2160*3 bytes
queue.put(l)Since delayed memory free checks (and subsequent frees if applicable) occur in one of two situations:
This works great for many small objects, but with larger buffers these can accumulate quickly. I tried a few things but the diff --git a/Python/pystate.c b/Python/pystate.c
index ee35f0fa945..d9d731a15bc 100644
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -2169,6 +2169,9 @@ _PyThreadState_Attach(PyThreadState *tstate)
#if defined(Py_DEBUG)
errno = err;
#endif
+#ifdef Py_GIL_DISABLED
+ _PyMem_ProcessDelayed(tstate);
+#endif
}
static voidNot saying it is THE solution, but at the very least it shows memory usage can be reduced with no hit to performance (timed that with Another option would be to add another check in Thoughts? |
|
Ping @colesbury, is this still a valid PR or should I close it? |
|
It's valid, just big. |
|
!buildbot Usan.PR |
|
🤖 New build scheduled with the buildbot fleet by @encukou for commit 1a50981 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130771%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
|
!buildbot Usan.PR |
|
You don't have write permissions to trigger a build |
|
!buildbot Usan.PR |
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit f023e77 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130771%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
I added lock-free single element reads and writes by mostly copying the
listobject'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".
arraymodule is not free-thread safe. #128942