gh-127716: make memoryview thread-safe in free-threaded build - #156248
gh-127716: make memoryview thread-safe in free-threaded build#156248nascheme wants to merge 7 commits into
Conversation
Documentation build overview
21 files changed ·
|
Co-authored-by: ayaangazali <ayaangazali.work@gmail.com> Co-authored-by: Lu Xiaowei <weixlu420302@gmail.com>
4269e2d to
8fa2daf
Compare
* The CHECK_* macros can return. Restructure code so that unpin is always done for toreadonly(). * Use atomic load for asserts on `exports`.
Speed it up (less iterations, rounds and threads). Rename confusing assert_exporter_free() method to ensure_exporter_free().
…yview-thread-safe
* Add comment to CAS while loop, yielding is unnecessary. * Use "relaxed" memory ordering, seq-cst is overkill.
…yview-thread-safe
We only need the "while" loop if we are setting multiple flag bits concurrently. That's not needed, we only set RELEASED. So, restructure code, no while loop needed and someone can't accidently call the function in the wrong way.
|
@nascheme What do you think about making the iterator hold exports for its whole lifetime, instead of pinning in |
|
Increasing performance would be nice but that would change the behaviour of |
I mean
cpython/Objects/memoryobject.c Lines 1253 to 1263 in e3c9012 What I'm thinking (quick Sol 5.6-assisted proof of concept patch to demonstrate the idea): diff --git a/Objects/memoryobject.c b/Objects/memoryobject.c
index 2d8537ee57..b66f3b9153 100644
--- a/Objects/memoryobject.c
+++ b/Objects/memoryobject.c
@@ -3946,6 +3946,11 @@ memoryiter_dealloc(PyObject *self)
{
memoryiterobject *it = (memoryiterobject *)self;
_PyObject_GC_UNTRACK(it);
+#ifdef Py_GIL_DISABLED
+ if (it->it_seq != NULL) {
+ mbuf_drop_export(it->it_seq->mbuf);
+ }
+#endif
Py_XDECREF(it->it_seq);
PyObject_GC_Del(it);
}
@@ -3969,33 +3974,28 @@ memoryiter_next(PyObject *self)
if (it->it_index >= it->it_length) {
/* Preserve StopIteration for exhausted iterators over relea */
+#ifndef Py_GIL_DISABLED
it->it_seq = NULL;
Py_DECREF(seq);
+#endif
return NULL;
}
-#ifdef Py_GIL_DISABLED
- /* Keep seq alive if another thread exhausts the iterator. */
- Py_INCREF(seq);
-#endif
+ CHECK_RELEASED(seq);
- PyObject *result = NULL;
- if (memoryview_pin(seq) == 0) {
- Py_buffer *view = &seq->view;
- char *ptr = (char *)view->buf;
+ Py_buffer *view = &seq->view;
+ char *ptr = (char *)view->buf;
- ptr += view->strides[0] * it->it_index++;
- ptr = ADJUST_PTR(ptr, view->suboffsets, 0);
- if (ptr != NULL) {
- result = unpack_single(seq, ptr, it->it_fmt);
- }
- memoryview_unpin(seq);
+ ptr += view->strides[0] * it->it_index++;
+ ptr = ADJUST_PTR(ptr, view->suboffsets, 0);
+ if (ptr == NULL) {
+ return NULL;
}
-
-#ifdef Py_GIL_DISABLED
- Py_DECREF(seq);
-#endif
- return result;
+ return unpack_single(seq, ptr, it->it_fmt);
}
static PyObject *
@@ -4032,6 +4032,11 @@ memory_iter_pinned(PyObject *seq)
it->it_length = memory_length((PyObject *)obj);
it->it_index = 0;
it->it_seq = (PyMemoryViewObject*)Py_NewRef(obj);
+#ifdef Py_GIL_DISABLED
+ FT_ATOMIC_ADD_SSIZE(obj->mbuf->exports, 1);
+#endif
_PyObject_GC_TRACK(it);
return (PyObject *)it;
}The downside is that one won't be able to resize a ba = bytearray(128); m = memoryview(ba); it = iter(m)
m.release()
ba.append(1) # would raise |
4 participants
Note: this PR was developed with assistance from LLMs, Claude Opus/Fable and GPT 5.6 Sol.
This incorporates changes from gh-155882 and gh-154770. It goes further and ensures the underlying buffer remains "pinned" if a thread releases the memory view while other threads have in-flight operations on it.
Performance testing results are below. The goal is negligible slowdown for the GIL-enabled build (achieved). Also, we want negligible slowdown in the un-contended free-threaded case (also achieved). Unfortunately, multi-threaded scaling with a shared
memoryviewobject is terrible.I explored various ways of improving the scaling (QSBR, RCU, hazard pointers) but I think other options are too complex in terms of implementation or have other critical problems. If you want multiple threads operating on the same underlying "mbuf", a good way to do it is to create a thread-local
memoryviewcopy. You need to do this from the orginal "exporter" object, not the existingmemoryviewobject. I have an additional change that adds a.reexport()method that does this, which is probably handly.Single-thread performance
Times are nanoseconds per operation (lower is better), minus loop overhead; iteration is
nanoseconds per element. "base" is before this PR and "patched" is after.
Results from Intel i7-14700K
getitemsetitemformatslicecasttobytes_16memoryview_of_viewstruct.unpack_fromBytesIO.readintoBytesIO.writeResults from Macbook M3 Pro.
getitemsetitemformatslicecasttobytes_16memoryview_of_viewunpack_frombytesio_writebytesio_readintoFree-threaded scaling
Throughput is millions of operations per second (higher is better). For iteration, one yielded
element is one operation. Base has no
reexport(), so that workload has apatched row only; compare it against the
separate views of one exporterrows.Results from Intel i7-14700K
mv[0]mv[0]mv[0]reexport()views,mv[0]mv[0]tobytes()4 KiBstruct.unpack_fromResults from Macbook M3 Pro. Note that this CPU has 6 P-cores and 6 E-cores.
mv[0]mv[0]mv[0]reexport()views,mv[0]mv[0]tobytes()4 KiBstruct.unpack_fromAnything that shares one managed buffer contends (shared
mbuf->exportscache line).