From 4881760ae849a7ad5651bfaae12b338cf0d73871 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 3 Oct 2023 12:07:52 -0600 Subject: [PATCH 1/4] Clear the XID registry during runtime fini. --- Python/pystate.c | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index fe056bf4687026..8511aa98a3f1bb 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -495,6 +495,8 @@ _PyRuntimeState_Init(_PyRuntimeState *runtime) return _PyStatus_OK(); } +static void _xidregistry_clear(struct _xidregistry *); + void _PyRuntimeState_Fini(_PyRuntimeState *runtime) { @@ -503,6 +505,8 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime) assert(runtime->object_state.interpreter_leaks == 0); #endif + _xidregistry_clear(&runtime->xidregistry); + if (gilstate_tss_initialized(runtime)) { gilstate_tss_fini(runtime); } @@ -2571,23 +2575,25 @@ _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *data) crossinterpdatafunc. It would be simpler and more efficient. */ static int -_xidregistry_add_type(struct _xidregistry *xidregistry, PyTypeObject *cls, - crossinterpdatafunc getdata) +_xidregistry_add_type(struct _xidregistry *xidregistry, + PyTypeObject *cls, crossinterpdatafunc getdata) { // Note that we effectively replace already registered classes + // (since first match wins and we're adding to the front of the list) // rather than failing. struct _xidregitem *newhead = PyMem_RawMalloc(sizeof(struct _xidregitem)); if (newhead == NULL) { return -1; } - // XXX Assign a callback to clear the entry from the registry? - newhead->cls = PyWeakref_NewRef((PyObject *)cls, NULL); + *newhead = (struct _xidregitem){ + // XXX Assign a callback to clear the entry from the registry? + .cls = PyWeakref_NewRef((PyObject *)cls, NULL), + .getdata = getdata, + }; if (newhead->cls == NULL) { PyMem_RawFree(newhead); return -1; } - newhead->getdata = getdata; - newhead->prev = NULL; newhead->next = xidregistry->head; if (newhead->next != NULL) { newhead->next->prev = newhead; @@ -2617,6 +2623,20 @@ _xidregistry_remove_entry(struct _xidregistry *xidregistry, return next; } +static void +_xidregistry_clear(struct _xidregistry *xidregistry) +{ + struct _xidregitem *cur = xidregistry->head; + xidregistry->head = NULL; + while (cur != NULL) { + struct _xidregitem *next = cur->next; + /* Release the weakref. */ + Py_DECREF(cur->cls); + PyMem_RawFree(cur); + cur = next; + } +} + static struct _xidregitem * _xidregistry_find_type(struct _xidregistry *xidregistry, PyTypeObject *cls) { @@ -2629,11 +2649,10 @@ _xidregistry_find_type(struct _xidregistry *xidregistry, PyTypeObject *cls) } else { assert(PyType_Check(registered)); + Py_DECREF(registered); if (registered == (PyObject *)cls) { - Py_DECREF(registered); return cur; } - Py_DECREF(registered); cur = cur->next; } } @@ -2644,7 +2663,7 @@ static void _register_builtins_for_crossinterpreter_data(struct _xidregistry *xi int _PyCrossInterpreterData_RegisterClass(PyTypeObject *cls, - crossinterpdatafunc getdata) + crossinterpdatafunc getdata) { if (!PyType_Check(cls)) { PyErr_Format(PyExc_ValueError, "only classes may be registered"); From df4c217195eccebf04b99bc9121e78eaf90979c7 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 3 Oct 2023 12:09:36 -0600 Subject: [PATCH 2/4] Give each interpreter an XID registry. --- Include/internal/pycore_interp.h | 40 +++++++++++++++++++------------ Include/internal/pycore_runtime.h | 5 +--- Python/pystate.c | 13 ++++++++++ 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index ebf02281a7a2a6..2e1e6a0c1cadf4 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -39,6 +39,28 @@ struct _Py_long_state { int max_str_digits; }; + +/* cross-interpreter data registry */ + +/* For now we use a global registry of shareable classes. An + alternative would be to add a tp_* slot for a class's + crossinterpdatafunc. It would be simpler and more efficient. */ + +struct _xidregitem; + +struct _xidregitem { + struct _xidregitem *prev; + struct _xidregitem *next; + PyObject *cls; // weakref to a PyTypeObject + crossinterpdatafunc getdata; +}; + +struct _xidregistry { + PyThread_type_lock mutex; + struct _xidregitem *head; +}; + + /* interpreter state */ /* PyInterpreterState holds the global state for one of the runtime's @@ -150,6 +172,9 @@ struct _is { Py_ssize_t co_extra_user_count; freefunc co_extra_freefuncs[MAX_CO_EXTRA_USERS]; + // XXX Remove this field once we have a tp_* slot. + struct _xidregistry xidregistry; + #ifdef HAVE_FORK PyObject *before_forkers; PyObject *after_forkers_parent; @@ -239,21 +264,6 @@ _PyInterpreterState_SetFinalizing(PyInterpreterState *interp, PyThreadState *tst } -/* cross-interpreter data registry */ - -/* For now we use a global registry of shareable classes. An - alternative would be to add a tp_* slot for a class's - crossinterpdatafunc. It would be simpler and more efficient. */ - -struct _xidregitem; - -struct _xidregitem { - struct _xidregitem *prev; - struct _xidregitem *next; - PyObject *cls; // weakref to a PyTypeObject - crossinterpdatafunc getdata; -}; - extern PyInterpreterState* _PyInterpreterState_LookUpID(int64_t); extern int _PyInterpreterState_IDInitref(PyInterpreterState *); diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index cc3a3420befa3d..1dc243e46e8ef8 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -201,10 +201,7 @@ typedef struct pyruntimestate { tools. */ // XXX Remove this field once we have a tp_* slot. - struct _xidregistry { - PyThread_type_lock mutex; - struct _xidregitem *head; - } xidregistry; + struct _xidregistry xidregistry; struct _pymem_allocators allocators; struct _obmalloc_global_state obmalloc; diff --git a/Python/pystate.c b/Python/pystate.c index 8511aa98a3f1bb..1a819587cdcd7d 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -552,6 +552,11 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) for (int i = 0; i < NUMLOCKS; i++) { reinit_err += _PyThread_at_fork_reinit(lockptrs[i]); } + /* PyOS_AfterFork_Child(), which calls this function, later calls + _PyInterpreterState_DeleteExceptMain(), so we only need to update + the main interpreter here. */ + assert(runtime->interpreters.main != NULL); + runtime->interpreters.main->xidregistry.mutex = runtime->xidregistry.mutex; PyMem_SetAllocator(PYMEM_DOMAIN_RAW, &old_alloc); @@ -713,6 +718,10 @@ init_interpreter(PyInterpreterState *interp, interp->dtoa = (struct _dtoa_state)_dtoa_state_INIT(interp); } interp->f_opcode_trace_set = false; + + assert(runtime->xidregistry.mutex != NULL); + interp->xidregistry.mutex = runtime->xidregistry.mutex; + interp->_initialized = 1; return _PyStatus_OK(); } @@ -934,6 +943,10 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate) Py_CLEAR(interp->sysdict); Py_CLEAR(interp->builtins); + _xidregistry_clear(&interp->xidregistry); + /* The lock is owned by the runtime, so we don't free it here. */ + interp->xidregistry.mutex = NULL; + if (tstate->interp == interp) { /* We are now safe to fix tstate->_status.cleared. */ // XXX Do this (much) earlier? From 3cbb996e068316d0ac1fbdb739eb17582e11c3f7 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 3 Oct 2023 13:20:15 -0600 Subject: [PATCH 3/4] Keep heap types on the per-interpreter registry. --- Include/internal/pycore_interp.h | 5 +- Python/pystate.c | 99 +++++++++++++++++++++----------- 2 files changed, 71 insertions(+), 33 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 2e1e6a0c1cadf4..5c992cbbd7b943 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -51,7 +51,10 @@ struct _xidregitem; struct _xidregitem { struct _xidregitem *prev; struct _xidregitem *next; - PyObject *cls; // weakref to a PyTypeObject + /* This can be a dangling pointer, but only if weakref is set. */ + PyTypeObject *cls; + /* This is NULL for builtin types. */ + PyObject *weakref; crossinterpdatafunc getdata; }; diff --git a/Python/pystate.c b/Python/pystate.c index 1a819587cdcd7d..3daf56afa79df0 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2599,13 +2599,17 @@ _xidregistry_add_type(struct _xidregistry *xidregistry, return -1; } *newhead = (struct _xidregitem){ - // XXX Assign a callback to clear the entry from the registry? - .cls = PyWeakref_NewRef((PyObject *)cls, NULL), + // We do not keep a reference, to avoid keeping the class alive. + .cls = cls, .getdata = getdata, }; - if (newhead->cls == NULL) { - PyMem_RawFree(newhead); - return -1; + if (cls->tp_flags & Py_TPFLAGS_HEAPTYPE) { + // XXX Assign a callback to clear the entry from the registry? + newhead->weakref = PyWeakref_NewRef((PyObject *)cls, NULL); + if (newhead->weakref == NULL) { + PyMem_RawFree(newhead); + return -1; + } } newhead->next = xidregistry->head; if (newhead->next != NULL) { @@ -2631,7 +2635,7 @@ _xidregistry_remove_entry(struct _xidregistry *xidregistry, if (next != NULL) { next->prev = entry->prev; } - Py_DECREF(entry->cls); + Py_XDECREF(entry->weakref); PyMem_RawFree(entry); return next; } @@ -2643,8 +2647,7 @@ _xidregistry_clear(struct _xidregistry *xidregistry) xidregistry->head = NULL; while (cur != NULL) { struct _xidregitem *next = cur->next; - /* Release the weakref. */ - Py_DECREF(cur->cls); + Py_XDECREF(cur->weakref); PyMem_RawFree(cur); cur = next; } @@ -2655,25 +2658,51 @@ _xidregistry_find_type(struct _xidregistry *xidregistry, PyTypeObject *cls) { struct _xidregitem *cur = xidregistry->head; while (cur != NULL) { - PyObject *registered = _PyWeakref_GET_REF(cur->cls); - if (registered == NULL) { - // The weakly ref'ed object was freed. - cur = _xidregistry_remove_entry(xidregistry, cur); - } - else { + if (cur->weakref != NULL) { + // cur is/was a heap type. + PyObject *registered = _PyWeakref_GET_REF(cur->weakref); + if (registered == NULL) { + // The weakly ref'ed object was freed. + cur = _xidregistry_remove_entry(xidregistry, cur); + continue; + } assert(PyType_Check(registered)); + assert(cur->cls == (PyTypeObject *)registered); + assert(cur->cls->tp_flags & Py_TPFLAGS_HEAPTYPE); Py_DECREF(registered); - if (registered == (PyObject *)cls) { - return cur; - } - cur = cur->next; } + if (cur->cls == cls) { + return cur; + } + cur = cur->next; } return NULL; } +static inline struct _xidregistry * +_get_xidregistry(PyInterpreterState *interp, PyTypeObject *cls) +{ + struct _xidregistry *xidregistry = &interp->runtime->xidregistry; + if (cls->tp_flags & Py_TPFLAGS_HEAPTYPE) { + assert(interp->xidregistry.mutex == xidregistry->mutex); + xidregistry = &interp->xidregistry; + } + return xidregistry; +} + static void _register_builtins_for_crossinterpreter_data(struct _xidregistry *xidregistry); +static inline void +_ensure_builtins_xid(PyInterpreterState *interp, struct _xidregistry *xidregistry) +{ + if (xidregistry != &interp->xidregistry) { + assert(xidregistry == &interp->runtime->xidregistry); + if (xidregistry->head == NULL) { + _register_builtins_for_crossinterpreter_data(xidregistry); + } + } +} + int _PyCrossInterpreterData_RegisterClass(PyTypeObject *cls, crossinterpdatafunc getdata) @@ -2687,12 +2716,13 @@ _PyCrossInterpreterData_RegisterClass(PyTypeObject *cls, return -1; } - struct _xidregistry *xidregistry = &_PyRuntime.xidregistry ; + PyInterpreterState *interp = _PyInterpreterState_GET(); + struct _xidregistry *xidregistry = _get_xidregistry(interp, cls); PyThread_acquire_lock(xidregistry->mutex, WAIT_LOCK); - if (xidregistry->head == NULL) { - _register_builtins_for_crossinterpreter_data(xidregistry); - } + + _ensure_builtins_xid(interp, xidregistry); int res = _xidregistry_add_type(xidregistry, cls, getdata); + PyThread_release_lock(xidregistry->mutex); return res; } @@ -2701,13 +2731,16 @@ int _PyCrossInterpreterData_UnregisterClass(PyTypeObject *cls) { int res = 0; - struct _xidregistry *xidregistry = &_PyRuntime.xidregistry ; + PyInterpreterState *interp = _PyInterpreterState_GET(); + struct _xidregistry *xidregistry = _get_xidregistry(interp, cls); PyThread_acquire_lock(xidregistry->mutex, WAIT_LOCK); + struct _xidregitem *matched = _xidregistry_find_type(xidregistry, cls); if (matched != NULL) { (void)_xidregistry_remove_entry(xidregistry, matched); res = 1; } + PyThread_release_lock(xidregistry->mutex); return res; } @@ -2720,17 +2753,19 @@ _PyCrossInterpreterData_UnregisterClass(PyTypeObject *cls) crossinterpdatafunc _PyCrossInterpreterData_Lookup(PyObject *obj) { - struct _xidregistry *xidregistry = &_PyRuntime.xidregistry ; - PyObject *cls = PyObject_Type(obj); + PyTypeObject *cls = Py_TYPE(obj); + + PyInterpreterState *interp = _PyInterpreterState_GET(); + struct _xidregistry *xidregistry = _get_xidregistry(interp, cls); PyThread_acquire_lock(xidregistry->mutex, WAIT_LOCK); - if (xidregistry->head == NULL) { - _register_builtins_for_crossinterpreter_data(xidregistry); - } - struct _xidregitem *matched = _xidregistry_find_type(xidregistry, - (PyTypeObject *)cls); - Py_DECREF(cls); + + _ensure_builtins_xid(interp, xidregistry); + + struct _xidregitem *matched = _xidregistry_find_type(xidregistry, cls); + crossinterpdatafunc func = matched != NULL ? matched->getdata : NULL; + PyThread_release_lock(xidregistry->mutex); - return matched != NULL ? matched->getdata : NULL; + return func; } /* cross-interpreter data for builtin types */ From 92fad3f5df3d56016a1cece412fbe5e289a270ce Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 3 Oct 2023 13:55:29 -0600 Subject: [PATCH 4/4] Only add entries if no existing match. --- Include/internal/pycore_interp.h | 1 + Python/pystate.c | 22 +++++++++++++++++----- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 5c992cbbd7b943..6465ee4a38d802 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -55,6 +55,7 @@ struct _xidregitem { PyTypeObject *cls; /* This is NULL for builtin types. */ PyObject *weakref; + size_t refcount; crossinterpdatafunc getdata; }; diff --git a/Python/pystate.c b/Python/pystate.c index 3daf56afa79df0..eeaa3b5239c5f2 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2591,9 +2591,6 @@ static int _xidregistry_add_type(struct _xidregistry *xidregistry, PyTypeObject *cls, crossinterpdatafunc getdata) { - // Note that we effectively replace already registered classes - // (since first match wins and we're adding to the front of the list) - // rather than failing. struct _xidregitem *newhead = PyMem_RawMalloc(sizeof(struct _xidregitem)); if (newhead == NULL) { return -1; @@ -2601,6 +2598,7 @@ _xidregistry_add_type(struct _xidregistry *xidregistry, *newhead = (struct _xidregitem){ // We do not keep a reference, to avoid keeping the class alive. .cls = cls, + .refcount = 1, .getdata = getdata, }; if (cls->tp_flags & Py_TPFLAGS_HEAPTYPE) { @@ -2716,13 +2714,23 @@ _PyCrossInterpreterData_RegisterClass(PyTypeObject *cls, return -1; } + int res = 0; PyInterpreterState *interp = _PyInterpreterState_GET(); struct _xidregistry *xidregistry = _get_xidregistry(interp, cls); PyThread_acquire_lock(xidregistry->mutex, WAIT_LOCK); _ensure_builtins_xid(interp, xidregistry); - int res = _xidregistry_add_type(xidregistry, cls, getdata); + struct _xidregitem *matched = _xidregistry_find_type(xidregistry, cls); + if (matched != NULL) { + assert(matched->getdata == getdata); + matched->refcount += 1; + goto finally; + } + + res = _xidregistry_add_type(xidregistry, cls, getdata); + +finally: PyThread_release_lock(xidregistry->mutex); return res; } @@ -2737,7 +2745,11 @@ _PyCrossInterpreterData_UnregisterClass(PyTypeObject *cls) struct _xidregitem *matched = _xidregistry_find_type(xidregistry, cls); if (matched != NULL) { - (void)_xidregistry_remove_entry(xidregistry, matched); + assert(matched->refcount > 0); + matched->refcount -= 1; + if (matched->refcount == 0) { + (void)_xidregistry_remove_entry(xidregistry, matched); + } res = 1; }