gh-158972: Drain each thread's page list during GC

Include/internal/pycore_pymem.h

@@ -129,6 +129,10 @@ extern void _PyMem_AbandonDelayed(PyThreadState *tstate);
 // On interpreter shutdown, frees all delayed free requests.
 extern void _PyMem_FiniDelayed(PyInterpreterState *interp);
 
+// Free every empty page parked on each thread's page_list that has reached its
+// QSBR goal
+extern void _PyMem_FreeParkedQSBRPages(PyInterpreterState *interp);
+
 #ifdef __cplusplus
 }
 #endif

Lib/test/test_free_threading/test_gc.py

@@ -10,6 +10,11 @@
 from test import support
 from test.support import threading_helper
 
+try:
+    import _testinternalcapi
+except ImportError:
+    _testinternalcapi = None
+
 
 class MyObj:
     pass
@@ -210,5 +215,85 @@ def reader():
             pass
 
 
+def allocate_and_free():
+    # Free enough small objects to leave many mimalloc pages empty. Empty
+    # pages in the object heaps wait on QSBR before they are freed.
+    junk = [[] for _ in range(100_000)]
+    del junk
+
+
+@threading_helper.requires_working_threading()
+@unittest.skipIf(_testinternalcapi is None, "requires _testinternalcapi")
+class TestQSBRPageList(TestCase):
+    # gh-158972: gc.collect() should free empty pages waiting on QSBR, even
+    # if the thread that parked them is idle.
+
+    def parked_pages(self):
+        return _testinternalcapi.get_qsbr_page_list_length()
+
+    def test_gc_collect_frees_parked_pages(self):
+        gc.collect()
+        allocate_and_free()
+        parked = self.parked_pages()
+        if parked < 10:
+            self.skipTest(f"only {parked} pages were parked")
+        gc.collect()
+        self.assertLess(self.parked_pages(), parked // 4)
+
+    def test_gc_collect_frees_pages_parked_by_idle_thread(self):
+        done = threading.Lock()
+        done.acquire()
+        resume = threading.Lock()
+        resume.acquire()
+
+        def worker():
+            allocate_and_free()
+            done.release()
+            # Block without allocating so the worker doesn't free its parked
+            # pages itself.
+            resume.acquire()
+
+        t = Thread(target=worker)
+        t.start()
+        try:
+            done.acquire()
+            parked = self.parked_pages()
+            if parked < 10:
+                self.skipTest(f"only {parked} pages were parked")
+            gc.collect()
+            self.assertLess(self.parked_pages(), parked // 4)
+        finally:
+            resume.release()
+            t.join()
+
+    def test_gc_collect_while_starting_threads(self):
+        # A new thread is added to the interpreter before it initializes its
+        # mimalloc state, so the GC can see thread states without a usable
+        # page list.
+        stop = threading.Event()
+
+        def collector():
+            while not stop.is_set():
+                gc.collect()
+
+        deadline = time.monotonic() + 2.0
+
+        def starter():
+            while time.monotonic() < deadline:
+                threads = [Thread(target=lambda: None) for _ in range(16)]
+                with threading_helper.start_threads(threads):
+                    pass
+
+        c = Thread(target=collector)
+        c.start()
+        try:
+            starters = [Thread(target=starter) for _ in range(4)]
+            with threading_helper.start_threads(starters):
+                pass
+        finally:
+            stop.set()
+            c.join()
+
+
 if __name__ == "__main__":
     unittest.main()

Modules/_testinternalcapi.c

@@ -2885,6 +2885,28 @@ get_long_lived_total(PyObject *self, PyObject *Py_UNUSED(ignored))
     return PyLong_FromInt64(PyInterpreterState_Get()->gc.long_lived_total);
 }
 
+// Return the total number of mimalloc pages on all threads' QSBR page lists.
+static PyObject *
+get_qsbr_page_list_length(PyObject *self, PyObject *Py_UNUSED(ignored))
+{
+    PyInterpreterState *interp = _PyInterpreterState_GET();
+    Py_ssize_t count = 0;
+    _PyEval_StopTheWorld(interp);
+    _Py_FOR_EACH_TSTATE_BEGIN(interp, p) {
+        struct _mimalloc_thread_state *m = &((_PyThreadStateImpl *)p)->mimalloc;
+        if (!_Py_atomic_load_int(&m->initialized)) {
+            continue;
+        }
+        struct llist_node *node;
+        llist_for_each(node, &m->page_list) {
+            count++;
+        }
+    }
+    _Py_FOR_EACH_TSTATE_END(interp);
+    _PyEval_StartTheWorld(interp);
+    return PyLong_FromSsize_t(count);
+}
+
 #endif
 
 static PyObject *
@@ -3442,6 +3464,7 @@ static PyMethodDef module_functions[] = {
     {"get_tlbc", get_tlbc, METH_O, NULL},
     {"get_tlbc_id", get_tlbc_id, METH_O, NULL},
     {"get_long_lived_total", get_long_lived_total, METH_NOARGS},
+    {"get_qsbr_page_list_length", get_qsbr_page_list_length, METH_NOARGS},
 #endif
 #ifdef _Py_TIER2
     {"uop_symbols_test", _Py_uop_symbols_test, METH_NOARGS},

Objects/obmalloc.c

@@ -226,16 +226,26 @@ _PyMem_mi_page_reclaimed(mi_page_t *page)
 #endif
 }
 
+#ifdef Py_GIL_DISABLED
+// Free the empty pages on `target`'s QSBR page list whose QSBR goal has been
+// reached.
+//
+// `cur` must be the calling thread's state: _Py_qsbr_poll() must be called
+// with the caller's own QSBR state. The goals themselves are global, so `cur`
+// can check pages parked by any thread.
+//
+// `target` is the thread whose page list is drained. It must be `cur`, or a
+// thread that is suspended (e.g., during a stop-the-world pause), because
+// _mi_page_free() modifies the heap and segment state of the thread that owns
+// the page.
 static void
-_PyMem_mi_heap_collect_qsbr(mi_heap_t *heap)
+_PyMem_mi_collect_tstate_page_list(_PyThreadStateImpl *cur, _PyThreadStateImpl *target)
 {
-#ifdef Py_GIL_DISABLED
-    if (!heap->page_use_qsbr) {
+    if (!_Py_atomic_load_int(&target->mimalloc.initialized)) {
+        // The thread may not have called tstate_mimalloc_bind() yet.
         return;
     }
-
-    _PyThreadStateImpl *tstate = (_PyThreadStateImpl *)_PyThreadState_GET();
-    struct llist_node *head = &tstate->mimalloc.page_list;
+    struct llist_node *head = &target->mimalloc.page_list;
     if (llist_empty(head)) {
         return;
     }
@@ -249,13 +259,26 @@ _PyMem_mi_heap_collect_qsbr(mi_heap_t *heap)
             continue;
         }
 
-        if (!_Py_qsbr_poll(tstate->qsbr, page->qsbr_goal)) {
+        if (!_Py_qsbr_poll(cur->qsbr, page->qsbr_goal)) {
             return;
         }
 
         _PyMem_mi_page_clear_qsbr(page);
         _mi_page_free(page, mi_page_queue_of(page), false);
     }
+}
+#endif
+
+static void
+_PyMem_mi_heap_collect_qsbr(mi_heap_t *heap)
+{
+#ifdef Py_GIL_DISABLED
+    if (!heap->page_use_qsbr) {
+        return;
+    }
+
+    _PyThreadStateImpl *tstate = (_PyThreadStateImpl *)_PyThreadState_GET();
+    _PyMem_mi_collect_tstate_page_list(tstate, tstate);
 #endif
 }
 
@@ -1702,6 +1725,19 @@ _PyMem_FiniDelayed(PyInterpreterState *interp)
     }
 }
 
+#ifdef Py_GIL_DISABLED
+void
+_PyMem_FreeParkedQSBRPages(PyInterpreterState *interp)
+{
+    _PyThreadStateImpl *tstate = (_PyThreadStateImpl *) _PyThreadState_GET();
+    _Py_FOR_EACH_TSTATE_BEGIN(interp, p) {
+        _PyMem_mi_collect_tstate_page_list(tstate, (_PyThreadStateImpl *) p);
+    }
+    _Py_FOR_EACH_TSTATE_END(interp);
+}
+#endif
+
+
 /**************************/
 /* the "object" allocator */
 /**************************/

Python/gc_free_threading.c

@@ -2110,6 +2110,8 @@ gc_collect_internal(PyInterpreterState *interp, struct collection_state *state,
 
     process_delayed_frees(interp, state);
 
+    _PyMem_FreeParkedQSBRPages(interp);
+
     #ifdef GC_ENABLE_MARK_ALIVE
     // If gc.freeze() was used, it seems likely that doing this "mark alive"
     // pass will not be a performance win.  Typically the majority of alive