From 9ff8d520e624096864ca296e88b5fde84f88b0b6 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 12 Apr 2024 17:06:38 +0000 Subject: [PATCH] gh-117783: Immortalize objects that use deferred reference counting Deferred reference counting is not fully implemented yet. As a temporary measure, we immortalize objects that would use deferred reference counting to avoid multi-threaded scaling bottlenecks. This is only performed in the free-threaded build once the first non-main thread is started. Additionally, some tests, including refleak tests, suppress this behavior. --- Include/internal/pycore_gc.h | 17 +++++++++++++++++ Lib/test/libregrtest/main.py | 8 ++++++-- Lib/test/libregrtest/single.py | 5 ++++- Lib/test/support/__init__.py | 19 +++++++++++++++++++ Lib/test/test_capi/test_watchers.py | 6 +++++- Lib/test/test_code.py | 6 +++++- Lib/test/test_functools.py | 1 + Lib/test/test_weakref.py | 5 ++++- Modules/_testinternalcapi.c | 22 ++++++++++++++++++++++ Objects/object.c | 7 +++++++ Python/gc_free_threading.c | 28 ++++++++++++++++++++++++++++ Python/pylifecycle.c | 17 +++++++++++++++++ Python/pystate.c | 9 +++++++++ 13 files changed, 144 insertions(+), 6 deletions(-) diff --git a/Include/internal/pycore_gc.h b/Include/internal/pycore_gc.h index 60020b5c01f8a6c..dfc68965df65949 100644 --- a/Include/internal/pycore_gc.h +++ b/Include/internal/pycore_gc.h @@ -312,6 +312,18 @@ struct _gc_runtime_state { collections, and are awaiting to undergo a full collection for the first time. */ Py_ssize_t long_lived_pending; + + /* gh-117783: Deferred reference counting is not fully implemented yet, so + as a temporary measure we treat objects using deferred referenence + counting as immortal. */ + struct { + /* Immortalize objects instead of marking them as using deferred + reference counting. */ + int enabled; + + /* Set enabled=1 when the first background thread is created. */ + int enable_on_thread; + } immortalize; #endif }; @@ -343,6 +355,11 @@ extern void _PyGC_ClearAllFreeLists(PyInterpreterState *interp); extern void _Py_ScheduleGC(PyThreadState *tstate); extern void _Py_RunGC(PyThreadState *tstate); +#ifdef Py_GIL_DISABLED +// gh-117783: Immortalize objects that use deferred reference counting +extern void _PyGC_ImmortalizeDeferredObjects(PyInterpreterState *interp); +#endif + #ifdef __cplusplus } #endif diff --git a/Lib/test/libregrtest/main.py b/Lib/test/libregrtest/main.py index 3c9d9620053355a..9e7a7d608800914 100644 --- a/Lib/test/libregrtest/main.py +++ b/Lib/test/libregrtest/main.py @@ -7,7 +7,8 @@ import time import trace -from test.support import os_helper, MS_WINDOWS, flush_std_streams +from test.support import (os_helper, MS_WINDOWS, flush_std_streams, + suppress_immortalization) from .cmdline import _parse_args, Namespace from .findtests import findtests, split_test_packages, list_cases @@ -526,7 +527,10 @@ def _run_tests(self, selected: TestTuple, tests: TestList | None) -> int: if self.num_workers: self._run_tests_mp(runtests, self.num_workers) else: - self.run_tests_sequentially(runtests) + # gh-117783: don't immortalize deferred objects when tracking + # refleaks. Only releveant for the free-threaded build. + with suppress_immortalization(runtests.hunt_refleak): + self.run_tests_sequentially(runtests) coverage = self.results.get_coverage_results() self.display_result(runtests) diff --git a/Lib/test/libregrtest/single.py b/Lib/test/libregrtest/single.py index 235029d8620ff5b..fc2f2716ad4ce05 100644 --- a/Lib/test/libregrtest/single.py +++ b/Lib/test/libregrtest/single.py @@ -303,7 +303,10 @@ def run_single_test(test_name: TestName, runtests: RunTests) -> TestResult: result = TestResult(test_name) pgo = runtests.pgo try: - _runtest(result, runtests) + # gh-117783: don't immortalize deferred objects when tracking + # refleaks. Only releveant for the free-threaded build. + with support.suppress_immortalization(runtests.hunt_refleak): + _runtest(result, runtests) except: if not pgo: msg = traceback.format_exc() diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index be3f93ab2e5fd1a..83ed7aa5ae43bbe 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -515,6 +515,25 @@ def has_no_debug_ranges(): def requires_debug_ranges(reason='requires co_positions / debug_ranges'): return unittest.skipIf(has_no_debug_ranges(), reason) +@contextlib.contextmanager +def suppress_immortalization(suppress=True): + """Suppress immortalization of deferred objects.""" + try: + import _testinternalcapi + except ImportError: + yield + return + + if not suppress: + yield + return + + old_values = _testinternalcapi.set_immortalize_deferred(False) + try: + yield + finally: + _testinternalcapi.set_immortalize_deferred(*old_values) + MS_WINDOWS = (sys.platform == 'win32') # Is not actually used in tests, but is kept for compatibility. diff --git a/Lib/test/test_capi/test_watchers.py b/Lib/test/test_capi/test_watchers.py index 8e84d0077c7573a..90665a7561b3162 100644 --- a/Lib/test/test_capi/test_watchers.py +++ b/Lib/test/test_capi/test_watchers.py @@ -1,7 +1,9 @@ import unittest from contextlib import contextmanager, ExitStack -from test.support import catch_unraisable_exception, import_helper, gc_collect +from test.support import ( + catch_unraisable_exception, import_helper, + gc_collect, suppress_immortalization) # Skip this test if the _testcapi module isn't available. @@ -382,6 +384,7 @@ def assert_event_counts(self, exp_created_0, exp_destroyed_0, self.assertEqual( exp_destroyed_1, _testcapi.get_code_watcher_num_destroyed_events(1)) + @suppress_immortalization() def test_code_object_events_dispatched(self): # verify that all counts are zero before any watchers are registered self.assert_event_counts(0, 0, 0, 0) @@ -428,6 +431,7 @@ def test_error(self): self.assertIsNone(cm.unraisable.object) self.assertEqual(str(cm.unraisable.exc_value), "boom!") + @suppress_immortalization() def test_dealloc_error(self): co = _testcapi.code_newempty("test_watchers", "dummy0", 0) with self.code_watcher(2): diff --git a/Lib/test/test_code.py b/Lib/test/test_code.py index fe8c672e71a7b55..aa793f562253930 100644 --- a/Lib/test/test_code.py +++ b/Lib/test/test_code.py @@ -141,7 +141,8 @@ ctypes = None from test.support import (cpython_only, check_impl_detail, requires_debug_ranges, - gc_collect, Py_GIL_DISABLED) + gc_collect, Py_GIL_DISABLED, + suppress_immortalization) from test.support.script_helper import assert_python_ok from test.support import threading_helper, import_helper from test.support.bytecode_helper import instructions_with_positions @@ -577,6 +578,7 @@ def test_interned_string_with_null(self): class CodeWeakRefTest(unittest.TestCase): + @suppress_immortalization() def test_basic(self): # Create a code object in a clean environment so that we know we have # the only reference to it left. @@ -827,6 +829,7 @@ def test_bad_index(self): self.assertEqual(GetExtra(f.__code__, FREE_INDEX+100, ctypes.c_voidp(100)), 0) + @suppress_immortalization() def test_free_called(self): # Verify that the provided free function gets invoked # when the code object is cleaned up. @@ -854,6 +857,7 @@ def test_get_set(self): del f @threading_helper.requires_working_threading() + @suppress_immortalization() def test_free_different_thread(self): # Freeing a code object on a different thread then # where the co_extra was set should be safe. diff --git a/Lib/test/test_functools.py b/Lib/test/test_functools.py index ec5f6af5e17842c..bb4c7cc8701fb42 100644 --- a/Lib/test/test_functools.py +++ b/Lib/test/test_functools.py @@ -1833,6 +1833,7 @@ def f(): return 1 self.assertEqual(f.cache_parameters(), {'maxsize': 1000, "typed": True}) + @support.suppress_immortalization() def test_lru_cache_weakrefable(self): @self.module.lru_cache def test_function(x): diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py index 499ba77fd19542d..fa35209e34ae247 100644 --- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -12,7 +12,7 @@ import random from test import support -from test.support import script_helper, ALWAYS_EQ +from test.support import script_helper, ALWAYS_EQ, suppress_immortalization from test.support import gc_collect from test.support import import_helper from test.support import threading_helper @@ -650,6 +650,7 @@ class C(object): # deallocation of c2. del c2 + @suppress_immortalization() def test_callback_in_cycle(self): import gc @@ -742,6 +743,7 @@ class D: del c1, c2, C, D gc.collect() + @suppress_immortalization() def test_callback_in_cycle_resurrection(self): import gc @@ -877,6 +879,7 @@ def test_init(self): # No exception should be raised here gc.collect() + @suppress_immortalization() def test_classes(self): # Check that classes are weakrefable. class A(object): diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index cc9e1403f87ecd0..24ffa412e283570 100644 --- a/Modules/_testinternalcapi.c +++ b/Modules/_testinternalcapi.c @@ -1936,6 +1936,27 @@ get_py_thread_id(PyObject *self, PyObject *Py_UNUSED(ignored)) } #endif +static PyObject * +set_immortalize_deferred(PyObject *self, PyObject *value) +{ +#ifdef Py_GIL_DISABLED + PyInterpreterState *interp = PyInterpreterState_Get(); + int old_enabled = interp->gc.immortalize.enabled; + int old_enabled_on_thread = interp->gc.immortalize.enable_on_thread; + int enabled_on_thread = 0; + if (!PyArg_ParseTuple(value, "i|i", + &interp->gc.immortalize.enabled, + &enabled_on_thread)) + { + return NULL; + } + interp->gc.immortalize.enable_on_thread = enabled_on_thread; + return Py_BuildValue("ii", old_enabled, old_enabled_on_thread); +#else + Py_RETURN_FALSE; +#endif +} + static PyObject * has_inline_values(PyObject *self, PyObject *obj) { @@ -2029,6 +2050,7 @@ static PyMethodDef module_functions[] = { #ifdef Py_GIL_DISABLED {"py_thread_id", get_py_thread_id, METH_NOARGS}, #endif + {"set_immortalize_deferred", set_immortalize_deferred, METH_VARARGS}, {"uop_symbols_test", _Py_uop_symbols_test, METH_NOARGS}, {NULL, NULL} /* sentinel */ }; diff --git a/Objects/object.c b/Objects/object.c index 214e7c5b567928f..47bc6be71aaa9d6 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2433,6 +2433,13 @@ _PyObject_SetDeferredRefcount(PyObject *op) assert(PyType_IS_GC(Py_TYPE(op))); assert(_Py_IsOwnedByCurrentThread(op)); assert(op->ob_ref_shared == 0); + PyInterpreterState *interp = _PyInterpreterState_GET(); + if (interp->gc.immortalize.enabled) { + // gh-117696: immortalize objects instead of using deferred reference + // counting for now. + _Py_SetImmortal(op); + return; + } op->ob_gc_bits |= _PyGC_BITS_DEFERRED; op->ob_ref_local += 1; op->ob_ref_shared = _Py_REF_QUEUED; diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 9cf0e989d0993f5..58632036812572f 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -704,6 +704,10 @@ _PyGC_Init(PyInterpreterState *interp) { GCState *gcstate = &interp->gc; + if (_Py_IsMainInterpreter(interp)) { + gcstate->immortalize.enable_on_thread = 1; + } + gcstate->garbage = PyList_New(0); if (gcstate->garbage == NULL) { return _PyStatus_NO_MEMORY(); @@ -1781,6 +1785,30 @@ custom_visitor_wrapper(const mi_heap_t *heap, const mi_heap_area_t *area, return true; } +// gh-117783: Immortalize objects that use deferred reference counting to +// temporarily work around scaling bottlenecks. +static bool +immortalize_visitor(const mi_heap_t *heap, const mi_heap_area_t *area, + void *block, size_t block_size, void *args) +{ + PyObject *op = op_from_block(block, args, false); + if (op != NULL && _PyObject_HasDeferredRefcount(op)) { + _Py_SetImmortal(op); + op->ob_gc_bits &= ~_PyGC_BITS_DEFERRED; + } + return true; +} + +void +_PyGC_ImmortalizeDeferredObjects(PyInterpreterState *interp) +{ + struct visitor_args args; + _PyEval_StopTheWorld(interp); + gc_visit_heaps(interp, &immortalize_visitor, &args); + interp->gc.immortalize.enabled = 1; + _PyEval_StartTheWorld(interp); +} + void PyUnstable_GC_VisitObjects(gcvisitobjects_t callback, void *arg) { diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index cc1824634e7a7f5..7b923e946fcc961 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1491,6 +1491,22 @@ finalize_modules_delete_special(PyThreadState *tstate, int verbose) } } +static void +swap_module_dict(PyModuleObject *mod) +{ + if (_Py_IsImmortal(mod->md_dict)) { + // gh-117783: Immortalizing module dicts can cause some finalizers to + // run much later than typical leading to attribute errors due to + // partially cleared modules. To avoid this, we copy the module dict + // if it was immortalized. + PyObject *copy = PyDict_Copy(mod->md_dict); + if (copy == NULL) { + PyErr_FormatUnraisable("Exception ignored on removing modules"); + return; + } + Py_SETREF(mod->md_dict, copy); + } +} static PyObject* finalize_remove_modules(PyObject *modules, int verbose) @@ -1521,6 +1537,7 @@ finalize_remove_modules(PyObject *modules, int verbose) if (verbose && PyUnicode_Check(name)) { \ PySys_FormatStderr("# cleanup[2] removing %U\n", name); \ } \ + swap_module_dict((PyModuleObject *)mod); \ STORE_MODULE_WEAKREF(name, mod); \ if (PyObject_SetItem(modules, name, Py_None) < 0) { \ PyErr_FormatUnraisable("Exception ignored on removing modules"); \ diff --git a/Python/pystate.c b/Python/pystate.c index 37480df88aeb726..8172fc449af6df3 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1567,6 +1567,15 @@ new_threadstate(PyInterpreterState *interp, int whence) // Must be called with lock unlocked to avoid re-entrancy deadlock. PyMem_RawFree(new_tstate); } + else { +#ifdef Py_GIL_DISABLED + if (interp->gc.immortalize.enable_on_thread && !interp->gc.immortalize.enabled) { + // Immortalize objects marked as using deferred reference counting + // the first time a non-main thread is created. + _PyGC_ImmortalizeDeferredObjects(interp); + } +#endif + } #ifdef Py_GIL_DISABLED // Must be called with lock unlocked to avoid lock ordering deadlocks.