GeezerVison

Documentation build overview

📚 cpython-previews | 🛠️ Build #35545131 | 📁 Comparing 8833436 against main (b062727)

  🔍 Preview build  

Hi, thanks for the thorough fix! I read the code and it looks like all operations of the counters are well handled, so I'll close my PR.

Also, I checkout your branch and run the reproducers from #127716 and #155606. Both issues appear to be resolved.

@nascheme What do you think about making the iterator hold exports for its whole lifetime, instead of pinning in memoryiter_next()? I believe this should remove the performance penalty, at the expense of granularity.

Increasing performance would be nice but that would change the behaviour of memoryview.release(). I.e. you couldn't call it as long as an iterator is alive. So, I think we should not make that change.

Increasing performance would be nice but that would change the behaviour of memoryview.release(). I.e. you couldn't call it as long as an iterator is alive. So, I think we should not make that change.

I mean mbuf->exports, not self->exports. Sorry for not being clear.

memoryview.release() checks self->exports:

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 bytearray while iter still exists:

ba = bytearray(128); m = memoryview(ba); it = iter(m)
m.release()
ba.append(1)  # would raise

I mean mbuf->exports, not self->exports.

I see. That still changes the release semantics as your example shows. My guess is that iterating over the memoryview is likely not more common than using getitem or slicing it. So, I feel like this optimization is not worth the (small) code complexity it adds and the change in semantics.

Hi! Granted I'm not familiar with python's policies for cherrypicking fixes for release, I was wondering whether you expected this fix in a specific future python 3.{14,15,16}.x release? Many thanks!

I think there is a version of this PR that uses hazard pointers and scales better under shared reads. However, it would be more complicated and I think it's possible to move to that approach in the future, in a manner than composes nicely with this. So I think we shouldn't delay this on a version that scales better: it can always be done in the future if reports come in about multithreaded performance. Or perhaps later in the 3.16 release cycle.

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 8833436 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F156248%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.