From 723d4d2fe8e77b398f0ccffcfa541149caaac6a1 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 6 May 2024 20:12:39 -0400 Subject: [PATCH] gh-118527: Intern code consts in free-threaded build (#118667) We already intern and immortalize most string constants. In the free-threaded build, other constants can be a source of reference count contention because they are shared by all threads running the same code objects. --- Include/internal/pycore_code.h | 10 + Include/internal/pycore_interp.h | 1 + Include/internal/pycore_setobject.h | 3 + Lib/test/support/__init__.py | 9 + Lib/test/test_code.py | 23 +- Lib/test/test_ctypes/test_internals.py | 2 +- Lib/test/test_ctypes/test_python_api.py | 2 +- Lib/test/test_memoryio.py | 2 +- Modules/_testinternalcapi.c | 12 + Objects/codeobject.c | 302 ++++++++++++++++++++++-- Objects/setobject.c | 6 + Python/bltinmodule.c | 13 + Python/pylifecycle.c | 7 + Python/pystate.c | 1 + 14 files changed, 375 insertions(+), 18 deletions(-) diff --git a/Include/internal/pycore_code.h b/Include/internal/pycore_code.h index 8d832c59e36..bcbaf60f226 100644 --- a/Include/internal/pycore_code.h +++ b/Include/internal/pycore_code.h @@ -8,6 +8,8 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif +#include "pycore_lock.h" // PyMutex + // We hide some of the newer PyCodeObject fields behind macros. // This helps with backporting certain changes to 3.12. @@ -16,6 +18,14 @@ extern "C" { #define _PyCode_HAS_INSTRUMENTATION(CODE) \ (CODE->_co_instrumentation_version > 0) +struct _py_code_state { + PyMutex mutex; + // Interned constants from code objects. Used by the free-threaded build. + struct _Py_hashtable_t *constants; +}; + +extern PyStatus _PyCode_Init(PyInterpreterState *interp); +extern void _PyCode_Fini(PyInterpreterState *interp); #define CODE_MAX_WATCHERS 8 diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index a2aec6dd4b7..86dada5061e 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -245,6 +245,7 @@ struct _is { struct _Py_long_state long_state; struct _dtoa_state dtoa; struct _py_func_state func_state; + struct _py_code_state code_state; struct _Py_dict_state dict_state; struct _Py_exc_state exc_state; diff --git a/Include/internal/pycore_setobject.h b/Include/internal/pycore_setobject.h index 41b351ead25..0494c07fe18 100644 --- a/Include/internal/pycore_setobject.h +++ b/Include/internal/pycore_setobject.h @@ -30,6 +30,9 @@ PyAPI_DATA(PyObject *) _PySet_Dummy; PyAPI_FUNC(int) _PySet_Contains(PySetObject *so, PyObject *key); +// Clears the set without acquiring locks. Used by _PyCode_Fini. +extern void _PySet_ClearInternal(PySetObject *so); + #ifdef __cplusplus } #endif diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py index 999fffb03ed..e2a854663dd 100644 --- a/Lib/test/support/__init__.py +++ b/Lib/test/support/__init__.py @@ -535,6 +535,15 @@ def suppress_immortalization(suppress=True): finally: _testinternalcapi.set_immortalize_deferred(*old_values) +def skip_if_suppress_immortalization(): + try: + import _testinternalcapi + except ImportError: + return + return unittest.skipUnless(_testinternalcapi.get_immortalize_deferred(), + "requires immortalization of deferred objects") + + MS_WINDOWS = (sys.platform == 'win32') # Is not actually used in tests, but is kept for compatibility. diff --git a/Lib/test/test_code.py b/Lib/test/test_code.py index aa793f56225..059fab1e2ec 100644 --- a/Lib/test/test_code.py +++ b/Lib/test/test_code.py @@ -142,7 +142,8 @@ except ImportError: from test.support import (cpython_only, check_impl_detail, requires_debug_ranges, gc_collect, Py_GIL_DISABLED, - suppress_immortalization) + suppress_immortalization, + skip_if_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 @@ -570,11 +571,31 @@ class CodeConstsTest(unittest.TestCase): self.assertIsInterned(f()) @cpython_only + @unittest.skipIf(Py_GIL_DISABLED, "free-threaded build interns all string constants") def test_interned_string_with_null(self): co = compile(r'res = "str\0value!"', '?', 'exec') v = self.find_const(co.co_consts, 'str\0value!') self.assertIsNotInterned(v) + @cpython_only + @unittest.skipUnless(Py_GIL_DISABLED, "does not intern all constants") + @skip_if_suppress_immortalization() + def test_interned_constants(self): + # compile separately to avoid compile time de-duping + + globals = {} + exec(textwrap.dedent(""" + def func1(): + return (0.0, (1, 2, "hello")) + """), globals) + + exec(textwrap.dedent(""" + def func2(): + return (0.0, (1, 2, "hello")) + """), globals) + + self.assertTrue(globals["func1"]() is globals["func2"]()) + class CodeWeakRefTest(unittest.TestCase): diff --git a/Lib/test/test_ctypes/test_internals.py b/Lib/test/test_ctypes/test_internals.py index 94c9a86c2d0..778da6573da 100644 --- a/Lib/test/test_ctypes/test_internals.py +++ b/Lib/test/test_ctypes/test_internals.py @@ -28,7 +28,7 @@ class ObjectsTestCase(unittest.TestCase): self.assertEqual(ci._objects, None) def test_c_char_p(self): - s = b"Hello, World" + s = "Hello, World".encode("ascii") refcnt = sys.getrefcount(s) cs = c_char_p(s) self.assertEqual(refcnt + 1, sys.getrefcount(s)) diff --git a/Lib/test/test_ctypes/test_python_api.py b/Lib/test/test_ctypes/test_python_api.py index 77da3585592..1072a109833 100644 --- a/Lib/test/test_ctypes/test_python_api.py +++ b/Lib/test/test_ctypes/test_python_api.py @@ -47,7 +47,7 @@ class PythonAPITestCase(unittest.TestCase): @support.refcount_test def test_PyObj_FromPtr(self): - s = "abc def ghi jkl" + s = object() ref = sys.getrefcount(s) # id(python-object) is the address pyobj = _ctypes.PyObj_FromPtr(id(s)) diff --git a/Lib/test/test_memoryio.py b/Lib/test/test_memoryio.py index 8192502a407..95629ed862d 100644 --- a/Lib/test/test_memoryio.py +++ b/Lib/test/test_memoryio.py @@ -801,7 +801,7 @@ class CBytesIOTest(PyBytesIOTest): def _test_cow_mutation(self, mutation): # Common code for all BytesIO copy-on-write mutation tests. - imm = b' ' * 1024 + imm = (' ' * 1024).encode("ascii") old_rc = sys.getrefcount(imm) memio = self.ioclass(imm) self.assertEqual(sys.getrefcount(imm), old_rc + 1) diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index de98af32b5d..e209c7e0526 100644 --- a/Modules/_testinternalcapi.c +++ b/Modules/_testinternalcapi.c @@ -1985,6 +1985,17 @@ set_immortalize_deferred(PyObject *self, PyObject *value) #endif } +static PyObject * +get_immortalize_deferred(PyObject *self, PyObject *Py_UNUSED(ignored)) +{ +#ifdef Py_GIL_DISABLED + PyInterpreterState *interp = PyInterpreterState_Get(); + return PyBool_FromLong(interp->gc.immortalize.enable_on_thread_created); +#else + Py_RETURN_FALSE; +#endif +} + static PyObject * has_inline_values(PyObject *self, PyObject *obj) { @@ -2081,6 +2092,7 @@ static PyMethodDef module_functions[] = { {"py_thread_id", get_py_thread_id, METH_NOARGS}, #endif {"set_immortalize_deferred", set_immortalize_deferred, METH_VARARGS}, + {"get_immortalize_deferred", get_immortalize_deferred, METH_NOARGS}, #ifdef _Py_TIER2 {"uop_symbols_test", _Py_uop_symbols_test, METH_NOARGS}, #endif diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 2ce5f7dca2e..3d804f73a54 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -5,6 +5,8 @@ #include "pycore_code.h" // _PyCodeConstructor #include "pycore_frame.h" // FRAME_SPECIALS_SIZE +#include "pycore_hashtable.h" // _Py_hashtable_t +#include "pycore_initconfig.h" // _PyStatus_OK() #include "pycore_interp.h" // PyInterpreterState.co_extra_freefuncs #include "pycore_object.h" // _PyObject_SetDeferredRefcount #include "pycore_opcode_metadata.h" // _PyOpcode_Deopt, _PyOpcode_Caches @@ -100,10 +102,20 @@ PyCode_ClearWatcher(int watcher_id) * generic helpers ******************/ -/* all_name_chars(s): true iff s matches [a-zA-Z0-9_]* */ static int -all_name_chars(PyObject *o) +should_intern_string(PyObject *o) { +#ifdef Py_GIL_DISABLED + // The free-threaded build interns (and immortalizes) all string constants + // unless we've disabled immortalizing objects that use deferred reference + // counting. + PyInterpreterState *interp = _PyInterpreterState_GET(); + if (interp->gc.immortalize.enable_on_thread_created) { + return 1; + } +#endif + + // compute if s matches [a-zA-Z0-9_] const unsigned char *s, *e; if (!PyUnicode_IS_ASCII(o)) @@ -118,6 +130,10 @@ all_name_chars(PyObject *o) return 1; } +#ifdef Py_GIL_DISABLED +static PyObject *intern_one_constant(PyObject *op); +#endif + static int intern_strings(PyObject *tuple) { @@ -135,14 +151,16 @@ intern_strings(PyObject *tuple) return 0; } -/* Intern selected string constants */ +/* Intern constants. In the default build, this interns selected string + constants. In the free-threaded build, this also interns non-string + constants. */ static int -intern_string_constants(PyObject *tuple, int *modified) +intern_constants(PyObject *tuple, int *modified) { for (Py_ssize_t i = PyTuple_GET_SIZE(tuple); --i >= 0; ) { PyObject *v = PyTuple_GET_ITEM(tuple, i); if (PyUnicode_CheckExact(v)) { - if (all_name_chars(v)) { + if (should_intern_string(v)) { PyObject *w = v; PyUnicode_InternInPlace(&v); if (w != v) { @@ -154,7 +172,7 @@ intern_string_constants(PyObject *tuple, int *modified) } } else if (PyTuple_CheckExact(v)) { - if (intern_string_constants(v, NULL) < 0) { + if (intern_constants(v, NULL) < 0) { return -1; } } @@ -165,7 +183,7 @@ intern_string_constants(PyObject *tuple, int *modified) return -1; } int tmp_modified = 0; - if (intern_string_constants(tmp, &tmp_modified) < 0) { + if (intern_constants(tmp, &tmp_modified) < 0) { Py_DECREF(tmp); return -1; } @@ -184,6 +202,59 @@ intern_string_constants(PyObject *tuple, int *modified) } Py_DECREF(tmp); } +#ifdef Py_GIL_DISABLED + else if (PySlice_Check(v)) { + PySliceObject *slice = (PySliceObject *)v; + PyObject *tmp = PyTuple_New(3); + if (tmp == NULL) { + return -1; + } + PyTuple_SET_ITEM(tmp, 0, Py_NewRef(slice->start)); + PyTuple_SET_ITEM(tmp, 1, Py_NewRef(slice->stop)); + PyTuple_SET_ITEM(tmp, 2, Py_NewRef(slice->step)); + int tmp_modified = 0; + if (intern_constants(tmp, &tmp_modified) < 0) { + Py_DECREF(tmp); + return -1; + } + if (tmp_modified) { + v = PySlice_New(PyTuple_GET_ITEM(tmp, 0), + PyTuple_GET_ITEM(tmp, 1), + PyTuple_GET_ITEM(tmp, 2)); + if (v == NULL) { + Py_DECREF(tmp); + return -1; + } + PyTuple_SET_ITEM(tuple, i, v); + Py_DECREF(slice); + if (modified) { + *modified = 1; + } + } + Py_DECREF(tmp); + } + + // Intern non-string consants in the free-threaded build, but only if + // we are also immortalizing objects that use deferred reference + // counting. + PyThreadState *tstate = PyThreadState_GET(); + if (!_Py_IsImmortal(v) && !PyCode_Check(v) && + !PyUnicode_CheckExact(v) && + tstate->interp->gc.immortalize.enable_on_thread_created) + { + PyObject *interned = intern_one_constant(v); + if (interned == NULL) { + return -1; + } + else if (interned != v) { + PyTuple_SET_ITEM(tuple, i, interned); + Py_SETREF(v, interned); + if (modified) { + *modified = 1; + } + } + } +#endif } return 0; } @@ -540,18 +611,41 @@ remove_column_info(PyObject *locations) return res; } +static int +intern_code_constants(struct _PyCodeConstructor *con) +{ +#ifdef Py_GIL_DISABLED + PyInterpreterState *interp = _PyInterpreterState_GET(); + struct _py_code_state *state = &interp->code_state; + PyMutex_Lock(&state->mutex); +#endif + if (intern_strings(con->names) < 0) { + goto error; + } + if (intern_constants(con->consts, NULL) < 0) { + goto error; + } + if (intern_strings(con->localsplusnames) < 0) { + goto error; + } +#ifdef Py_GIL_DISABLED + PyMutex_Unlock(&state->mutex); +#endif + return 0; + +error: +#ifdef Py_GIL_DISABLED + PyMutex_Unlock(&state->mutex); +#endif + return -1; +} + /* The caller is responsible for ensuring that the given data is valid. */ PyCodeObject * _PyCode_New(struct _PyCodeConstructor *con) { - if (intern_strings(con->names) < 0) { - return NULL; - } - if (intern_string_constants(con->consts, NULL) < 0) { - return NULL; - } - if (intern_strings(con->localsplusnames) < 0) { + if (intern_code_constants(con) < 0) { return NULL; } @@ -2397,3 +2491,183 @@ _PyCode_ConstantKey(PyObject *op) } return key; } + +#ifdef Py_GIL_DISABLED +static PyObject * +intern_one_constant(PyObject *op) +{ + PyInterpreterState *interp = _PyInterpreterState_GET(); + _Py_hashtable_t *consts = interp->code_state.constants; + + assert(!PyUnicode_CheckExact(op)); // strings are interned separately + + _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(consts, op); + if (entry == NULL) { + if (_Py_hashtable_set(consts, op, op) != 0) { + return NULL; + } + +#ifdef Py_REF_DEBUG + Py_ssize_t refcnt = Py_REFCNT(op); + if (refcnt != 1) { + // Adjust the reftotal to account for the fact that we only + // restore a single reference in _PyCode_Fini. + _Py_AddRefTotal(_PyThreadState_GET(), -(refcnt - 1)); + } +#endif + + _Py_SetImmortal(op); + return op; + } + + assert(_Py_IsImmortal(entry->value)); + return (PyObject *)entry->value; +} + +static int +compare_constants(const void *key1, const void *key2) { + PyObject *op1 = (PyObject *)key1; + PyObject *op2 = (PyObject *)key2; + if (op1 == op2) { + return 1; + } + if (Py_TYPE(op1) != Py_TYPE(op2)) { + return 0; + } + // We compare container contents by identity because we have already + // internalized the items. + if (PyTuple_CheckExact(op1)) { + Py_ssize_t size = PyTuple_GET_SIZE(op1); + if (size != PyTuple_GET_SIZE(op2)) { + return 0; + } + for (Py_ssize_t i = 0; i < size; i++) { + if (PyTuple_GET_ITEM(op1, i) != PyTuple_GET_ITEM(op2, i)) { + return 0; + } + } + return 1; + } + else if (PyFrozenSet_CheckExact(op1)) { + if (PySet_GET_SIZE(op1) != PySet_GET_SIZE(op2)) { + return 0; + } + Py_ssize_t pos1 = 0, pos2 = 0; + PyObject *obj1, *obj2; + Py_hash_t hash1, hash2; + while ((_PySet_NextEntry(op1, &pos1, &obj1, &hash1)) && + (_PySet_NextEntry(op2, &pos2, &obj2, &hash2))) + { + if (obj1 != obj2) { + return 0; + } + } + return 1; + } + else if (PySlice_Check(op1)) { + PySliceObject *s1 = (PySliceObject *)op1; + PySliceObject *s2 = (PySliceObject *)op2; + return (s1->start == s2->start && + s1->stop == s2->stop && + s1->step == s2->step); + } + else if (PyBytes_CheckExact(op1) || PyLong_CheckExact(op1)) { + return PyObject_RichCompareBool(op1, op2, Py_EQ); + } + else if (PyFloat_CheckExact(op1)) { + // Ensure that, for example, +0.0 and -0.0 are distinct + double f1 = PyFloat_AS_DOUBLE(op1); + double f2 = PyFloat_AS_DOUBLE(op2); + return memcmp(&f1, &f2, sizeof(double)) == 0; + } + else if (PyComplex_CheckExact(op1)) { + Py_complex c1 = ((PyComplexObject *)op1)->cval; + Py_complex c2 = ((PyComplexObject *)op2)->cval; + return memcmp(&c1, &c2, sizeof(Py_complex)) == 0; + } + _Py_FatalErrorFormat("unexpected type in compare_constants: %s", + Py_TYPE(op1)->tp_name); + return 0; +} + +static Py_uhash_t +hash_const(const void *key) +{ + PyObject *op = (PyObject *)key; + if (PySlice_Check(op)) { + PySliceObject *s = (PySliceObject *)op; + PyObject *data[3] = { s->start, s->stop, s->step }; + return _Py_HashBytes(&data, sizeof(data)); + } + else if (PyTuple_CheckExact(op)) { + Py_ssize_t size = PyTuple_GET_SIZE(op); + PyObject **data = _PyTuple_ITEMS(op); + return _Py_HashBytes(data, sizeof(PyObject *) * size); + } + Py_hash_t h = PyObject_Hash(op); + if (h == -1) { + // This should never happen: all the constants we support have + // infallible hash functions. + Py_FatalError("code: hash failed"); + } + return (Py_uhash_t)h; +} + +static int +clear_containers(_Py_hashtable_t *ht, const void *key, const void *value, + void *user_data) +{ + // First clear containers to avoid recursive deallocation later on in + // destroy_key. + PyObject *op = (PyObject *)key; + if (PyTuple_CheckExact(op)) { + for (Py_ssize_t i = 0; i < PyTuple_GET_SIZE(op); i++) { + Py_CLEAR(_PyTuple_ITEMS(op)[i]); + } + } + else if (PySlice_Check(op)) { + PySliceObject *slice = (PySliceObject *)op; + Py_SETREF(slice->start, Py_None); + Py_SETREF(slice->stop, Py_None); + Py_SETREF(slice->step, Py_None); + } + else if (PyFrozenSet_CheckExact(op)) { + _PySet_ClearInternal((PySetObject *)op); + } + return 0; +} + +static void +destroy_key(void *key) +{ + _Py_ClearImmortal(key); +} +#endif + +PyStatus +_PyCode_Init(PyInterpreterState *interp) +{ +#ifdef Py_GIL_DISABLED + struct _py_code_state *state = &interp->code_state; + state->constants = _Py_hashtable_new_full(&hash_const, &compare_constants, + &destroy_key, NULL, NULL); + if (state->constants == NULL) { + return _PyStatus_NO_MEMORY(); + } +#endif + return _PyStatus_OK(); +} + +void +_PyCode_Fini(PyInterpreterState *interp) +{ +#ifdef Py_GIL_DISABLED + // Free interned constants + struct _py_code_state *state = &interp->code_state; + if (state->constants) { + _Py_hashtable_foreach(state->constants, &clear_containers, NULL); + _Py_hashtable_destroy(state->constants); + state->constants = NULL; + } +#endif +} diff --git a/Objects/setobject.c b/Objects/setobject.c index 19975e3d4d1..68986bb6a6b 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -2621,6 +2621,12 @@ PySet_Clear(PyObject *set) return 0; } +void +_PySet_ClearInternal(PySetObject *so) +{ + (void)set_clear_internal(so); +} + int PySet_Contains(PyObject *anyset, PyObject *key) { diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index e7b60ebca5e..d192d5be751 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -866,8 +866,21 @@ builtin_compile_impl(PyObject *module, PyObject *source, PyObject *filename, if (str == NULL) goto error; +#ifdef Py_GIL_DISABLED + // gh-118527: Disable immortalization of code constants for explicit + // compile() calls to get consistent frozen outputs between the default + // and free-threaded builds. + PyInterpreterState *interp = _PyInterpreterState_GET(); + int old_value = interp->gc.immortalize.enable_on_thread_created; + interp->gc.immortalize.enable_on_thread_created = 0; +#endif + result = Py_CompileStringObject(str, filename, start[compile_mode], &cf, optimize); +#ifdef Py_GIL_DISABLED + interp->gc.immortalize.enable_on_thread_created = old_value; +#endif + Py_XDECREF(source_copy); goto finally; diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index f24b0482c2b..67bbbd01ca0 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -854,6 +854,11 @@ pycore_interp_init(PyThreadState *tstate) return status; } + status = _PyCode_Init(interp); + if (_PyStatus_EXCEPTION(status)) { + return status; + } + status = _PyDtoa_Init(interp); if (_PyStatus_EXCEPTION(status)) { return status; @@ -1827,6 +1832,8 @@ finalize_interp_types(PyInterpreterState *interp) _PyTypes_Fini(interp); + _PyCode_Fini(interp); + // Call _PyUnicode_ClearInterned() before _PyDict_Fini() since it uses // a dict internally. _PyUnicode_ClearInterned(interp); diff --git a/Python/pystate.c b/Python/pystate.c index f442d87ba31..7c75263b7e5 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -404,6 +404,7 @@ _Py_COMP_DIAG_POP &(runtime)->audit_hooks.mutex, \ &(runtime)->allocators.mutex, \ &(runtime)->_main_interpreter.types.mutex, \ + &(runtime)->_main_interpreter.code_state.mutex, \ } static void