diff --git a/Include/internal/pycore_pymem.h b/Include/internal/pycore_pymem.h index 05484e847f1195..8f5e4b64960ea8 100644 --- a/Include/internal/pycore_pymem.h +++ b/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 diff --git a/Lib/test/test_free_threading/test_gc.py b/Lib/test/test_free_threading/test_gc.py index 30282a111345f8..d4d53600987652 100644 --- a/Lib/test/test_free_threading/test_gc.py +++ b/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() diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index 479297a996c250..279d0f34b72508 100644 --- a/Modules/_testinternalcapi.c +++ b/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}, diff --git a/Objects/obmalloc.c b/Objects/obmalloc.c index 502c3eea6806cf..7a60252c7ad543 100644 --- a/Objects/obmalloc.c +++ b/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 */ /**************************/ diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index e8ed866312c94b..2dc8d222b06b72 100644 --- a/Python/gc_free_threading.c +++ b/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