From 9bf3820977b70e6f3cc0d9d98b37e9ffbfa22fe5 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 14 Mar 2024 16:37:33 -0700 Subject: [PATCH 01/66] Add _PyObject_SetMaybeWeakref We need to tag weakrefs and their referents --- Include/internal/pycore_object.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 13fe543133f11e..be8b7776bf1b0b 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -507,6 +507,25 @@ _Py_XNewRefWithLock(PyObject *obj) return _Py_NewRefWithLock(obj); } +static inline void +_PyObject_SetMaybeWeakref(PyObject *op) +{ + if (_Py_IsImmortal(op)) { + return; + } + for (;;) { + Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared); + if ((shared & _Py_REF_SHARED_FLAG_MASK) != 0) { + // Nothing to do if it's in WEAKREFS, QUEUED, or MERGED states. + return; + } + if (_Py_atomic_compare_exchange_ssize( + &op->ob_ref_shared, &shared, shared | _Py_REF_MAYBE_WEAKREF)) { + return; + } + } +} + #endif #ifdef Py_REF_DEBUG From 75603c158bfe227befbcebea5ced973f67ff9b58 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 14 Mar 2024 17:04:26 -0700 Subject: [PATCH 02/66] Make PyWeakref_NewRef thread-safe --- Objects/weakrefobject.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index b7b29064151609..fd2d64420ddeae 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -33,6 +33,10 @@ init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) self->wr_next = NULL; self->wr_callback = Py_XNewRef(callback); self->vectorcall = weakref_vectorcall; +#ifdef Py_GIL_DISABLED + _PyObject_SetMaybeWeakref(ob); + _PyObject_SetMaybeWeakref((PyObject *)self); +#endif } static PyWeakReference * @@ -792,22 +796,32 @@ PyWeakref_NewRef(PyObject *ob, PyObject *callback) return NULL; } list = GET_WEAKREFS_LISTPTR(ob); + /* NB: For free-threaded builds its critical that no code inside the + * critical section can release it. We need to recompute ref/proxy after + * any code that may release the critical section. + */ + Py_BEGIN_CRITICAL_SECTION(ob); get_basic_refs(*list, &ref, &proxy); if (callback == Py_None) callback = NULL; if (callback == NULL) /* return existing weak reference if it exists */ result = ref; +#ifdef Py_GIL_DISABLED + /* Incref will fail if the existing weakref is being destroyed */ + if (result == NULL || + !_Py_TryIncref((PyObject **)&result, (PyObject *)result)) { +#else if (result != NULL) Py_INCREF(result); else { +#endif /* We do not need to recompute ref/proxy; new_weakref() cannot trigger GC. */ result = new_weakref(ob, callback); if (result != NULL) { if (callback == NULL) { - assert(ref == NULL); insert_head(result, list); } else { @@ -821,6 +835,7 @@ PyWeakref_NewRef(PyObject *ob, PyObject *callback) } } } + Py_END_CRITICAL_SECTION(); return (PyObject *) result; } From c4f8453397466d88289b8eb616833a6da3da13d7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 14 Mar 2024 17:10:28 -0700 Subject: [PATCH 03/66] Make PyWeakref_NewProxy thread-safe --- Objects/weakrefobject.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index fd2d64420ddeae..732b3a86002c2e 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -854,15 +854,26 @@ PyWeakref_NewProxy(PyObject *ob, PyObject *callback) return NULL; } list = GET_WEAKREFS_LISTPTR(ob); + /* NB: For free-threaded builds its critical that no code inside the + * critical section can release it. We need to recompute ref/proxy after + * any code that may release the critical section. + */ + Py_BEGIN_CRITICAL_SECTION(ob); get_basic_refs(*list, &ref, &proxy); if (callback == Py_None) callback = NULL; if (callback == NULL) /* attempt to return an existing weak reference if it exists */ result = proxy; +#ifdef Py_GIL_DISABLED + /* Incref will fail if the existing weakref is being destroyed */ + if (result == NULL || + !_Py_TryIncref((PyObject **)&result, (PyObject *)result)) { +#else if (result != NULL) Py_INCREF(result); else { +#endif /* We do not need to recompute ref/proxy; new_weakref cannot trigger GC. */ @@ -888,6 +899,7 @@ PyWeakref_NewProxy(PyObject *ob, PyObject *callback) insert_after(result, prev); } } + Py_END_CRITICAL_SECTION(); return (PyObject *) result; } From ece230f01375f7419d7e6a1089997618aada19bb Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 14 Mar 2024 17:25:30 -0700 Subject: [PATCH 04/66] Make weakref___new___ thread-safe --- Objects/weakrefobject.c | 82 ++++++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 33 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 732b3a86002c2e..1053f402ea304c 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -287,6 +287,52 @@ parse_weakref_init_args(const char *funcname, PyObject *args, PyObject *kwargs, return PyArg_UnpackTuple(args, funcname, 1, 2, obp, callbackp); } +static PyWeakReference * +new_weakref_lock_held(PyTypeObject *type, PyObject *ob, PyObject *callback) +{ + PyWeakReference *ref, *proxy; + PyWeakReference **list = GET_WEAKREFS_LISTPTR(ob); + get_basic_refs(*list, &ref, &proxy); + if (callback == NULL && type == &_PyWeakref_RefType) { +#ifdef Py_GIL_DISABLED + if (ref != NULL && + _Py_TryIncref((PyObject**)&ref, (PyObject*)ref)) { + /* We can re-use an existing reference. */ + return ref; + } +#else + if (ref != NULL) { + /* We can re-use an existing reference. */ + return Py_NewRef(ref); + } +#endif + } + /* We have to create a new reference. */ + /* Note: the tp_alloc() can trigger cyclic GC, so the weakref + list on ob can be mutated. This means that the ref and + proxy pointers we got back earlier may have been collected, + so we need to compute these values again before we use + them. */ + PyWeakReference *self = (PyWeakReference *) (type->tp_alloc(type, 0)); + if (self != NULL) { + init_weakref(self, ob, callback); + if (callback == NULL && type == &_PyWeakref_RefType) { + insert_head(self, list); + } + else { + PyWeakReference *prev; + + get_basic_refs(*list, &ref, &proxy); + prev = (proxy == NULL) ? ref : proxy; + if (prev == NULL) + insert_head(self, list); + else + insert_after(self, prev); + } + } + return self; +} + static PyObject * weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) { @@ -294,8 +340,6 @@ weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) PyObject *ob, *callback = NULL; if (parse_weakref_init_args("__new__", args, kwargs, &ob, &callback)) { - PyWeakReference *ref, *proxy; - PyWeakReference **list; if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(ob))) { PyErr_Format(PyExc_TypeError, @@ -305,37 +349,9 @@ weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) } if (callback == Py_None) callback = NULL; - list = GET_WEAKREFS_LISTPTR(ob); - get_basic_refs(*list, &ref, &proxy); - if (callback == NULL && type == &_PyWeakref_RefType) { - if (ref != NULL) { - /* We can re-use an existing reference. */ - return Py_NewRef(ref); - } - } - /* We have to create a new reference. */ - /* Note: the tp_alloc() can trigger cyclic GC, so the weakref - list on ob can be mutated. This means that the ref and - proxy pointers we got back earlier may have been collected, - so we need to compute these values again before we use - them. */ - self = (PyWeakReference *) (type->tp_alloc(type, 0)); - if (self != NULL) { - init_weakref(self, ob, callback); - if (callback == NULL && type == &_PyWeakref_RefType) { - insert_head(self, list); - } - else { - PyWeakReference *prev; - - get_basic_refs(*list, &ref, &proxy); - prev = (proxy == NULL) ? ref : proxy; - if (prev == NULL) - insert_head(self, list); - else - insert_after(self, prev); - } - } + Py_BEGIN_CRITICAL_SECTION(ob); + self = new_weakref_lock_held(type, ob, callback); + Py_END_CRITICAL_SECTION(); } return (PyObject *)self; } From 9acae594386dc60545468ef9c4bd567529c44a89 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 14 Mar 2024 17:40:57 -0700 Subject: [PATCH 05/66] Make weakref_hash thread-safe --- Objects/weakrefobject.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 1053f402ea304c..59573395fd74db 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -154,7 +154,7 @@ weakref_vectorcall(PyObject *self, PyObject *const *args, } static Py_hash_t -weakref_hash(PyWeakReference *self) +weakref_hash_lock_held(PyWeakReference *self) { if (self->hash != -1) return self->hash; @@ -168,6 +168,15 @@ weakref_hash(PyWeakReference *self) return self->hash; } +static Py_hash_t +weakref_hash(PyWeakReference *self) +{ + Py_hash_t hash; + Py_BEGIN_CRITICAL_SECTION(self); + hash = weakref_hash_lock_held(self); + Py_END_CRITICAL_SECTION(); + return hash; +} static PyObject * weakref_repr(PyObject *self) From 6d00f51a860c5c7404b9a663b8861cdfdff73ecf Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 15 Mar 2024 16:52:49 -0700 Subject: [PATCH 06/66] Make _PyStaticType_ClearWeakRefs thread-safe --- Objects/weakrefobject.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 59573395fd74db..9fbc845801fe43 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1069,10 +1069,12 @@ _PyStaticType_ClearWeakRefs(PyInterpreterState *interp, PyTypeObject *type) { static_builtin_state *state = _PyStaticType_GetState(interp, type); PyObject **list = _PyStaticType_GET_WEAKREFS_LISTPTR(state); + Py_BEGIN_CRITICAL_SECTION(type); while (*list != NULL) { /* Note that clear_weakref() pops the first ref off the type's weaklist before clearing its wr_object and wr_callback. That is how we're able to loop over the list. */ clear_weakref((PyWeakReference *)*list); } + Py_END_CRITICAL_SECTION(); } From 363a1a6b0d57d3220b28f3bd5b2b2db2e76aa194 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 15 Mar 2024 14:55:55 -0700 Subject: [PATCH 07/66] Handle multiple weakrefs --- Include/cpython/weakrefobject.h | 11 ++ Objects/weakrefobject.c | 248 +++++++++++++++++++++++++++++++- 2 files changed, 256 insertions(+), 3 deletions(-) diff --git a/Include/cpython/weakrefobject.h b/Include/cpython/weakrefobject.h index 1559e2def61260..00d0f45ba2735e 100644 --- a/Include/cpython/weakrefobject.h +++ b/Include/cpython/weakrefobject.h @@ -2,6 +2,10 @@ # error "this header file must not be included directly" #endif +#ifdef Py_GIL_DISABLED +typedef struct _PyWeakRefUnlinker _PyWeakRefUnlinker; +#endif + /* PyWeakReference is the base struct for the Python ReferenceType, ProxyType, * and CallableProxyType. */ @@ -22,6 +26,13 @@ struct _PyWeakReference { */ Py_hash_t hash; +#ifdef Py_GIL_DISABLED + /* A pointer to an object that is used to coordinating concurrent attempts + * at unlinking the weakref. + */ + _PyWeakRefUnlinker *unlinker; +#endif + /* If wr_object is weakly referenced, wr_object has a doubly-linked NULL- * terminated list of weak references to it. These are the list pointers. * If wr_object goes away, wr_object is set to Py_None, and these pointers diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 9fbc845801fe43..13a173151122b3 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1,9 +1,103 @@ #include "Python.h" +#include "pycore_lock.h" #include "pycore_modsupport.h" // _PyArg_NoKwnames() #include "pycore_object.h" // _PyObject_GET_WEAKREFS_LISTPTR() #include "pycore_pyerrors.h" // _PyErr_ChainExceptions1() #include "pycore_weakref.h" // _PyWeakref_GET_REF() +#ifdef Py_GIL_DISABLED +/* + * Thread-safety for free-threaded builds + * ====================================== + * + * In free-threaded builds we need to protect mutable state of: + * + * - The weakref + * - The referenced object + * - The linked-list of weakrefs + * + * The above may be modified concurrently when creating weakrefs, destroying + * weakrefs, or destroying the referenced object. Critical sections are + * used to protect the mutable state: + * + * - The weakref is protected by its per-object lock. + * - The referenced object is protected by its per-object lock. + * - The linked-list of weakrefs is protected by the referenced object's + * per-object lock. + * - Both locks must be held (using the two-variant form of critical sections) + * if both the weakref and referenced object or linked-list need to be + * modified. + * + */ + +typedef enum { + WR_UNLINK_NOT_STARTED, + WR_UNLINK_STARTED, + WR_UNLINK_DONE, +} _PyWeakRefUnlinkState; + +/* + * _PyWeakRefUnlinker is used to coordinate concurrent attempts at unlinking + * weakrefs. + * + * XXX - More docs?? + * XXX - Can this be a OnceFlag? + */ +typedef struct _PyWeakRefUnlinker { + /* Holds a value from _PyWeakRefUnlinkState */ + int state; + PyEvent done; + Py_ssize_t refcount; +} _PyWeakRefUnlinker; + +static _PyWeakRefUnlinker * +_PyWeakRefUnlinker_New(void) +{ + _PyWeakRefUnlinker *self = (_PyWeakRefUnlinker *)PyMem_RawCalloc(1, sizeof(_PyWeakRefUnlinker)); + if (self == NULL) { + PyErr_NoMemory(); + return NULL; + } + self->state = WR_UNLINK_NOT_STARTED; + self->done = (PyEvent){0}; + self->refcount = 1; + return self; +} + +static void +_PyWeakRefUnlinker_Incref(_PyWeakRefUnlinker *self) +{ + _Py_atomic_add_ssize(&self->refcount, 1); +} + +static void +_PyWeakRefUnlinker_Decref(_PyWeakRefUnlinker *self) +{ + if (_Py_atomic_add_ssize(&self->refcount, -1) == 1) { + PyMem_RawFree(self); + } +} + +static int +_PyWeakRefUnlinker_Start(_PyWeakRefUnlinker *self) +{ + int expected = WR_UNLINK_NOT_STARTED; + return _Py_atomic_compare_exchange_int(&self->state, &expected, WR_UNLINK_STARTED); +} + +static void +_PyWeakRefUnlinker_Wait(_PyWeakRefUnlinker *self) +{ + PyEvent_Wait(&self->done); +} + +static void +_PyWeakRefUnlinker_Finish(_PyWeakRefUnlinker *self) +{ + _Py_atomic_exchange_int(&self->state, WR_UNLINK_DONE); + _PyEvent_Notify(&self->done); +} +#endif // Py_GIL_DISABLED #define GET_WEAKREFS_LISTPTR(o) \ @@ -24,7 +118,7 @@ _PyWeakref_GetWeakrefCount(PyWeakReference *head) static PyObject *weakref_vectorcall(PyObject *self, PyObject *const *args, size_t nargsf, PyObject *kwnames); -static void +static int init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) { self->hash = -1; @@ -34,9 +128,15 @@ init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) self->wr_callback = Py_XNewRef(callback); self->vectorcall = weakref_vectorcall; #ifdef Py_GIL_DISABLED + self->unlinker = _PyWeakRefUnlinker_New(); + if (self->unlinker == NULL) { + Py_XDECREF(self->wr_callback); + return -1; + } _PyObject_SetMaybeWeakref(ob); _PyObject_SetMaybeWeakref((PyObject *)self); #endif + return 0; } static PyWeakReference * @@ -46,12 +146,86 @@ new_weakref(PyObject *ob, PyObject *callback) result = PyObject_GC_New(PyWeakReference, &_PyWeakref_RefType); if (result) { - init_weakref(result, ob, callback); + if (init_weakref(result, ob, callback) < 0) { + return NULL; + } PyObject_GC_Track(result); } return result; } +#ifdef Py_GIL_DISABLED + +static void +unlink_weakref_lock_held(PyWeakReference *self) +{ + if (self->wr_object != Py_None) { + PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); + if (*list == self) + /* If 'self' is the end of the list (and thus self->wr_next == NULL) + then the weakref list itself (and thus the value of *list) will + end up being set to NULL. */ + *list = self->wr_next; + self->wr_object = Py_None; + if (self->wr_prev != NULL) + self->wr_prev->wr_next = self->wr_next; + if (self->wr_next != NULL) + self->wr_next->wr_prev = self->wr_prev; + self->wr_prev = NULL; + self->wr_next = NULL; + } +} + +static void +gc_unlink_weakref(PyWeakReference *self) +{ + unlink_weakref_lock_held(self); + _PyWeakRefUnlinker_Finish(self->unlinker); +} + +static void +unlink_weakref(PyWeakReference *self) +{ + Py_BEGIN_CRITICAL_SECTION2(self->wr_object, self); + unlink_weakref_lock_held(self); + Py_END_CRITICAL_SECTION2(); +} + +static void +clear_weakref(PyWeakReference *self) +{ + _PyWeakRefUnlinker *unlinker = self->unlinker; + _PyWeakRefUnlinker_Incref(unlinker); + + if (_PyWeakRefUnlinker_Start(unlinker)) { + unlink_weakref(self); + Py_BEGIN_CRITICAL_SECTION(self); + if (self->wr_callback != NULL) { + PyObject *callback = self->wr_callback; + self->wr_callback = NULL; + Py_DECREF(callback); + } + Py_END_CRITICAL_SECTION(); + _PyWeakRefUnlinker_Finish(unlinker); + } + else { + _PyWeakRefUnlinker_Wait(unlinker); + } + + _PyWeakRefUnlinker_Decref(unlinker); +} + +static void +gc_clear_weakref(PyWeakReference *self) +{ + gc_unlink_weakref(self); + if (self->wr_callback != NULL) { + Py_DECREF(self->wr_callback); + self->wr_callback = NULL; + } +} + +#else /* This function clears the passed-in reference and removes it from the * list of weak references for the referent. This is the only code that @@ -85,6 +259,8 @@ clear_weakref(PyWeakReference *self) } } +#endif // Py_GIL_DISABLED + /* Cyclic gc uses this to *just* clear the passed-in reference, leaving * the callback intact and uncalled. It must be possible to call self's * tp_dealloc() after calling this, so self has to be left in a sane enough @@ -106,7 +282,11 @@ _PyWeakref_ClearRef(PyWeakReference *self) /* Preserve and restore the callback around clear_weakref. */ callback = self->wr_callback; self->wr_callback = NULL; +#ifdef Py_GIL_DISABLED + gc_unlink_weakref(self); +#else clear_weakref(self); +#endif self->wr_callback = callback; } @@ -115,6 +295,9 @@ weakref_dealloc(PyObject *self) { PyObject_GC_UnTrack(self); clear_weakref((PyWeakReference *) self); +#ifdef Py_GIL_DISABLED + _PyWeakRefUnlinker_Decref(((PyWeakReference*)self)->unlinker); +#endif Py_TYPE(self)->tp_free(self); } @@ -130,7 +313,11 @@ gc_traverse(PyWeakReference *self, visitproc visit, void *arg) static int gc_clear(PyWeakReference *self) { +#ifdef Py_GIL_DISABLED + gc_clear_weakref(self); +#else clear_weakref(self); +#endif return 0; } @@ -324,7 +511,9 @@ new_weakref_lock_held(PyTypeObject *type, PyObject *ob, PyObject *callback) them. */ PyWeakReference *self = (PyWeakReference *) (type->tp_alloc(type, 0)); if (self != NULL) { - init_weakref(self, ob, callback); + if (init_weakref(self, ob, callback) < 0) { + return NULL; + } if (callback == NULL && type == &_PyWeakref_RefType) { insert_head(self, list); } @@ -976,6 +1165,7 @@ handle_callback(PyWeakReference *ref, PyObject *callback) Py_DECREF(cbresult); } + /* This function is called by the tp_dealloc handler to clear weak references. * * This iterates through the weak references for 'object' and calls callbacks @@ -995,6 +1185,57 @@ PyObject_ClearWeakRefs(PyObject *object) return; } list = GET_WEAKREFS_LISTPTR(object); +#ifdef Py_GIL_DISABLED + /* Protect the linked-list and the head pointer in object */ + Py_BEGIN_CRITICAL_SECTION(object); + + /* Remove the callback-less basic and proxy references, which always appear + at the head of the list. There may be two of each - one live and one in + the process of being destroyed. + */ + for (;;) { + if (*list != NULL && (*list)->wr_callback == NULL) { + clear_weakref(*list); + } + else { + break; + } + } + + /* Deal with non-canonical (subtypes or refs with callbacks) references. + At this point no new weakrefs to this object can be created, so we + should be safe to iterate until the list is empty. + */ + PyObject *exc = PyErr_GetRaisedException(); + while (*list != NULL) { + PyWeakReference *current = *list; + _PyWeakRefUnlinker *unlinker = current->unlinker; + _PyWeakRefUnlinker_Incref(unlinker); + if (_PyWeakRefUnlinker_Start(unlinker)) { + PyObject *callback; + Py_BEGIN_CRITICAL_SECTION(current); + callback = current->wr_callback; + current->wr_callback = NULL; + Py_END_CRITICAL_SECTION(); + unlink_weakref(current); + _PyWeakRefUnlinker_Finish(unlinker); + if (callback != NULL) { + if (_Py_TryIncref((PyObject **) ¤t, (PyObject *) current)) { + handle_callback(current, callback); + Py_DECREF(current); + } + Py_DECREF(callback); + } + } else { + _PyWeakRefUnlinker_Wait(unlinker); + } + _PyWeakRefUnlinker_Decref(unlinker); + } + assert(!PyErr_Occurred()); + PyErr_SetRaisedException(exc); + + Py_END_CRITICAL_SECTION(); +#else /* Remove the callback-less basic and proxy references */ if (*list != NULL && (*list)->wr_callback == NULL) { clear_weakref(*list); @@ -1056,6 +1297,7 @@ PyObject_ClearWeakRefs(PyObject *object) assert(!PyErr_Occurred()); PyErr_SetRaisedException(exc); } +#endif // Py_GIL_DISABLED } /* This function is called by _PyStaticType_Dealloc() to clear weak references. From c9b3bf97c0163ef329d5d05361eba0b01a2181fa Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 15 Mar 2024 17:03:05 -0700 Subject: [PATCH 08/66] Make _PyWeakref_GetWeakrefCount thread-safe --- Include/internal/pycore_weakref.h | 3 ++- Objects/weakrefobject.c | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index dea267b49039e7..151439ca3c7fc7 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -71,6 +71,8 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) return ret; } +// NB: In free-threaded builds the referenced object must be live for the +// duration of the call. extern Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head); extern void _PyWeakref_ClearRef(PyWeakReference *self); @@ -79,4 +81,3 @@ extern void _PyWeakref_ClearRef(PyWeakReference *self); } #endif #endif /* !Py_INTERNAL_WEAKREF_H */ - diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 13a173151122b3..5873b67879b2c3 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -108,11 +108,15 @@ Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head) { Py_ssize_t count = 0; - + if (head == NULL) { + return 0; + } + Py_BEGIN_CRITICAL_SECTION(head->wr_object); while (head != NULL) { ++count; head = head->wr_next; } + Py_END_CRITICAL_SECTION(); return count; } From b7b33153faf506564bab0b99f3c579779bfdd7d4 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 15 Mar 2024 17:50:14 -0700 Subject: [PATCH 09/66] Make _weakref._getweakrefs thread-safe --- Modules/_weakref.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/Modules/_weakref.c b/Modules/_weakref.c index 7225dbc9ce4a1b..5c034ec713e1c8 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -101,11 +101,41 @@ _weakref_getweakrefs_impl(PyObject *module, PyObject *object) return NULL; } +#ifdef Py_GIL_DISABLED + Py_ssize_t num_added = 0; + Py_BEGIN_CRITICAL_SECTION(object); + PyWeakReference *current = *list; + // Weakrefs may be added or removed since the count was computed. + while (num_added < count && current != NULL) { + if (_Py_TryIncref((PyObject**) ¤t, (PyObject *) current)) { + PyList_SET_ITEM(result, num_added, current); + num_added++; + } + current = current->wr_next; + } + Py_END_CRITICAL_SECTION(); + // Don't return an incomplete list + if (num_added != count) { + PyObject *new_list = PyList_New(num_added); + if (new_list == NULL) { + Py_DECREF(result); + return NULL; + } + for (Py_ssize_t i = 0; i < num_added; i++) { + PyObject *obj = PyList_GET_ITEM(result, i); + PyList_SET_ITEM(new_list, i, obj); + PyList_SET_ITEM(result, i, NULL); + } + Py_DECREF(result); + result = new_list; + } +#else PyWeakReference *current = *list; for (Py_ssize_t i = 0; i < count; ++i) { PyList_SET_ITEM(result, i, Py_NewRef(current)); current = current->wr_next; } +#endif return result; } From e6e2d281f3e52cbe002af9766eb26ddf3c9a60ee Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 15 Mar 2024 17:50:37 -0700 Subject: [PATCH 10/66] Make weakref clearing in subtype_dealloc thread-safe --- Include/internal/pycore_weakref.h | 4 ++++ Objects/weakrefobject.c | 39 +++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 151439ca3c7fc7..c67f2466b66681 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -75,6 +75,10 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) // duration of the call. extern Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head); +// Clear all the weak references to obj but leave their callbacks uncalled and +// intact. +extern void _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj); + extern void _PyWeakref_ClearRef(PyWeakReference *self); #ifdef __cplusplus diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 5873b67879b2c3..6d173f28e3fea6 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -219,6 +219,23 @@ clear_weakref(PyWeakReference *self) _PyWeakRefUnlinker_Decref(unlinker); } +static void +clear_weakref_only(PyWeakReference *self) +{ + _PyWeakRefUnlinker *unlinker = self->unlinker; + _PyWeakRefUnlinker_Incref(unlinker); + + if (_PyWeakRefUnlinker_Start(unlinker)) { + unlink_weakref(self); + _PyWeakRefUnlinker_Finish(unlinker); + } + else { + _PyWeakRefUnlinker_Wait(unlinker); + } + + _PyWeakRefUnlinker_Decref(unlinker); +} + static void gc_clear_weakref(PyWeakReference *self) { @@ -1324,3 +1341,25 @@ _PyStaticType_ClearWeakRefs(PyInterpreterState *interp, PyTypeObject *type) } Py_END_CRITICAL_SECTION(); } + +void +_PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj) +{ + /* Modeled after GET_WEAKREFS_LISTPTR(). + + This is never triggered for static types so we can avoid the + (slightly) more costly _PyObject_GET_WEAKREFS_LISTPTR(). */ + PyWeakReference **list = \ + _PyObject_GET_WEAKREFS_LISTPTR_FROM_OFFSET(obj); +#ifdef Py_GIL_DISABLED + Py_BEGIN_CRITICAL_SECTION(obj); + while (*list != NULL) { + clear_weakref_only((PyWeakReference *)*list); + } + Py_END_CRITICAL_SECTION(); +#else + while (*list) { + _PyWeakref_ClearRef(*list); + } +#endif +} From 0d259d186dc6f263b6544c366e678a12e90dc030 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 15 Mar 2024 18:01:35 -0700 Subject: [PATCH 11/66] Fix incorrect size in test_sys for weakref/proxy --- Lib/test/test_sys.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_sys.py b/Lib/test/test_sys.py index 37c16cd1047885..32fbe93e7831bc 100644 --- a/Lib/test/test_sys.py +++ b/Lib/test/test_sys.py @@ -12,7 +12,7 @@ import sysconfig import test.support from test import support -from test.support import os_helper +from test.support import os_helper, Py_GIL_DISABLED from test.support.script_helper import assert_python_ok, assert_python_failure from test.support import threading_helper from test.support import import_helper @@ -1707,11 +1707,15 @@ class newstyleclass(object): pass # TODO: add check that forces layout of unicodefields # weakref import weakref - check(weakref.ref(int), size('2Pn3P')) + if Py_GIL_DISABLED: + expected_size = size('2Pn4P') + else: + expected_size = size('2Pn3P') + check(weakref.ref(int), expected_size) # weakproxy # XXX # weakcallableproxy - check(weakref.proxy(int), size('2Pn3P')) + check(weakref.proxy(int), expected_size) def check_slots(self, obj, base, extra): expected = sys.getsizeof(base) + struct.calcsize(extra) From 7cc523b412f67a10099fbee3072bcdc68ee021ac Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 18 Mar 2024 16:29:00 -0700 Subject: [PATCH 12/66] Use a once flag --- Include/cpython/weakrefobject.h | 8 +- Include/internal/pycore_lock.h | 11 ++ Objects/weakrefobject.c | 214 ++++++++++++-------------------- Python/lock.c | 26 ++++ 4 files changed, 119 insertions(+), 140 deletions(-) diff --git a/Include/cpython/weakrefobject.h b/Include/cpython/weakrefobject.h index 00d0f45ba2735e..df9ba417d98e00 100644 --- a/Include/cpython/weakrefobject.h +++ b/Include/cpython/weakrefobject.h @@ -3,7 +3,7 @@ #endif #ifdef Py_GIL_DISABLED -typedef struct _PyWeakRefUnlinker _PyWeakRefUnlinker; +struct _PyOnceFlagRC; #endif /* PyWeakReference is the base struct for the Python ReferenceType, ProxyType, @@ -27,10 +27,10 @@ struct _PyWeakReference { Py_hash_t hash; #ifdef Py_GIL_DISABLED - /* A pointer to an object that is used to coordinating concurrent attempts - * at unlinking the weakref. + /* Used in free-threaded builds to ensure that a weakref is only cleared + * once. */ - _PyWeakRefUnlinker *unlinker; + struct _PyOnceFlagRC *clear_once; #endif /* If wr_object is weakly referenced, wr_object has a doubly-linked NULL- diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index f993c95ecbf75a..3551f0a7101efb 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -217,6 +217,17 @@ _PyOnceFlag_CallOnce(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg) return _PyOnceFlag_CallOnceSlow(flag, fn, arg); } +// A refcounted flag +typedef struct _PyOnceFlagRC { + _PyOnceFlag flag; + + Py_ssize_t refcount; +} _PyOnceFlagRC; + +PyAPI_FUNC(_PyOnceFlagRC *) _PyOnceFlagRC_New(void); +PyAPI_FUNC(void) _PyOnceFlagRC_Incref(_PyOnceFlagRC *flag); +PyAPI_FUNC(void) _PyOnceFlagRC_Decref(_PyOnceFlagRC *flag); + // A readers-writer (RW) lock. The lock supports multiple concurrent readers or // a single writer. The lock is write-preferring: if a writer is waiting while // the lock is read-locked then, new readers will be blocked. This avoids diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 6d173f28e3fea6..918e01938cf4ed 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -5,7 +5,6 @@ #include "pycore_pyerrors.h" // _PyErr_ChainExceptions1() #include "pycore_weakref.h" // _PyWeakref_GET_REF() -#ifdef Py_GIL_DISABLED /* * Thread-safety for free-threaded builds * ====================================== @@ -30,76 +29,6 @@ * */ -typedef enum { - WR_UNLINK_NOT_STARTED, - WR_UNLINK_STARTED, - WR_UNLINK_DONE, -} _PyWeakRefUnlinkState; - -/* - * _PyWeakRefUnlinker is used to coordinate concurrent attempts at unlinking - * weakrefs. - * - * XXX - More docs?? - * XXX - Can this be a OnceFlag? - */ -typedef struct _PyWeakRefUnlinker { - /* Holds a value from _PyWeakRefUnlinkState */ - int state; - PyEvent done; - Py_ssize_t refcount; -} _PyWeakRefUnlinker; - -static _PyWeakRefUnlinker * -_PyWeakRefUnlinker_New(void) -{ - _PyWeakRefUnlinker *self = (_PyWeakRefUnlinker *)PyMem_RawCalloc(1, sizeof(_PyWeakRefUnlinker)); - if (self == NULL) { - PyErr_NoMemory(); - return NULL; - } - self->state = WR_UNLINK_NOT_STARTED; - self->done = (PyEvent){0}; - self->refcount = 1; - return self; -} - -static void -_PyWeakRefUnlinker_Incref(_PyWeakRefUnlinker *self) -{ - _Py_atomic_add_ssize(&self->refcount, 1); -} - -static void -_PyWeakRefUnlinker_Decref(_PyWeakRefUnlinker *self) -{ - if (_Py_atomic_add_ssize(&self->refcount, -1) == 1) { - PyMem_RawFree(self); - } -} - -static int -_PyWeakRefUnlinker_Start(_PyWeakRefUnlinker *self) -{ - int expected = WR_UNLINK_NOT_STARTED; - return _Py_atomic_compare_exchange_int(&self->state, &expected, WR_UNLINK_STARTED); -} - -static void -_PyWeakRefUnlinker_Wait(_PyWeakRefUnlinker *self) -{ - PyEvent_Wait(&self->done); -} - -static void -_PyWeakRefUnlinker_Finish(_PyWeakRefUnlinker *self) -{ - _Py_atomic_exchange_int(&self->state, WR_UNLINK_DONE); - _PyEvent_Notify(&self->done); -} -#endif // Py_GIL_DISABLED - - #define GET_WEAKREFS_LISTPTR(o) \ ((PyWeakReference **) _PyObject_GET_WEAKREFS_LISTPTR(o)) @@ -132,8 +61,8 @@ init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) self->wr_callback = Py_XNewRef(callback); self->vectorcall = weakref_vectorcall; #ifdef Py_GIL_DISABLED - self->unlinker = _PyWeakRefUnlinker_New(); - if (self->unlinker == NULL) { + self->clear_once = _PyOnceFlagRC_New(); + if (self->clear_once == NULL) { Py_XDECREF(self->wr_callback); return -1; } @@ -180,70 +109,95 @@ unlink_weakref_lock_held(PyWeakReference *self) } } +static void +clear_callback_lock_held(PyWeakReference *self) +{ + if (self->wr_callback != NULL) { + PyObject *callback = self->wr_callback; + self->wr_callback = NULL; + Py_DECREF(callback); + } +} + static void gc_unlink_weakref(PyWeakReference *self) { unlink_weakref_lock_held(self); - _PyWeakRefUnlinker_Finish(self->unlinker); } -static void -unlink_weakref(PyWeakReference *self) +static int +do_clear_weakref(PyWeakReference *self) { Py_BEGIN_CRITICAL_SECTION2(self->wr_object, self); unlink_weakref_lock_held(self); + clear_callback_lock_held(self); Py_END_CRITICAL_SECTION2(); + return 0; } static void clear_weakref(PyWeakReference *self) { - _PyWeakRefUnlinker *unlinker = self->unlinker; - _PyWeakRefUnlinker_Incref(unlinker); - - if (_PyWeakRefUnlinker_Start(unlinker)) { - unlink_weakref(self); - Py_BEGIN_CRITICAL_SECTION(self); - if (self->wr_callback != NULL) { - PyObject *callback = self->wr_callback; - self->wr_callback = NULL; - Py_DECREF(callback); - } - Py_END_CRITICAL_SECTION(); - _PyWeakRefUnlinker_Finish(unlinker); - } - else { - _PyWeakRefUnlinker_Wait(unlinker); - } + _PyOnceFlagRC *once = self->clear_once; + _PyOnceFlagRC_Incref(once); + _PyOnceFlag_CallOnce(&once->flag, (_Py_once_fn_t *) do_clear_weakref, self); + _PyOnceFlagRC_Decref(once); +} + +static void +gc_clear_weakref(PyWeakReference *self) +{ + unlink_weakref_lock_held(self); + clear_callback_lock_held(self); +} - _PyWeakRefUnlinker_Decref(unlinker); +static int +do_clear_weakref_only(PyWeakReference *self) +{ + Py_BEGIN_CRITICAL_SECTION2(self->wr_object, self); + unlink_weakref_lock_held(self); + Py_END_CRITICAL_SECTION2(); + return 0; } static void clear_weakref_only(PyWeakReference *self) { - _PyWeakRefUnlinker *unlinker = self->unlinker; - _PyWeakRefUnlinker_Incref(unlinker); + _PyOnceFlagRC *once = self->clear_once; + _PyOnceFlagRC_Incref(once); + _PyOnceFlag_CallOnce(&once->flag, (_Py_once_fn_t *) do_clear_weakref_only, self); + _PyOnceFlagRC_Decref(once); +} - if (_PyWeakRefUnlinker_Start(unlinker)) { - unlink_weakref(self); - _PyWeakRefUnlinker_Finish(unlinker); - } - else { - _PyWeakRefUnlinker_Wait(unlinker); - } +typedef struct { + PyWeakReference *weakref; + PyObject *callback; +} ClearArgs; - _PyWeakRefUnlinker_Decref(unlinker); +static int +do_clear_weakref_and_take_callback(ClearArgs *args) +{ + PyWeakReference *weakref = args->weakref; + Py_BEGIN_CRITICAL_SECTION2(weakref->wr_object, weakref); + unlink_weakref_lock_held(weakref); + args->callback = weakref->wr_callback; + weakref->wr_callback = NULL; + Py_END_CRITICAL_SECTION2(); + return 0; } -static void -gc_clear_weakref(PyWeakReference *self) +static PyObject * +clear_weakref_and_take_callback(PyWeakReference *self) { - gc_unlink_weakref(self); - if (self->wr_callback != NULL) { - Py_DECREF(self->wr_callback); - self->wr_callback = NULL; - } + ClearArgs args = { + .weakref = self, + .callback = NULL, + }; + _PyOnceFlagRC *once = self->clear_once; + _PyOnceFlagRC_Incref(once); + _PyOnceFlag_CallOnce(&once->flag, (_Py_once_fn_t *) do_clear_weakref_and_take_callback, &args); + _PyOnceFlagRC_Decref(once); + return args.callback; } #else @@ -317,7 +271,7 @@ weakref_dealloc(PyObject *self) PyObject_GC_UnTrack(self); clear_weakref((PyWeakReference *) self); #ifdef Py_GIL_DISABLED - _PyWeakRefUnlinker_Decref(((PyWeakReference*)self)->unlinker); + _PyOnceFlagRC_Decref(((PyWeakReference*)self)->clear_once); #endif Py_TYPE(self)->tp_free(self); } @@ -1186,7 +1140,6 @@ handle_callback(PyWeakReference *ref, PyObject *callback) Py_DECREF(cbresult); } - /* This function is called by the tp_dealloc handler to clear weak references. * * This iterates through the weak references for 'object' and calls callbacks @@ -1207,7 +1160,11 @@ PyObject_ClearWeakRefs(PyObject *object) } list = GET_WEAKREFS_LISTPTR(object); #ifdef Py_GIL_DISABLED - /* Protect the linked-list and the head pointer in object */ + /* Protect the linked-list and the head pointer in object. + + At this point no new weakrefs to object can be created, so we should be + safe to iterate until the list is empty. + */ Py_BEGIN_CRITICAL_SECTION(object); /* Remove the callback-less basic and proxy references, which always appear @@ -1223,34 +1180,19 @@ PyObject_ClearWeakRefs(PyObject *object) } } - /* Deal with non-canonical (subtypes or refs with callbacks) references. - At this point no new weakrefs to this object can be created, so we - should be safe to iterate until the list is empty. - */ + /* Deal with non-canonical (subtypes or refs with callbacks) references. */ PyObject *exc = PyErr_GetRaisedException(); while (*list != NULL) { - PyWeakReference *current = *list; - _PyWeakRefUnlinker *unlinker = current->unlinker; - _PyWeakRefUnlinker_Incref(unlinker); - if (_PyWeakRefUnlinker_Start(unlinker)) { - PyObject *callback; - Py_BEGIN_CRITICAL_SECTION(current); - callback = current->wr_callback; - current->wr_callback = NULL; - Py_END_CRITICAL_SECTION(); - unlink_weakref(current); - _PyWeakRefUnlinker_Finish(unlinker); + PyWeakReference *cur = *list; + int live = _Py_TryIncref((PyObject **) &cur, (PyObject *) cur); + PyObject *callback = clear_weakref_and_take_callback(cur); + if (live) { if (callback != NULL) { - if (_Py_TryIncref((PyObject **) ¤t, (PyObject *) current)) { - handle_callback(current, callback); - Py_DECREF(current); - } + handle_callback(cur, callback); Py_DECREF(callback); } - } else { - _PyWeakRefUnlinker_Wait(unlinker); + Py_DECREF(cur); } - _PyWeakRefUnlinker_Decref(unlinker); } assert(!PyErr_Occurred()); PyErr_SetRaisedException(exc); diff --git a/Python/lock.c b/Python/lock.c index 7d1ead585dee6c..f78c0879a90b02 100644 --- a/Python/lock.c +++ b/Python/lock.c @@ -362,6 +362,32 @@ _PyOnceFlag_CallOnceSlow(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg) } } +_PyOnceFlagRC * +_PyOnceFlagRC_New(void) +{ + _PyOnceFlagRC *self = (_PyOnceFlagRC *)PyMem_RawCalloc(1, sizeof(_PyOnceFlagRC)); + if (self == NULL) { + PyErr_NoMemory(); + return NULL; + } + self->refcount = 1; + return self; +} + +void +_PyOnceFlagRC_Incref(_PyOnceFlagRC *self) +{ + _Py_atomic_add_ssize(&self->refcount, 1); +} + +void +_PyOnceFlagRC_Decref(_PyOnceFlagRC *self) +{ + if (_Py_atomic_add_ssize(&self->refcount, -1) == 1) { + PyMem_RawFree(self); + } +} + #define _Py_WRITE_LOCKED 1 #define _PyRWMutex_READER_SHIFT 2 #define _Py_RWMUTEX_MAX_READERS (UINTPTR_MAX >> _PyRWMutex_READER_SHIFT) From 11be247ebe95637ab2f467892e8de5df834ca7c3 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 18 Mar 2024 16:34:02 -0700 Subject: [PATCH 13/66] Simplify clearing callback-free refs --- Objects/weakrefobject.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 918e01938cf4ed..e97db43ceeba99 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1171,13 +1171,8 @@ PyObject_ClearWeakRefs(PyObject *object) at the head of the list. There may be two of each - one live and one in the process of being destroyed. */ - for (;;) { - if (*list != NULL && (*list)->wr_callback == NULL) { - clear_weakref(*list); - } - else { - break; - } + while (*list != NULL && (*list)->wr_callback == NULL) { + clear_weakref(*list); } /* Deal with non-canonical (subtypes or refs with callbacks) references. */ From 8fcd33ffee1b540580894fec674865f54e1f2890 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 18 Mar 2024 16:52:43 -0700 Subject: [PATCH 14/66] Rationalize naming --- Objects/weakrefobject.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index e97db43ceeba99..4158e4cd6fdf86 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -90,7 +90,7 @@ new_weakref(PyObject *ob, PyObject *callback) #ifdef Py_GIL_DISABLED static void -unlink_weakref_lock_held(PyWeakReference *self) +unlink_and_clear_object_lock_held(PyWeakReference *self) { if (self->wr_object != Py_None) { PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); @@ -119,22 +119,17 @@ clear_callback_lock_held(PyWeakReference *self) } } -static void -gc_unlink_weakref(PyWeakReference *self) -{ - unlink_weakref_lock_held(self); -} - static int do_clear_weakref(PyWeakReference *self) { Py_BEGIN_CRITICAL_SECTION2(self->wr_object, self); - unlink_weakref_lock_held(self); + unlink_and_clear_object_lock_held(self); clear_callback_lock_held(self); Py_END_CRITICAL_SECTION2(); return 0; } +// Atomically clear the weakref static void clear_weakref(PyWeakReference *self) { @@ -144,28 +139,30 @@ clear_weakref(PyWeakReference *self) _PyOnceFlagRC_Decref(once); } +// Clear the weakref while the world is stopped static void gc_clear_weakref(PyWeakReference *self) { - unlink_weakref_lock_held(self); + unlink_and_clear_object_lock_held(self); clear_callback_lock_held(self); } static int -do_clear_weakref_only(PyWeakReference *self) +do_clear_weakref_and_leave_callback(PyWeakReference *self) { Py_BEGIN_CRITICAL_SECTION2(self->wr_object, self); - unlink_weakref_lock_held(self); + unlink_and_clear_object_lock_held(self); Py_END_CRITICAL_SECTION2(); return 0; } +// Atomically clear the weakref, but leave the callback intact static void -clear_weakref_only(PyWeakReference *self) +clear_weakref_and_leave_callback(PyWeakReference *self) { _PyOnceFlagRC *once = self->clear_once; _PyOnceFlagRC_Incref(once); - _PyOnceFlag_CallOnce(&once->flag, (_Py_once_fn_t *) do_clear_weakref_only, self); + _PyOnceFlag_CallOnce(&once->flag, (_Py_once_fn_t *) do_clear_weakref_and_leave_callback, self); _PyOnceFlagRC_Decref(once); } @@ -179,13 +176,14 @@ do_clear_weakref_and_take_callback(ClearArgs *args) { PyWeakReference *weakref = args->weakref; Py_BEGIN_CRITICAL_SECTION2(weakref->wr_object, weakref); - unlink_weakref_lock_held(weakref); + unlink_and_clear_object_lock_held(weakref); args->callback = weakref->wr_callback; weakref->wr_callback = NULL; Py_END_CRITICAL_SECTION2(); return 0; } +// Clear the weakref and return the a strong reference to the callback, if any static PyObject * clear_weakref_and_take_callback(PyWeakReference *self) { @@ -258,7 +256,7 @@ _PyWeakref_ClearRef(PyWeakReference *self) callback = self->wr_callback; self->wr_callback = NULL; #ifdef Py_GIL_DISABLED - gc_unlink_weakref(self); + unlink_and_clear_object_lock_held(self); #else clear_weakref(self); #endif @@ -1291,7 +1289,7 @@ _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj) #ifdef Py_GIL_DISABLED Py_BEGIN_CRITICAL_SECTION(obj); while (*list != NULL) { - clear_weakref_only((PyWeakReference *)*list); + clear_weakref_and_leave_callback((PyWeakReference *)*list); } Py_END_CRITICAL_SECTION(); #else From 1e1e609fd0ba61512e7527f71bfe881db4432de3 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 18 Mar 2024 16:52:56 -0700 Subject: [PATCH 15/66] Fix warning --- Objects/weakrefobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 4158e4cd6fdf86..4e0e92ec6f3131 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -472,7 +472,7 @@ new_weakref_lock_held(PyTypeObject *type, PyObject *ob, PyObject *callback) #else if (ref != NULL) { /* We can re-use an existing reference. */ - return Py_NewRef(ref); + return (PyWeakReference *) Py_NewRef(ref); } #endif } From 19a274e4e1e8f087a753e2f445e6c5f87169b471 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 18 Mar 2024 17:08:42 -0700 Subject: [PATCH 16/66] Simplify ClearWeakRefs --- Objects/weakrefobject.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 4e0e92ec6f3131..210ad0b340a602 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -248,19 +248,17 @@ clear_weakref(PyWeakReference *self) void _PyWeakref_ClearRef(PyWeakReference *self) { - PyObject *callback; - assert(self != NULL); assert(PyWeakref_Check(self)); - /* Preserve and restore the callback around clear_weakref. */ - callback = self->wr_callback; - self->wr_callback = NULL; #ifdef Py_GIL_DISABLED unlink_and_clear_object_lock_held(self); #else + /* Preserve and restore the callback around clear_weakref. */ + PyObject *callback = self->wr_callback; + self->wr_callback = NULL; clear_weakref(self); -#endif self->wr_callback = callback; +#endif } static void From 817a5a3f5f4caf880c76edffc2aacc936ee90553 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 19 Mar 2024 11:22:14 -0700 Subject: [PATCH 17/66] Refactor clearing code --- Objects/weakrefobject.c | 46 ++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 210ad0b340a602..2f62d6ae0e72a5 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -109,34 +109,12 @@ unlink_and_clear_object_lock_held(PyWeakReference *self) } } -static void +static PyObject * clear_callback_lock_held(PyWeakReference *self) { - if (self->wr_callback != NULL) { - PyObject *callback = self->wr_callback; - self->wr_callback = NULL; - Py_DECREF(callback); - } -} - -static int -do_clear_weakref(PyWeakReference *self) -{ - Py_BEGIN_CRITICAL_SECTION2(self->wr_object, self); - unlink_and_clear_object_lock_held(self); - clear_callback_lock_held(self); - Py_END_CRITICAL_SECTION2(); - return 0; -} - -// Atomically clear the weakref -static void -clear_weakref(PyWeakReference *self) -{ - _PyOnceFlagRC *once = self->clear_once; - _PyOnceFlagRC_Incref(once); - _PyOnceFlag_CallOnce(&once->flag, (_Py_once_fn_t *) do_clear_weakref, self); - _PyOnceFlagRC_Decref(once); + PyObject *callback = self->wr_callback; + self->wr_callback = NULL; + return callback; } // Clear the weakref while the world is stopped @@ -144,7 +122,8 @@ static void gc_clear_weakref(PyWeakReference *self) { unlink_and_clear_object_lock_held(self); - clear_callback_lock_held(self); + PyObject *callback = clear_callback_lock_held(self); + Py_XDECREF(callback); } static int @@ -156,7 +135,7 @@ do_clear_weakref_and_leave_callback(PyWeakReference *self) return 0; } -// Atomically clear the weakref, but leave the callback intact +// Clear the weakref, but leave the callback intact static void clear_weakref_and_leave_callback(PyWeakReference *self) { @@ -177,8 +156,7 @@ do_clear_weakref_and_take_callback(ClearArgs *args) PyWeakReference *weakref = args->weakref; Py_BEGIN_CRITICAL_SECTION2(weakref->wr_object, weakref); unlink_and_clear_object_lock_held(weakref); - args->callback = weakref->wr_callback; - weakref->wr_callback = NULL; + args->callback = clear_callback_lock_held(args->weakref); Py_END_CRITICAL_SECTION2(); return 0; } @@ -198,6 +176,14 @@ clear_weakref_and_take_callback(PyWeakReference *self) return args.callback; } +// Clear the weakref and its callback +static void +clear_weakref(PyWeakReference *self) +{ + PyObject *callback = clear_weakref_and_take_callback(self); + Py_XDECREF(callback); +} + #else /* This function clears the passed-in reference and removes it from the From d493e7d03f90c5e9d18256d107f2764671b5f413 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 20 Mar 2024 13:45:01 -0700 Subject: [PATCH 18/66] Protect wr_object with a separate mutex --- Include/cpython/object.h | 7 + Include/cpython/weakrefobject.h | 10 +- Include/internal/pycore_lock.h | 11 - Include/internal/pycore_weakref.h | 30 ++- Objects/object.c | 19 ++ Objects/typeobject.c | 1 + Objects/weakrefobject.c | 324 +++++++++++++++++++++++------- Python/lock.c | 26 --- 8 files changed, 307 insertions(+), 121 deletions(-) diff --git a/Include/cpython/object.h b/Include/cpython/object.h index 7512bb70c760fd..f6b25c47fd345a 100644 --- a/Include/cpython/object.h +++ b/Include/cpython/object.h @@ -465,6 +465,13 @@ PyAPI_FUNC(void) _PyTrash_end(PyThreadState *tstate); /* Python 3.10 private API, invoked by the Py_TRASHCAN_BEGIN(). */ PyAPI_FUNC(int) _PyTrash_cond(PyObject *op, destructor dealloc); +#ifdef Py_GIL_DISABLED + +/* Private API, used by weakrefs in free-threaded builds */ +PyAPI_FUNC(int) _PyTrash_contains(PyObject *op); + +#endif + #define Py_TRASHCAN_BEGIN_CONDITION(op, cond) \ do { \ PyThreadState *_tstate = NULL; \ diff --git a/Include/cpython/weakrefobject.h b/Include/cpython/weakrefobject.h index df9ba417d98e00..6faaf6f8620eb4 100644 --- a/Include/cpython/weakrefobject.h +++ b/Include/cpython/weakrefobject.h @@ -2,10 +2,6 @@ # error "this header file must not be included directly" #endif -#ifdef Py_GIL_DISABLED -struct _PyOnceFlagRC; -#endif - /* PyWeakReference is the base struct for the Python ReferenceType, ProxyType, * and CallableProxyType. */ @@ -27,10 +23,8 @@ struct _PyWeakReference { Py_hash_t hash; #ifdef Py_GIL_DISABLED - /* Used in free-threaded builds to ensure that a weakref is only cleared - * once. - */ - struct _PyOnceFlagRC *clear_once; + /* Used in free-threaded builds to protect wr_object and wr_callback. */ + struct _PyWeakRefClearState *clear_state; #endif /* If wr_object is weakly referenced, wr_object has a doubly-linked NULL- diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index 3551f0a7101efb..f993c95ecbf75a 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -217,17 +217,6 @@ _PyOnceFlag_CallOnce(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg) return _PyOnceFlag_CallOnceSlow(flag, fn, arg); } -// A refcounted flag -typedef struct _PyOnceFlagRC { - _PyOnceFlag flag; - - Py_ssize_t refcount; -} _PyOnceFlagRC; - -PyAPI_FUNC(_PyOnceFlagRC *) _PyOnceFlagRC_New(void); -PyAPI_FUNC(void) _PyOnceFlagRC_Incref(_PyOnceFlagRC *flag); -PyAPI_FUNC(void) _PyOnceFlagRC_Decref(_PyOnceFlagRC *flag); - // A readers-writer (RW) lock. The lock supports multiple concurrent readers or // a single writer. The lock is write-preferring: if a writer is waiting while // the lock is read-locked then, new readers will be blocked. This avoids diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index c67f2466b66681..dcf99af058c90a 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -8,9 +8,18 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif -#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION() +#include "pycore_lock.h" #include "pycore_object.h" // _Py_REF_IS_MERGED() +#ifdef Py_GIL_DISABLED +typedef struct _PyWeakRefClearState { + PyMutex mutex; + _PyOnceFlag once; + int cleared; + Py_ssize_t refcount; +} _PyWeakRefClearState; +#endif + static inline int _is_dead(PyObject *obj) { // Explanation for the Py_REFCNT() check: when a weakref's target is part @@ -27,12 +36,23 @@ static inline int _is_dead(PyObject *obj) #endif } +#if defined(Py_GIL_DISABLED) +# define LOCK_WR_OBJECT(weakref) PyMutex_Lock(&(weakref)->clear_state->mutex); +# define UNLOCK_WR_OBJECT(weakref) PyMutex_Unlock(&(weakref)->clear_state->mutex); +#else +# define LOCK_WR_OBJECT(weakref) +# define UNLOCK_WR_OBJECT(weakref) +#endif + +// NB: In free-threaded builds nothing between the LOCK/UNLOCK calls below can +// suspend. + static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) { assert(PyWeakref_Check(ref_obj)); PyObject *ret = NULL; - Py_BEGIN_CRITICAL_SECTION(ref_obj); PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); + LOCK_WR_OBJECT(ref) PyObject *obj = ref->wr_object; if (obj == Py_None) { @@ -48,7 +68,7 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) #endif ret = Py_NewRef(obj); end: - Py_END_CRITICAL_SECTION(); + UNLOCK_WR_OBJECT(ref); return ret; } @@ -56,8 +76,8 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) { assert(PyWeakref_Check(ref_obj)); int ret = 0; - Py_BEGIN_CRITICAL_SECTION(ref_obj); PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); + LOCK_WR_OBJECT(ref); PyObject *obj = ref->wr_object; if (obj == Py_None) { // clear_weakref() was called @@ -67,7 +87,7 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) // See _PyWeakref_GET_REF() for the rationale of this test ret = _is_dead(obj); } - Py_END_CRITICAL_SECTION(); + UNLOCK_WR_OBJECT(ref); return ret; } diff --git a/Objects/object.c b/Objects/object.c index b4f0fd4d7db941..5efaa28a4c737c 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2835,6 +2835,25 @@ _PyTrash_cond(PyObject *op, destructor dealloc) return Py_TYPE(op)->tp_dealloc == dealloc; } +#ifdef Py_GIL_DISABLED + +int +_PyTrash_contains(PyObject *op) +{ + PyThreadState *tstate = _PyThreadState_GET(); + struct _py_trashcan *trash = _PyTrash_get_state(tstate); + PyObject *cur = trash->delete_later; + while (cur) { + if (cur == op) { + return 1; + } + cur = (PyObject *) cur->ob_tid; + } + return 0; +} + +#endif + void _Py_NO_RETURN _PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg, diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 82822784aaf407..d66b539d85c362 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -4,6 +4,7 @@ #include "pycore_abstract.h" // _PySequence_IterSearch() #include "pycore_call.h" // _PyObject_VectorcallTstate() #include "pycore_code.h" // CO_FAST_FREE +#include "pycore_critical_section.h" #include "pycore_dict.h" // _PyDict_KeysSize() #include "pycore_frame.h" // _PyInterpreterFrame #include "pycore_lock.h" // _PySeqLock_* diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 2f62d6ae0e72a5..6271e3f09b8416 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1,34 +1,114 @@ #include "Python.h" +#include "pycore_critical_section.h" #include "pycore_lock.h" #include "pycore_modsupport.h" // _PyArg_NoKwnames() #include "pycore_object.h" // _PyObject_GET_WEAKREFS_LISTPTR() #include "pycore_pyerrors.h" // _PyErr_ChainExceptions1() #include "pycore_weakref.h" // _PyWeakref_GET_REF() +#ifdef Py_GIL_DISABLED /* * Thread-safety for free-threaded builds * ====================================== * * In free-threaded builds we need to protect mutable state of: * - * - The weakref - * - The referenced object + * - The weakref (the referenced object, the hash) + * - The referenced object (its head-of-list pointer) * - The linked-list of weakrefs * * The above may be modified concurrently when creating weakrefs, destroying - * weakrefs, or destroying the referenced object. Critical sections are - * used to protect the mutable state: + * weakrefs, or destroying the referenced object. A combination of techniques + * are used to ensure it is accessed safely: * - * - The weakref is protected by its per-object lock. - * - The referenced object is protected by its per-object lock. - * - The linked-list of weakrefs is protected by the referenced object's + * - The weakref's hash is protected using a critical section on the weakref's * per-object lock. - * - Both locks must be held (using the two-variant form of critical sections) - * if both the weakref and referenced object or linked-list need to be - * modified. + * - The referenced object and the callback are protected by a separate mutex + * in the weakref, the `clear_mutex`. + * - The linked-list of weakrefs and the object's head-of-list pointer are + * protected using a critical section on the referenced object's per-object + * lock. + * - Concurrent attempts to clear the weakref (i.e. from the referenced object + * and the weakref) cooperate using the protocol described below. + * + * Destroying the referenced object + * -------------------------------- + * When the referenced object is destroyed it must clear the borrowed reference + * in each weakref and unlink the weakref from the linked-list: + * + * 1. Acquire a critical section on the referenced object's per-object lock. + * 2. If the linked-list is empty, release the critical section and stop. + * 3. For the first object in the list, incref its `clear_state`. + * 4. Release the critical section. + * 5. Call the once flag. If we enter: + * a. Acquire a CS on the referenced object's per-object lock. + * b. Lock the `clear_mutex`. Note that this is not a critical section, so we + * must be careful not to do anything that could suspend while it is held. + * c. Unlink the weakref from the list. + * d. Set `wr_object` to Py_None. + * e. Unlock the `clear_mutex`. + * f. Release the CS. + * 6. Goto (1). + * + * Destroying a weakref + * -------------------- + * The weakref must unlink itself from the linked-list and clear its borrowed + * reference when destroyed: * + * 1. Call its once flag. If we enter: + * a. Try to incref the referenced object. We must hold a strong reference to it + * before attempting to acquire the critical section. Otherwise, if it's + * cyclic garbage it may be destroyed if we're suspended when we acquire the + * CS on its per-object lock below. + * b. If (1a) fails, return -1. + * c. Acquire a CS on the referenced object's per-object lock. + * d. Lock `clear_mutex`. + * e. Unlink the weakref from the list. + * f. Set `wr_object` to Py_None. + * g. Unlock the `clear_mutex`. + * h. Release the CS. + * 2. If (1) failed, retry. + * + * Garbage Collection + * ------------------ + * If the GC determines that an object is cyclic garbage it needs to go through + * and clear any weakrefs to the object. Similarly, if the the GC determines + * that a weakref is garbage it must also be cleared. Thankfully, the world is + * stopped while GC is running, so it can just clear each weakref without + * resorting to the complex protocol above. */ +_PyWeakRefClearState * +_PyWeakRefClearState_New(void) +{ + _PyWeakRefClearState *self = (_PyWeakRefClearState *)PyMem_RawCalloc(1, sizeof(_PyWeakRefClearState)); + if (self == NULL) { + PyErr_NoMemory(); + return NULL; + } + self->mutex = (PyMutex){0}; + self->once = (_PyOnceFlag){0}; + self->cleared = 0; + self->refcount = 1; + return self; +} + +void +_PyWeakRefClearState_Incref(_PyWeakRefClearState *self) +{ + _Py_atomic_add_ssize(&self->refcount, 1); +} + +void +_PyWeakRefClearState_Decref(_PyWeakRefClearState *self) +{ + if (_Py_atomic_add_ssize(&self->refcount, -1) == 1) { + PyMem_RawFree(self); + } +} + +#endif + #define GET_WEAKREFS_LISTPTR(o) \ ((PyWeakReference **) _PyObject_GET_WEAKREFS_LISTPTR(o)) @@ -61,8 +141,8 @@ init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) self->wr_callback = Py_XNewRef(callback); self->vectorcall = weakref_vectorcall; #ifdef Py_GIL_DISABLED - self->clear_once = _PyOnceFlagRC_New(); - if (self->clear_once == NULL) { + self->clear_state = _PyWeakRefClearState_New(); + if (self->clear_state == NULL) { Py_XDECREF(self->wr_callback); return -1; } @@ -89,9 +169,11 @@ new_weakref(PyObject *ob, PyObject *callback) #ifdef Py_GIL_DISABLED +// Clear the weakref and steal its callback into `callback`, if provided. static void -unlink_and_clear_object_lock_held(PyWeakReference *self) +clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) { + // TODO: Assert locks are held or world is stopped if (self->wr_object != Py_None) { PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); if (*list == self) @@ -107,73 +189,139 @@ unlink_and_clear_object_lock_held(PyWeakReference *self) self->wr_prev = NULL; self->wr_next = NULL; } -} - -static PyObject * -clear_callback_lock_held(PyWeakReference *self) -{ - PyObject *callback = self->wr_callback; - self->wr_callback = NULL; - return callback; + if (callback != NULL) { + *callback = self->wr_callback; + self->wr_callback = NULL; + } + self->clear_state->cleared = 1; } // Clear the weakref while the world is stopped static void gc_clear_weakref(PyWeakReference *self) { - unlink_and_clear_object_lock_held(self); - PyObject *callback = clear_callback_lock_held(self); + PyObject *callback = NULL; + clear_weakref_lock_held(self, &callback); Py_XDECREF(callback); } +typedef struct { + PyWeakReference *self; + + // A strong reference to the callback in self (out param) + PyObject *callback; + + // A strong reference to the referenced object in self (out param) + PyObject *object; +} DeallocClearArgs; + static int -do_clear_weakref_and_leave_callback(PyWeakReference *self) +try_clear_weakref_for_wr_dealloc(DeallocClearArgs *args) { - Py_BEGIN_CRITICAL_SECTION2(self->wr_object, self); - unlink_and_clear_object_lock_held(self); - Py_END_CRITICAL_SECTION2(); - return 0; + PyWeakReference *self = args->self; + if (self->clear_state->cleared) { + return 0; + } + + // Ensure we have a strong reference to the referenced object before we try + // to acquire the critical section. + if (_Py_TryIncref(&self->wr_object, self->wr_object)) { + args->object = self->wr_object; + Py_BEGIN_CRITICAL_SECTION(self->wr_object); + PyMutex_Lock(&self->clear_state->mutex); + clear_weakref_lock_held(self, &args->callback); + PyMutex_Unlock(&self->clear_state->mutex); + Py_END_CRITICAL_SECTION(); + return 0; + } + else if (_PyTrash_contains(self->wr_object)) { + PyMutex_Lock(&self->clear_state->mutex); + clear_weakref_lock_held(self, &args->callback); + PyMutex_Unlock(&self->clear_state->mutex); + return 0; + } + + return -1; } -// Clear the weakref, but leave the callback intact static void -clear_weakref_and_leave_callback(PyWeakReference *self) +clear_weakref_for_wr_dealloc(PyWeakReference *self) { - _PyOnceFlagRC *once = self->clear_once; - _PyOnceFlagRC_Incref(once); - _PyOnceFlag_CallOnce(&once->flag, (_Py_once_fn_t *) do_clear_weakref_and_leave_callback, self); - _PyOnceFlagRC_Decref(once); + for (int done = 0; !done;) { + DeallocClearArgs args = { + .self = self, + .callback = NULL, + .object = NULL, + }; + done = !_PyOnceFlag_CallOnce(&self->clear_state->once, (_Py_once_fn_t *) try_clear_weakref_for_wr_dealloc, &args); + Py_XDECREF(args.callback); + Py_XDECREF(args.object); + + if (!done) { + PyThreadState *tstate = _PyThreadState_GET();; + if (tstate && tstate->state == _Py_THREAD_ATTACHED) { + // Only detach if we are attached + PyEval_ReleaseThread(tstate); + sleep(1); + PyEval_AcquireThread(tstate); + } + + } + } } typedef struct { PyWeakReference *weakref; - PyObject *callback; + _PyWeakRefClearState *clear_state; + PyObject *object; + PyObject **callback; } ClearArgs; static int -do_clear_weakref_and_take_callback(ClearArgs *args) +do_clear_weakref(ClearArgs *args) { - PyWeakReference *weakref = args->weakref; - Py_BEGIN_CRITICAL_SECTION2(weakref->wr_object, weakref); - unlink_and_clear_object_lock_held(weakref); - args->callback = clear_callback_lock_held(args->weakref); - Py_END_CRITICAL_SECTION2(); + Py_BEGIN_CRITICAL_SECTION(args->object); + PyMutex_Lock(&args->clear_state->mutex); + if (!args->clear_state->cleared) { + clear_weakref_lock_held(args->weakref, args->callback); + } + PyMutex_Unlock(&args->clear_state->mutex); + Py_END_CRITICAL_SECTION(); return 0; } -// Clear the weakref and return the a strong reference to the callback, if any +// Clear the weakref, but leave the callback intact +static void +clear_weakref_and_leave_callback(PyWeakReference *self) +{ + _PyWeakRefClearState *clear_state = self->clear_state; + _PyWeakRefClearState_Incref(clear_state); + ClearArgs args = { + .weakref = self, + .clear_state = clear_state, + .object = self->wr_object, + .callback = NULL, + }; + _PyOnceFlag_CallOnce(&clear_state->once, (_Py_once_fn_t *) do_clear_weakref, &args); + _PyWeakRefClearState_Decref(clear_state); +} + +// Clear the weakref and return a strong reference to the callback, if any static PyObject * clear_weakref_and_take_callback(PyWeakReference *self) { + PyObject *callback = NULL; + _PyWeakRefClearState *clear_state = self->clear_state; + _PyWeakRefClearState_Incref(clear_state); ClearArgs args = { .weakref = self, - .callback = NULL, + .clear_state = clear_state, + .object = self->wr_object, + .callback = &callback, }; - _PyOnceFlagRC *once = self->clear_once; - _PyOnceFlagRC_Incref(once); - _PyOnceFlag_CallOnce(&once->flag, (_Py_once_fn_t *) do_clear_weakref_and_take_callback, &args); - _PyOnceFlagRC_Decref(once); - return args.callback; + _PyOnceFlag_CallOnce(&clear_state->once, (_Py_once_fn_t *) do_clear_weakref, &args); + _PyWeakRefClearState_Decref(clear_state); + return callback; } // Clear the weakref and its callback @@ -237,7 +385,7 @@ _PyWeakref_ClearRef(PyWeakReference *self) assert(self != NULL); assert(PyWeakref_Check(self)); #ifdef Py_GIL_DISABLED - unlink_and_clear_object_lock_held(self); + clear_weakref_lock_held(self, NULL); #else /* Preserve and restore the callback around clear_weakref. */ PyObject *callback = self->wr_callback; @@ -251,9 +399,12 @@ static void weakref_dealloc(PyObject *self) { PyObject_GC_UnTrack(self); - clear_weakref((PyWeakReference *) self); #ifdef Py_GIL_DISABLED - _PyOnceFlagRC_Decref(((PyWeakReference*)self)->clear_once); + PyWeakReference *wr = (PyWeakReference *) self; + clear_weakref_for_wr_dealloc(wr); + _PyWeakRefClearState_Decref(wr->clear_state); +#else + clear_weakref((PyWeakReference *) self); #endif Py_TYPE(self)->tp_free(self); } @@ -1122,6 +1273,56 @@ handle_callback(PyWeakReference *ref, PyObject *callback) Py_DECREF(cbresult); } +#ifdef Py_GIL_DISABLED + +// Clear the weakrefs and invoke their callbacks. +static void +clear_weakrefs_and_invoke_callbacks_lock_held(PyWeakReference **list) +{ + Py_ssize_t num_weakrefs = _PyWeakref_GetWeakrefCount(*list); + if (num_weakrefs == 0) { + return; + } + + PyObject *exc = PyErr_GetRaisedException(); + PyObject *tuple = PyTuple_New(num_weakrefs * 2); + if (tuple == NULL) { + _PyErr_ChainExceptions1(exc); + return; + } + + int num_items = 0; + while (*list != NULL) { + PyWeakReference *cur = *list; + int live = _Py_TryIncref((PyObject **) &cur, (PyObject *) cur); + PyObject *callback = clear_weakref_and_take_callback(cur); + if (live) { + assert(num_items / 2 < num_weakrefs); + PyTuple_SET_ITEM(tuple, num_items, (PyObject *) cur); + PyTuple_SET_ITEM(tuple, num_items + 1, callback); + num_items += 2; + } + else { + Py_DECREF(callback); + } + } + + for (int i = 0; i < num_items; i += 2) { + PyObject *callback = PyTuple_GET_ITEM(tuple, i + 1); + if (callback != NULL) { + PyObject *weakref = PyTuple_GET_ITEM(tuple, i); + handle_callback((PyWeakReference *)weakref, callback); + } + } + + Py_DECREF(tuple); + + assert(!PyErr_Occurred()); + PyErr_SetRaisedException(exc); +} + +#endif + /* This function is called by the tp_dealloc handler to clear weak references. * * This iterates through the weak references for 'object' and calls callbacks @@ -1142,11 +1343,6 @@ PyObject_ClearWeakRefs(PyObject *object) } list = GET_WEAKREFS_LISTPTR(object); #ifdef Py_GIL_DISABLED - /* Protect the linked-list and the head pointer in object. - - At this point no new weakrefs to object can be created, so we should be - safe to iterate until the list is empty. - */ Py_BEGIN_CRITICAL_SECTION(object); /* Remove the callback-less basic and proxy references, which always appear @@ -1158,21 +1354,7 @@ PyObject_ClearWeakRefs(PyObject *object) } /* Deal with non-canonical (subtypes or refs with callbacks) references. */ - PyObject *exc = PyErr_GetRaisedException(); - while (*list != NULL) { - PyWeakReference *cur = *list; - int live = _Py_TryIncref((PyObject **) &cur, (PyObject *) cur); - PyObject *callback = clear_weakref_and_take_callback(cur); - if (live) { - if (callback != NULL) { - handle_callback(cur, callback); - Py_DECREF(callback); - } - Py_DECREF(cur); - } - } - assert(!PyErr_Occurred()); - PyErr_SetRaisedException(exc); + clear_weakrefs_and_invoke_callbacks_lock_held(list); Py_END_CRITICAL_SECTION(); #else diff --git a/Python/lock.c b/Python/lock.c index f78c0879a90b02..7d1ead585dee6c 100644 --- a/Python/lock.c +++ b/Python/lock.c @@ -362,32 +362,6 @@ _PyOnceFlag_CallOnceSlow(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg) } } -_PyOnceFlagRC * -_PyOnceFlagRC_New(void) -{ - _PyOnceFlagRC *self = (_PyOnceFlagRC *)PyMem_RawCalloc(1, sizeof(_PyOnceFlagRC)); - if (self == NULL) { - PyErr_NoMemory(); - return NULL; - } - self->refcount = 1; - return self; -} - -void -_PyOnceFlagRC_Incref(_PyOnceFlagRC *self) -{ - _Py_atomic_add_ssize(&self->refcount, 1); -} - -void -_PyOnceFlagRC_Decref(_PyOnceFlagRC *self) -{ - if (_Py_atomic_add_ssize(&self->refcount, -1) == 1) { - PyMem_RawFree(self); - } -} - #define _Py_WRITE_LOCKED 1 #define _PyRWMutex_READER_SHIFT 2 #define _Py_RWMUTEX_MAX_READERS (UINTPTR_MAX >> _PyRWMutex_READER_SHIFT) From 88078a686bf0e6ca509d0819289634efdf25da8f Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 21 Mar 2024 15:10:13 -0700 Subject: [PATCH 19/66] Switch to a sharded lock for linked list and object --- Include/cpython/object.h | 7 - Include/internal/pycore_critical_section.h | 15 +- Include/internal/pycore_interp.h | 7 + Include/internal/pycore_weakref.h | 36 +-- Modules/_weakref.c | 8 +- Objects/object.c | 19 -- Objects/typeobject.c | 10 +- Objects/weakrefobject.c | 270 ++++++++------------- Python/pystate.c | 3 + 9 files changed, 148 insertions(+), 227 deletions(-) diff --git a/Include/cpython/object.h b/Include/cpython/object.h index f6b25c47fd345a..7512bb70c760fd 100644 --- a/Include/cpython/object.h +++ b/Include/cpython/object.h @@ -465,13 +465,6 @@ PyAPI_FUNC(void) _PyTrash_end(PyThreadState *tstate); /* Python 3.10 private API, invoked by the Py_TRASHCAN_BEGIN(). */ PyAPI_FUNC(int) _PyTrash_cond(PyObject *op, destructor dealloc); -#ifdef Py_GIL_DISABLED - -/* Private API, used by weakrefs in free-threaded builds */ -PyAPI_FUNC(int) _PyTrash_contains(PyObject *op); - -#endif - #define Py_TRASHCAN_BEGIN_CONDITION(op, cond) \ do { \ PyThreadState *_tstate = NULL; \ diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index 9163b5cf0f2e8a..f19f3119d57217 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -87,10 +87,12 @@ extern "C" { #define _Py_CRITICAL_SECTION_MASK 0x3 #ifdef Py_GIL_DISABLED -# define Py_BEGIN_CRITICAL_SECTION(op) \ +# define Py_BEGIN_CRITICAL_SECTION_MU(mu) \ { \ _PyCriticalSection _cs; \ - _PyCriticalSection_Begin(&_cs, &_PyObject_CAST(op)->ob_mutex) + _PyCriticalSection_Begin(&_cs, &mu) + +# define Py_BEGIN_CRITICAL_SECTION(op) Py_BEGIN_CRITICAL_SECTION_MU(_PyObject_CAST(op)->ob_mutex) # define Py_END_CRITICAL_SECTION() \ _PyCriticalSection_End(&_cs); \ @@ -105,10 +107,13 @@ extern "C" { _PyCriticalSection_XEnd(&_cs_opt); \ } -# define Py_BEGIN_CRITICAL_SECTION2(a, b) \ +# define Py_BEGIN_CRITICAL_SECTION2_MU(a, b) \ { \ _PyCriticalSection2 _cs2; \ - _PyCriticalSection2_Begin(&_cs2, &_PyObject_CAST(a)->ob_mutex, &_PyObject_CAST(b)->ob_mutex) + _PyCriticalSection2_Begin(&_cs2, &a, &b) + +# define Py_BEGIN_CRITICAL_SECTION2(a, b) \ + Py_BEGIN_CRITICAL_SECTION2_MU(_PyObject_CAST(a)->ob_mutex, _PyObject_CAST(b)->ob_mutex) # define Py_END_CRITICAL_SECTION2() \ _PyCriticalSection2_End(&_cs2); \ @@ -138,10 +143,12 @@ extern "C" { #else /* !Py_GIL_DISABLED */ // The critical section APIs are no-ops with the GIL. +# define Py_BEGIN_CRITICAL_SECTION_MU(mu) # define Py_BEGIN_CRITICAL_SECTION(op) # define Py_END_CRITICAL_SECTION() # define Py_XBEGIN_CRITICAL_SECTION(op) # define Py_XEND_CRITICAL_SECTION() +# define Py_BEGIN_CRITICAL_SECTION2_MU(a, b) # define Py_BEGIN_CRITICAL_SECTION2(a, b) # define Py_END_CRITICAL_SECTION2() # define _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(mutex) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index b8d0fdcce11ba8..b459405578cc90 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -59,6 +59,12 @@ struct _stoptheworld_state { PyThreadState *requester; // Thread that requested the pause (may be NULL). }; +#ifdef Py_GIL_DISABLED +// This should be prime but otherwise the choice is arbitrary. A larger value +// increases concurrency at the expense of memory. +#define NUM_WEAKREF_LIST_LOCKS 127 +#endif + /* cross-interpreter data registry */ /* Tracks some rare events per-interpreter, used by the optimizer to turn on/off @@ -203,6 +209,7 @@ struct _is { #if defined(Py_GIL_DISABLED) struct _mimalloc_interp_state mimalloc; struct _brc_state brc; // biased reference counting state + PyMutex weakref_locks[NUM_WEAKREF_LIST_LOCKS]; #endif // Per-interpreter state for the obmalloc allocator. For the main diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index dcf99af058c90a..afc2aad81c2fdd 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -8,16 +8,25 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif +#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION() #include "pycore_lock.h" #include "pycore_object.h" // _Py_REF_IS_MERGED() #ifdef Py_GIL_DISABLED + typedef struct _PyWeakRefClearState { + // Protects the weakref's wr_object and wr_callback. + // Protects the cleared flag below. PyMutex mutex; - _PyOnceFlag once; + + // Set if the weakref has been cleared. int cleared; + Py_ssize_t refcount; } _PyWeakRefClearState; + +#define WEAKREF_LIST_LOCK(obj) _PyInterpreterState_GET()->weakref_locks[((uintptr_t) obj) % NUM_WEAKREF_LIST_LOCKS] + #endif static inline int _is_dead(PyObject *obj) @@ -36,23 +45,12 @@ static inline int _is_dead(PyObject *obj) #endif } -#if defined(Py_GIL_DISABLED) -# define LOCK_WR_OBJECT(weakref) PyMutex_Lock(&(weakref)->clear_state->mutex); -# define UNLOCK_WR_OBJECT(weakref) PyMutex_Unlock(&(weakref)->clear_state->mutex); -#else -# define LOCK_WR_OBJECT(weakref) -# define UNLOCK_WR_OBJECT(weakref) -#endif - -// NB: In free-threaded builds nothing between the LOCK/UNLOCK calls below can -// suspend. - static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) { assert(PyWeakref_Check(ref_obj)); PyObject *ret = NULL; PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); - LOCK_WR_OBJECT(ref) + Py_BEGIN_CRITICAL_SECTION_MU(ref->clear_state->mutex); PyObject *obj = ref->wr_object; if (obj == Py_None) { @@ -68,7 +66,7 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) #endif ret = Py_NewRef(obj); end: - UNLOCK_WR_OBJECT(ref); + Py_END_CRITICAL_SECTION(); return ret; } @@ -77,7 +75,7 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) assert(PyWeakref_Check(ref_obj)); int ret = 0; PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); - LOCK_WR_OBJECT(ref); + Py_BEGIN_CRITICAL_SECTION_MU(ref->clear_state->mutex); PyObject *obj = ref->wr_object; if (obj == Py_None) { // clear_weakref() was called @@ -87,14 +85,16 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) // See _PyWeakref_GET_REF() for the rationale of this test ret = _is_dead(obj); } - UNLOCK_WR_OBJECT(ref); + Py_END_CRITICAL_SECTION(); return ret; } -// NB: In free-threaded builds the referenced object must be live for the -// duration of the call. +// NB: In free-threaded builds the weakref list lock for the referenced object +// must be held around calls to this function. extern Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head); +extern Py_ssize_t _PyWeakref_GetWeakrefCountThreadsafe(PyObject *obj); + // Clear all the weak references to obj but leave their callbacks uncalled and // intact. extern void _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj); diff --git a/Modules/_weakref.c b/Modules/_weakref.c index 5c034ec713e1c8..e5ff165eaefdd6 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -30,9 +30,7 @@ _weakref_getweakrefcount_impl(PyObject *module, PyObject *object) if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(object))) { return 0; } - PyWeakReference **list = GET_WEAKREFS_LISTPTR(object); - Py_ssize_t count = _PyWeakref_GetWeakrefCount(*list); - return count; + return _PyWeakref_GetWeakrefCountThreadsafe(object); } @@ -94,7 +92,7 @@ _weakref_getweakrefs_impl(PyObject *module, PyObject *object) } PyWeakReference **list = GET_WEAKREFS_LISTPTR(object); - Py_ssize_t count = _PyWeakref_GetWeakrefCount(*list); + Py_ssize_t count = _PyWeakref_GetWeakrefCountThreadsafe(object); PyObject *result = PyList_New(count); if (result == NULL) { @@ -103,7 +101,7 @@ _weakref_getweakrefs_impl(PyObject *module, PyObject *object) #ifdef Py_GIL_DISABLED Py_ssize_t num_added = 0; - Py_BEGIN_CRITICAL_SECTION(object); + Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(object)); PyWeakReference *current = *list; // Weakrefs may be added or removed since the count was computed. while (num_added < count && current != NULL) { diff --git a/Objects/object.c b/Objects/object.c index 5efaa28a4c737c..b4f0fd4d7db941 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2835,25 +2835,6 @@ _PyTrash_cond(PyObject *op, destructor dealloc) return Py_TYPE(op)->tp_dealloc == dealloc; } -#ifdef Py_GIL_DISABLED - -int -_PyTrash_contains(PyObject *op) -{ - PyThreadState *tstate = _PyThreadState_GET(); - struct _py_trashcan *trash = _PyTrash_get_state(tstate); - PyObject *cur = trash->delete_later; - while (cur) { - if (cur == op) { - return 1; - } - cur = (PyObject *) cur->ob_tid; - } - return 0; -} - -#endif - void _Py_NO_RETURN _PyObject_AssertFailed(PyObject *obj, const char *expr, const char *msg, diff --git a/Objects/typeobject.c b/Objects/typeobject.c index d66b539d85c362..6d220be12b319c 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -2188,15 +2188,7 @@ subtype_dealloc(PyObject *self) finalizers since they might rely on part of the object being finalized that has already been destroyed. */ if (type->tp_weaklistoffset && !base->tp_weaklistoffset) { - /* Modeled after GET_WEAKREFS_LISTPTR(). - - This is never triggered for static types so we can avoid the - (slightly) more costly _PyObject_GET_WEAKREFS_LISTPTR(). */ - PyWeakReference **list = \ - _PyObject_GET_WEAKREFS_LISTPTR_FROM_OFFSET(self); - while (*list) { - _PyWeakref_ClearRef(*list); - } + _PyWeakref_ClearWeakRefsExceptCallbacks(self); } } diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 6271e3f09b8416..66871c26bf2f79 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -4,6 +4,7 @@ #include "pycore_modsupport.h" // _PyArg_NoKwnames() #include "pycore_object.h" // _PyObject_GET_WEAKREFS_LISTPTR() #include "pycore_pyerrors.h" // _PyErr_ChainExceptions1() +#include "pycore_pystate.h" #include "pycore_weakref.h" // _PyWeakref_GET_REF() #ifdef Py_GIL_DISABLED @@ -15,67 +16,77 @@ * * - The weakref (the referenced object, the hash) * - The referenced object (its head-of-list pointer) - * - The linked-list of weakrefs + * - The linked list of weakrefs * * The above may be modified concurrently when creating weakrefs, destroying - * weakrefs, or destroying the referenced object. A combination of techniques - * are used to ensure it is accessed safely: + * weakrefs, or destroying the referenced object. * - * - The weakref's hash is protected using a critical section on the weakref's - * per-object lock. - * - The referenced object and the callback are protected by a separate mutex - * in the weakref, the `clear_mutex`. - * - The linked-list of weakrefs and the object's head-of-list pointer are - * protected using a critical section on the referenced object's per-object - * lock. - * - Concurrent attempts to clear the weakref (i.e. from the referenced object - * and the weakref) cooperate using the protocol described below. + * To avoid diverging from the default implementation too much, we must + * implement the following: + * + * - When a weakref is destroyed it must be removed from the linked list and + * its borrowed reference must be cleared. + * - When the referenced object is destroyed each of its weak references must + * be removed from the linked list and their internal reference cleared. + * + * A natural arrangement would be to use the per-object lock in the referenced + * object to protect its mutable state and the linked list. Similarly, we could + * use the per-object in each weakref to protect the weakref's mutable + * state. This is complicated by a few things: + * + * - The referenced object and the weakrefs only hold borrowed references to + * each other. + * - The referenced object and its weakrefs may be destroyed concurrently. + * - The GC may run while the referenced object or the weakrefs are being + * destroyed and free their counterpart. * - * Destroying the referenced object - * -------------------------------- - * When the referenced object is destroyed it must clear the borrowed reference - * in each weakref and unlink the weakref from the linked-list: + * If a weakref needs to be unlinked from the linked list during destruction, + * we need to ensure that the memory backing the referenced object is not freed + * during this process. Since the referenced object may be destroyed + * concurrently, we cannot rely on being able to incref it. To complicate + * matters further, if we do not hold a strong reference we cannot guarantee + * that the if the GC runs it won't reclaim the referenced object, and we + * cannot guarantee that GC won't run (unless it is disabled) because lock + * acquisition may suspend. * - * 1. Acquire a critical section on the referenced object's per-object lock. - * 2. If the linked-list is empty, release the critical section and stop. - * 3. For the first object in the list, incref its `clear_state`. - * 4. Release the critical section. - * 5. Call the once flag. If we enter: - * a. Acquire a CS on the referenced object's per-object lock. - * b. Lock the `clear_mutex`. Note that this is not a critical section, so we - * must be careful not to do anything that could suspend while it is held. - * c. Unlink the weakref from the list. - * d. Set `wr_object` to Py_None. - * e. Unlock the `clear_mutex`. - * f. Release the CS. - * 6. Goto (1). + * The inverse is true when destroying the referenced object. We must ensure + * that the memory backing the weakrefs remains alive while we're unlinking + * them, but we only hold borrowed references and they may also be destroyed + * concurrently. + * + * We can probably solve this by using QSBR to ensure that the memory backing + * either the weakref or the referenced object is not freed until we're ready, + * but for now we've chosen to work around it using a few different strategies: + * + * - The weakref's hash is protected using the weakref's per-object lock. + * - Each weakref contains a pointer to a separately refcounted piece of state + * that is used to protect its callback, its referenced object, and + * coordinate clearing between the GC, the weakref, and the referenced + * object. Having a separate lifetime means we can ensure it remains alive + * while being locked from the referenced object's destructor. + * - The linked-list of weakrefs and the object's head-of-list pointer are + * protected using a striped lock contained in the interpreter. This ensures + * that the lock remains alive while being locked from the weakref's + * destructor. Although this introduces a scalability bottleneck, it should + * be temporary. + * - All locking is performed using critical sections. * - * Destroying a weakref - * -------------------- - * The weakref must unlink itself from the linked-list and clear its borrowed - * reference when destroyed: + * All attempts to clear a weakref, except for the GC, must use the following + * protocol: * - * 1. Call its once flag. If we enter: - * a. Try to incref the referenced object. We must hold a strong reference to it - * before attempting to acquire the critical section. Otherwise, if it's - * cyclic garbage it may be destroyed if we're suspended when we acquire the - * CS on its per-object lock below. - * b. If (1a) fails, return -1. - * c. Acquire a CS on the referenced object's per-object lock. - * d. Lock `clear_mutex`. - * e. Unlink the weakref from the list. - * f. Set `wr_object` to Py_None. - * g. Unlock the `clear_mutex`. - * h. Release the CS. - * 2. If (1) failed, retry. + * 1. Incref its `clear_state`. We need a strong reference because the + * weakref may be freed if we suspend in the next step. + * 2. Acquire a two lock critical section using the striped lock and the + * `clear_state`'s mutex. + * 3. Check the `clear_state->cleared` flag. If it is unset, then we know + * that the weakref is still live, and can be cleared. + * 4. Release the CS from (2) + * 5. Decref the `clear_state` from (1). * - * Garbage Collection - * ------------------ - * If the GC determines that an object is cyclic garbage it needs to go through - * and clear any weakrefs to the object. Similarly, if the the GC determines - * that a weakref is garbage it must also be cleared. Thankfully, the world is - * stopped while GC is running, so it can just clear each weakref without - * resorting to the complex protocol above. + * Since the world is stopped when the GC runs, it is free to clear weakrefs + * without acquiring any locks, but must set the `cleared` flag to signal to + * any concurrent attempts at clearing the weakref that the weakref may be + * freed. */ _PyWeakRefClearState * @@ -87,7 +98,6 @@ _PyWeakRefClearState_New(void) return NULL; } self->mutex = (PyMutex){0}; - self->once = (_PyOnceFlag){0}; self->cleared = 0; self->refcount = 1; return self; @@ -117,14 +127,22 @@ Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head) { Py_ssize_t count = 0; - if (head == NULL) { - return 0; - } - Py_BEGIN_CRITICAL_SECTION(head->wr_object); while (head != NULL) { ++count; head = head->wr_next; } + return count; +} + +Py_ssize_t +_PyWeakref_GetWeakrefCountThreadsafe(PyObject *obj) +{ + if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(obj))) { + return 0; + } + Py_ssize_t count; + Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(obj)); + count = _PyWeakref_GetWeakrefCount(*GET_WEAKREFS_LISTPTR(obj)); Py_END_CRITICAL_SECTION(); return count; } @@ -180,7 +198,7 @@ clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) /* If 'self' is the end of the list (and thus self->wr_next == NULL) then the weakref list itself (and thus the value of *list) will end up being set to NULL. */ - *list = self->wr_next; + _Py_atomic_store_ptr(list, self->wr_next); self->wr_object = Py_None; if (self->wr_prev != NULL) self->wr_prev->wr_next = self->wr_next; @@ -193,7 +211,6 @@ clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) *callback = self->wr_callback; self->wr_callback = NULL; } - self->clear_state->cleared = 1; } // Clear the weakref while the world is stopped @@ -205,89 +222,16 @@ gc_clear_weakref(PyWeakReference *self) Py_XDECREF(callback); } -typedef struct { - PyWeakReference *self; - - // A strong reference to the callback in self (out param) - PyObject *callback; - - // A strong reference to the referenced object in self (out param) - PyObject *object; -} DeallocClearArgs; - -static int -try_clear_weakref_for_wr_dealloc(DeallocClearArgs *args) -{ - PyWeakReference *self = args->self; - if (self->clear_state->cleared) { - return 0; - } - - // Ensure we have a strong reference to the referenced object before we try - // to acquire the critical section. - if (_Py_TryIncref(&self->wr_object, self->wr_object)) { - args->object = self->wr_object; - Py_BEGIN_CRITICAL_SECTION(self->wr_object); - PyMutex_Lock(&self->clear_state->mutex); - clear_weakref_lock_held(self, &args->callback); - PyMutex_Unlock(&self->clear_state->mutex); - Py_END_CRITICAL_SECTION(); - return 0; - } - else if (_PyTrash_contains(self->wr_object)) { - PyMutex_Lock(&self->clear_state->mutex); - clear_weakref_lock_held(self, &args->callback); - PyMutex_Unlock(&self->clear_state->mutex); - return 0; - } - - return -1; -} - static void -clear_weakref_for_wr_dealloc(PyWeakReference *self) -{ - for (int done = 0; !done;) { - DeallocClearArgs args = { - .self = self, - .callback = NULL, - .object = NULL, - }; - done = !_PyOnceFlag_CallOnce(&self->clear_state->once, (_Py_once_fn_t *) try_clear_weakref_for_wr_dealloc, &args); - Py_XDECREF(args.callback); - Py_XDECREF(args.object); - - if (!done) { - PyThreadState *tstate = _PyThreadState_GET();; - if (tstate && tstate->state == _Py_THREAD_ATTACHED) { - // Only detach if we are attached - PyEval_ReleaseThread(tstate); - sleep(1); - PyEval_AcquireThread(tstate); - } - - } - } -} - -typedef struct { - PyWeakReference *weakref; - _PyWeakRefClearState *clear_state; - PyObject *object; - PyObject **callback; -} ClearArgs; - -static int -do_clear_weakref(ClearArgs *args) +do_clear_weakref(_PyWeakRefClearState *clear_state, PyObject *obj, PyWeakReference *weakref, PyObject **callback) { - Py_BEGIN_CRITICAL_SECTION(args->object); - PyMutex_Lock(&args->clear_state->mutex); - if (!args->clear_state->cleared) { - clear_weakref_lock_held(args->weakref, args->callback); + Py_BEGIN_CRITICAL_SECTION2_MU(WEAKREF_LIST_LOCK(obj), clear_state->mutex); + // We can only be sure that the memory backing weakref has not been freed + // if the cleared flag is unset. + if (!clear_state->cleared) { + clear_weakref_lock_held(weakref, callback); } - PyMutex_Unlock(&args->clear_state->mutex); - Py_END_CRITICAL_SECTION(); - return 0; + Py_END_CRITICAL_SECTION2(); } // Clear the weakref, but leave the callback intact @@ -296,13 +240,7 @@ clear_weakref_and_leave_callback(PyWeakReference *self) { _PyWeakRefClearState *clear_state = self->clear_state; _PyWeakRefClearState_Incref(clear_state); - ClearArgs args = { - .weakref = self, - .clear_state = clear_state, - .object = self->wr_object, - .callback = NULL, - }; - _PyOnceFlag_CallOnce(&clear_state->once, (_Py_once_fn_t *) do_clear_weakref, &args); + do_clear_weakref(clear_state, self->wr_object, self, NULL); _PyWeakRefClearState_Decref(clear_state); } @@ -313,13 +251,7 @@ clear_weakref_and_take_callback(PyWeakReference *self) PyObject *callback = NULL; _PyWeakRefClearState *clear_state = self->clear_state; _PyWeakRefClearState_Incref(clear_state); - ClearArgs args = { - .weakref = self, - .clear_state = clear_state, - .object = self->wr_object, - .callback = &callback, - }; - _PyOnceFlag_CallOnce(&clear_state->once, (_Py_once_fn_t *) do_clear_weakref, &args); + do_clear_weakref(clear_state, self->wr_object, self, &callback); _PyWeakRefClearState_Decref(clear_state); return callback; } @@ -399,12 +331,9 @@ static void weakref_dealloc(PyObject *self) { PyObject_GC_UnTrack(self); -#ifdef Py_GIL_DISABLED - PyWeakReference *wr = (PyWeakReference *) self; - clear_weakref_for_wr_dealloc(wr); - _PyWeakRefClearState_Decref(wr->clear_state); -#else clear_weakref((PyWeakReference *) self); +#ifdef Py_GIL_DISABLED + _PyWeakRefClearState_Decref(((PyWeakReference *)self)->clear_state); #endif Py_TYPE(self)->tp_free(self); } @@ -655,7 +584,7 @@ weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) } if (callback == Py_None) callback = NULL; - Py_BEGIN_CRITICAL_SECTION(ob); + Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(ob)); self = new_weakref_lock_held(type, ob, callback); Py_END_CRITICAL_SECTION(); } @@ -1122,7 +1051,7 @@ PyWeakref_NewRef(PyObject *ob, PyObject *callback) * critical section can release it. We need to recompute ref/proxy after * any code that may release the critical section. */ - Py_BEGIN_CRITICAL_SECTION(ob); + Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(ob)); get_basic_refs(*list, &ref, &proxy); if (callback == Py_None) callback = NULL; @@ -1144,6 +1073,13 @@ PyWeakref_NewRef(PyObject *ob, PyObject *callback) result = new_weakref(ob, callback); if (result != NULL) { if (callback == NULL) { +#ifdef Py_GIL_DISABLED + /* In free-threaded builds ref can be not-NULL if it is being + destroyed concurrently. + */ +#else + assert(ref == NULL); +#endif insert_head(result, list); } else { @@ -1180,7 +1116,7 @@ PyWeakref_NewProxy(PyObject *ob, PyObject *callback) * critical section can release it. We need to recompute ref/proxy after * any code that may release the critical section. */ - Py_BEGIN_CRITICAL_SECTION(ob); + Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(ob)); get_basic_refs(*list, &ref, &proxy); if (callback == Py_None) callback = NULL; @@ -1343,7 +1279,11 @@ PyObject_ClearWeakRefs(PyObject *object) } list = GET_WEAKREFS_LISTPTR(object); #ifdef Py_GIL_DISABLED - Py_BEGIN_CRITICAL_SECTION(object); + if (_Py_atomic_load_ptr(list) == NULL) { + // Fast path for the common case + return; + } + Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(object)); /* Remove the callback-less basic and proxy references, which always appear at the head of the list. There may be two of each - one live and one in @@ -1433,7 +1373,7 @@ _PyStaticType_ClearWeakRefs(PyInterpreterState *interp, PyTypeObject *type) { static_builtin_state *state = _PyStaticType_GetState(interp, type); PyObject **list = _PyStaticType_GET_WEAKREFS_LISTPTR(state); - Py_BEGIN_CRITICAL_SECTION(type); + Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(type)); while (*list != NULL) { /* Note that clear_weakref() pops the first ref off the type's weaklist before clearing its wr_object and wr_callback. @@ -1453,7 +1393,7 @@ _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj) PyWeakReference **list = \ _PyObject_GET_WEAKREFS_LISTPTR_FROM_OFFSET(obj); #ifdef Py_GIL_DISABLED - Py_BEGIN_CRITICAL_SECTION(obj); + Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(obj)); while (*list != NULL) { clear_weakref_and_leave_callback((PyWeakReference *)*list); } diff --git a/Python/pystate.c b/Python/pystate.c index 921e74ed5a9826..02c44337954c9f 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -617,6 +617,9 @@ init_interpreter(PyInterpreterState *interp, _PyType_InitCache(interp); #ifdef Py_GIL_DISABLED _Py_brc_init_state(interp); + for (int i = 0; i < NUM_WEAKREF_LIST_LOCKS; i++) { + interp->weakref_locks[i] = (PyMutex){0}; + } #endif llist_init(&interp->mem_free_queue.head); for (int i = 0; i < _PY_MONITORING_UNGROUPED_EVENTS; i++) { From dc1f6511179c43bd68a492632ad27bc47895c824 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 22 Mar 2024 14:26:05 -0700 Subject: [PATCH 20/66] Fix formatting --- Objects/weakrefobject.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 66871c26bf2f79..30a393c2a8fc6e 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -92,7 +92,8 @@ _PyWeakRefClearState * _PyWeakRefClearState_New(void) { - _PyWeakRefClearState *self = (_PyWeakRefClearState *)PyMem_RawCalloc(1, sizeof(_PyWeakRefClearState)); + _PyWeakRefClearState *self = (_PyWeakRefClearState *)PyMem_RawCalloc( + 1, sizeof(_PyWeakRefClearState)); if (self == NULL) { PyErr_NoMemory(); return NULL; @@ -223,7 +224,8 @@ gc_clear_weakref(PyWeakReference *self) } static void -do_clear_weakref(_PyWeakRefClearState *clear_state, PyObject *obj, PyWeakReference *weakref, PyObject **callback) +do_clear_weakref(_PyWeakRefClearState *clear_state, PyObject *obj, + PyWeakReference *weakref, PyObject **callback) { Py_BEGIN_CRITICAL_SECTION2_MU(WEAKREF_LIST_LOCK(obj), clear_state->mutex); // We can only be sure that the memory backing weakref has not been freed @@ -528,15 +530,14 @@ new_weakref_lock_held(PyTypeObject *type, PyObject *ob, PyObject *callback) get_basic_refs(*list, &ref, &proxy); if (callback == NULL && type == &_PyWeakref_RefType) { #ifdef Py_GIL_DISABLED - if (ref != NULL && - _Py_TryIncref((PyObject**)&ref, (PyObject*)ref)) { + if (ref != NULL && _Py_TryIncref((PyObject **)&ref, (PyObject *)ref)) { /* We can re-use an existing reference. */ return ref; } #else if (ref != NULL) { /* We can re-use an existing reference. */ - return (PyWeakReference *) Py_NewRef(ref); + return (PyWeakReference *)Py_NewRef(ref); } #endif } From 37c511a3bdcc91a3bf3c4d33e48afbbdd37bf3b5 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 22 Mar 2024 15:09:04 -0700 Subject: [PATCH 21/66] Remove vestigial include --- Objects/typeobject.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 6d220be12b319c..c3965d0ab379af 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -4,7 +4,6 @@ #include "pycore_abstract.h" // _PySequence_IterSearch() #include "pycore_call.h" // _PyObject_VectorcallTstate() #include "pycore_code.h" // CO_FAST_FREE -#include "pycore_critical_section.h" #include "pycore_dict.h" // _PyDict_KeysSize() #include "pycore_frame.h" // _PyInterpreterFrame #include "pycore_lock.h" // _PySeqLock_* From ccaededad211df001e45a6565d2e7327c007e66f Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 22 Mar 2024 16:45:51 -0700 Subject: [PATCH 22/66] Make sure we set the cleared flag --- Objects/weakrefobject.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 30a393c2a8fc6e..1e9900605faee4 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -212,6 +212,7 @@ clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) *callback = self->wr_callback; self->wr_callback = NULL; } + self->clear_state->cleared = 1; } // Clear the weakref while the world is stopped From 6a50381d4be88f3085012e35e815a0fad3665c4c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Sat, 23 Mar 2024 16:22:40 -0700 Subject: [PATCH 23/66] Use `Py_TryIncref` when returning the ref The `is_dead` check from the default build is insufficient, since the refcount may go to zero between when we check and when we incref. More generally, `Py_NewRef` is unsafe to use if you have a borrowed refernce. --- Include/internal/pycore_object.h | 1 - Include/internal/pycore_weakref.h | 8 ++++++-- Include/object.h | 1 + 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index be8b7776bf1b0b..8126eea9983a71 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -87,7 +87,6 @@ PyAPI_FUNC(void) _Py_NO_RETURN _Py_FatalRefcountErrorFunc( PyAPI_DATA(Py_ssize_t) _Py_RefTotal; extern void _Py_AddRefTotal(PyInterpreterState *, Py_ssize_t); -extern void _Py_IncRefTotal(PyInterpreterState *); extern void _Py_DecRefTotal(PyInterpreterState *); # define _Py_DEC_REFTOTAL(interp) \ diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index afc2aad81c2fdd..a94bb1d2dc85a3 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -58,13 +58,17 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) goto end; } +#if !defined(Py_GIL_DISABLED) if (_is_dead(obj)) { goto end; } -#if !defined(Py_GIL_DISABLED) assert(Py_REFCNT(obj) > 0); -#endif ret = Py_NewRef(obj); +#else + if (_Py_TryIncref(&obj, obj)) { + ret = obj; + } +#endif end: Py_END_CRITICAL_SECTION(); return ret; diff --git a/Include/object.h b/Include/object.h index 67a5e514c421c3..207fce17d96489 100644 --- a/Include/object.h +++ b/Include/object.h @@ -766,6 +766,7 @@ PyAPI_FUNC(void) _Py_NegativeRefcount(const char *filename, int lineno, PyObject *op); PyAPI_FUNC(void) _Py_INCREF_IncRefTotal(void); PyAPI_FUNC(void) _Py_DECREF_DecRefTotal(void); +PyAPI_FUNC(void) _Py_IncRefTotal(PyInterpreterState *); #endif // Py_REF_DEBUG && !Py_LIMITED_API PyAPI_FUNC(void) _Py_Dealloc(PyObject *); From d998d1b4ed182cbb0b2dc6f967c202851d289540 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Sun, 24 Mar 2024 12:09:56 -0700 Subject: [PATCH 24/66] Don't leak ref if initialization fails --- Objects/weakrefobject.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 1e9900605faee4..364791da51697b 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -179,6 +179,7 @@ new_weakref(PyObject *ob, PyObject *callback) result = PyObject_GC_New(PyWeakReference, &_PyWeakref_RefType); if (result) { if (init_weakref(result, ob, callback) < 0) { + Py_DECREF(result); return NULL; } PyObject_GC_Track(result); From 37076b5757b31152dbdb94a3823430f5c66d8e53 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Sun, 24 Mar 2024 12:46:27 -0700 Subject: [PATCH 25/66] Remove clinic critical section directives for `_weakref` module Locking is handled in the implementation --- Modules/_weakref.c | 8 +++----- Modules/clinic/_weakref.c.h | 20 +------------------- 2 files changed, 4 insertions(+), 24 deletions(-) diff --git a/Modules/_weakref.c b/Modules/_weakref.c index e5ff165eaefdd6..d584522ca324f4 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -14,7 +14,6 @@ module _weakref #include "clinic/_weakref.c.h" /*[clinic input] -@critical_section object _weakref.getweakrefcount -> Py_ssize_t object: object @@ -25,7 +24,7 @@ Return the number of weak references to 'object'. static Py_ssize_t _weakref_getweakrefcount_impl(PyObject *module, PyObject *object) -/*[clinic end generated code: output=301806d59558ff3e input=6535a580f1d0ebdc]*/ +/*[clinic end generated code: output=301806d59558ff3e input=7d4d04fcaccf64d5]*/ { if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(object))) { return 0; @@ -75,7 +74,6 @@ _weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct, /*[clinic input] -@critical_section object _weakref.getweakrefs object: object / @@ -84,8 +82,8 @@ Return a list of all weak reference objects pointing to 'object'. [clinic start generated code]*/ static PyObject * -_weakref_getweakrefs_impl(PyObject *module, PyObject *object) -/*[clinic end generated code: output=5ec268989fb8f035 input=3dea95b8f5b31bbb]*/ +_weakref_getweakrefs(PyObject *module, PyObject *object) +/*[clinic end generated code: output=25c7731d8e011824 input=00c6d0e5d3206693]*/ { if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(object))) { return PyList_New(0); diff --git a/Modules/clinic/_weakref.c.h b/Modules/clinic/_weakref.c.h index 550b6c4d71a015..8d7bc5dc936610 100644 --- a/Modules/clinic/_weakref.c.h +++ b/Modules/clinic/_weakref.c.h @@ -2,7 +2,6 @@ preserve [clinic start generated code]*/ -#include "pycore_critical_section.h"// Py_BEGIN_CRITICAL_SECTION() #include "pycore_modsupport.h" // _PyArg_CheckPositional() PyDoc_STRVAR(_weakref_getweakrefcount__doc__, @@ -23,9 +22,7 @@ _weakref_getweakrefcount(PyObject *module, PyObject *object) PyObject *return_value = NULL; Py_ssize_t _return_value; - Py_BEGIN_CRITICAL_SECTION(object); _return_value = _weakref_getweakrefcount_impl(module, object); - Py_END_CRITICAL_SECTION(); if ((_return_value == -1) && PyErr_Occurred()) { goto exit; } @@ -79,21 +76,6 @@ PyDoc_STRVAR(_weakref_getweakrefs__doc__, #define _WEAKREF_GETWEAKREFS_METHODDEF \ {"getweakrefs", (PyCFunction)_weakref_getweakrefs, METH_O, _weakref_getweakrefs__doc__}, -static PyObject * -_weakref_getweakrefs_impl(PyObject *module, PyObject *object); - -static PyObject * -_weakref_getweakrefs(PyObject *module, PyObject *object) -{ - PyObject *return_value = NULL; - - Py_BEGIN_CRITICAL_SECTION(object); - return_value = _weakref_getweakrefs_impl(module, object); - Py_END_CRITICAL_SECTION(); - - return return_value; -} - PyDoc_STRVAR(_weakref_proxy__doc__, "proxy($module, object, callback=None, /)\n" "--\n" @@ -130,4 +112,4 @@ _weakref_proxy(PyObject *module, PyObject *const *args, Py_ssize_t nargs) exit: return return_value; } -/*[clinic end generated code: output=d5d30707212a9870 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=60f59adc1dc9eab8 input=a9049054013a1b77]*/ From a07d2db3d85e480224308d8b8ac75fd992484daf Mon Sep 17 00:00:00 2001 From: Matt Page Date: Sun, 24 Mar 2024 14:44:06 -0700 Subject: [PATCH 26/66] Support async removal of dead weakrefs from WeakValueDictionary --- Include/internal/pycore_dict.h | 3 ++ Lib/test/test_weakref.py | 19 ++++++++ Modules/_weakref.c | 9 ++++ Objects/dictobject.c | 81 ++++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index ef59960dbab071..9617b4ebd397f9 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -18,6 +18,9 @@ extern PyObject* _PyDict_GetItemWithError(PyObject *dp, PyObject *key); extern int _PyDict_DelItemIf(PyObject *mp, PyObject *key, int (*predicate)(PyObject *value)); +#ifdef Py_GIL_DISABLED +extern int _PyDict_DelItemIfDeadWeakref(PyObject *mp, PyObject *key); +#endif // "KnownHash" variants // Export for '_asyncio' shared extension diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py index 4cdd66d3769e0c..d42729b58c2f27 100644 --- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -1865,6 +1865,25 @@ def test_threaded_weak_valued_consistency(self): self.assertEqual(len(d), 1) o = None # lose ref + @support.cpython_only + def test_weak_valued_consistency(self): + # A single-threaded, deterministic repro for issue #28427: old keys + # should not remove new values from WeakValueDictionary. This relies on + # an implementation detail of CPython's WeakValueDictionary (its + # underlying dictionary of KeyedRefs) to reproduce the issue. + d = weakref.WeakValueDictionary() + with support.disable_gc(): + d[10] = RefCycle() + # Keep the KeyedRef alive after it's replaced so that GC will invoke + # the callback. + wr = d.data[10] + # Replace the value with something that isn't cyclic garbage + o = RefCycle() + d[10] = o + # Trigger GC, which will invoke the callback for `wr` + gc.collect() + self.assertEqual(len(d), 1) + def check_threaded_weak_dict_copy(self, type_, deepcopy): # `type_` should be either WeakKeyDictionary or WeakValueDictionary. # `deepcopy` should be either True or False. diff --git a/Modules/_weakref.c b/Modules/_weakref.c index d584522ca324f4..19c6741ecf4906 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -32,6 +32,7 @@ _weakref_getweakrefcount_impl(PyObject *module, PyObject *object) return _PyWeakref_GetWeakrefCountThreadsafe(object); } +#ifndef Py_GIL_DISABLED static int is_dead_weakref(PyObject *value) @@ -43,6 +44,8 @@ is_dead_weakref(PyObject *value) return _PyWeakref_IS_DEAD(value); } +#endif + /*[clinic input] _weakref._remove_dead_weakref -> object @@ -59,6 +62,11 @@ _weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct, PyObject *key) /*[clinic end generated code: output=d9ff53061fcb875c input=19fc91f257f96a1d]*/ { +#ifdef Py_GIL_DISABLED + if (_PyDict_DelItemIfDeadWeakref(dct, key) < 0) { + return NULL; + } +#else if (_PyDict_DelItemIf(dct, key, is_dead_weakref) < 0) { if (PyErr_ExceptionMatches(PyExc_KeyError)) /* This function is meant to allow safe weak-value dicts @@ -69,6 +77,7 @@ _weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct, else return NULL; } +#endif Py_RETURN_NONE; } diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 536746ca41eed5..7aeed983c946d7 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -129,6 +129,7 @@ As a consequence of this, split keys have a maximum size of 16. #include "pycore_pyerrors.h" // _PyErr_GetRaisedException() #include "pycore_pystate.h" // _PyThreadState_GET() #include "pycore_setobject.h" // _PySet_NextEntry() +#include "pycore_weakref.h" #include "stringlib/eq.h" // unicode_eq() #include @@ -2638,6 +2639,86 @@ _PyDict_DelItemIf(PyObject *op, PyObject *key, return res; } +#ifdef Py_GIL_DISABLED + +static int +delitemifdeadweakref_lock_held(PyObject *op, PyObject *key) +{ + Py_ssize_t hashpos, ix; + PyDictObject *mp; + Py_hash_t hash; + PyObject *old_value; + + ASSERT_DICT_LOCKED(op); + + if (!PyDict_Check(op)) { + PyErr_BadInternalCall(); + return -1; + } + assert(key); + hash = PyObject_Hash(key); + if (hash == -1) + return -1; + mp = (PyDictObject *)op; + ix = _Py_dict_lookup(mp, key, hash, &old_value); + if (ix == DKIX_ERROR) + return -1; + if (ix == DKIX_EMPTY || old_value == NULL) { + return 0; + } + + // PyWeakref_Check may suspend; we need to get a strong reference + // to old_value in case its removed from the dictionary while + // we're suspended. + PyObject *orig_old_value = Py_NewRef(old_value); + if (!PyWeakref_Check(old_value)) { + PyErr_SetString(PyExc_TypeError, "not a weakref"); + return -1; + } + + if (!_PyWeakref_IS_DEAD(orig_old_value)) { + Py_DECREF(orig_old_value); + return 0; + } + + // The call to _PyWeakref_IS_DEAD may have suspended and the original value + // removed. + ix = _Py_dict_lookup(mp, key, hash, &old_value); + if (ix == DKIX_ERROR) { + Py_DECREF(orig_old_value); + return -1; + } + if (ix == DKIX_EMPTY || old_value != orig_old_value) { + // The key was removed or a new value was inserted. If the new value + // points to a dead weakref, then a subsequent call to this function + // will remove it. + Py_DECREF(orig_old_value); + return 0; + } + + hashpos = lookdict_index(mp->ma_keys, hash, ix); + assert(hashpos >= 0); + + PyInterpreterState *interp = _PyInterpreterState_GET(); + uint64_t new_version = _PyDict_NotifyEvent( + interp, PyDict_EVENT_DELETED, mp, key, NULL); + int res = delitem_common(mp, hashpos, ix, old_value, new_version); + Py_DECREF(orig_old_value); + return res; +} + +int +_PyDict_DelItemIfDeadWeakref(PyObject *op, PyObject *key) +{ + int res; + Py_BEGIN_CRITICAL_SECTION(op); + res = delitemifdeadweakref_lock_held(op, key); + Py_END_CRITICAL_SECTION(); + return res; +} + +#endif + static void clear_lock_held(PyObject *op) { From a90184808cf55b9e6b4a7c15937145a8672a2534 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 26 Mar 2024 14:54:37 -0700 Subject: [PATCH 27/66] Only use the striped lock --- Include/cpython/weakrefobject.h | 5 - Include/internal/pycore_critical_section.h | 15 +- Include/internal/pycore_dict.h | 3 - Include/internal/pycore_lock.h | 2 +- Include/internal/pycore_weakref.h | 60 ++- Lib/test/test_sys.py | 10 +- Modules/_weakref.c | 15 +- Objects/dictobject.c | 80 --- Objects/weakrefobject.c | 541 +++++++++++---------- Python/pystate.c | 7 + 10 files changed, 344 insertions(+), 394 deletions(-) diff --git a/Include/cpython/weakrefobject.h b/Include/cpython/weakrefobject.h index 6faaf6f8620eb4..1559e2def61260 100644 --- a/Include/cpython/weakrefobject.h +++ b/Include/cpython/weakrefobject.h @@ -22,11 +22,6 @@ struct _PyWeakReference { */ Py_hash_t hash; -#ifdef Py_GIL_DISABLED - /* Used in free-threaded builds to protect wr_object and wr_callback. */ - struct _PyWeakRefClearState *clear_state; -#endif - /* If wr_object is weakly referenced, wr_object has a doubly-linked NULL- * terminated list of weak references to it. These are the list pointers. * If wr_object goes away, wr_object is set to Py_None, and these pointers diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index f19f3119d57217..9163b5cf0f2e8a 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -87,12 +87,10 @@ extern "C" { #define _Py_CRITICAL_SECTION_MASK 0x3 #ifdef Py_GIL_DISABLED -# define Py_BEGIN_CRITICAL_SECTION_MU(mu) \ +# define Py_BEGIN_CRITICAL_SECTION(op) \ { \ _PyCriticalSection _cs; \ - _PyCriticalSection_Begin(&_cs, &mu) - -# define Py_BEGIN_CRITICAL_SECTION(op) Py_BEGIN_CRITICAL_SECTION_MU(_PyObject_CAST(op)->ob_mutex) + _PyCriticalSection_Begin(&_cs, &_PyObject_CAST(op)->ob_mutex) # define Py_END_CRITICAL_SECTION() \ _PyCriticalSection_End(&_cs); \ @@ -107,13 +105,10 @@ extern "C" { _PyCriticalSection_XEnd(&_cs_opt); \ } -# define Py_BEGIN_CRITICAL_SECTION2_MU(a, b) \ +# define Py_BEGIN_CRITICAL_SECTION2(a, b) \ { \ _PyCriticalSection2 _cs2; \ - _PyCriticalSection2_Begin(&_cs2, &a, &b) - -# define Py_BEGIN_CRITICAL_SECTION2(a, b) \ - Py_BEGIN_CRITICAL_SECTION2_MU(_PyObject_CAST(a)->ob_mutex, _PyObject_CAST(b)->ob_mutex) + _PyCriticalSection2_Begin(&_cs2, &_PyObject_CAST(a)->ob_mutex, &_PyObject_CAST(b)->ob_mutex) # define Py_END_CRITICAL_SECTION2() \ _PyCriticalSection2_End(&_cs2); \ @@ -143,12 +138,10 @@ extern "C" { #else /* !Py_GIL_DISABLED */ // The critical section APIs are no-ops with the GIL. -# define Py_BEGIN_CRITICAL_SECTION_MU(mu) # define Py_BEGIN_CRITICAL_SECTION(op) # define Py_END_CRITICAL_SECTION() # define Py_XBEGIN_CRITICAL_SECTION(op) # define Py_XEND_CRITICAL_SECTION() -# define Py_BEGIN_CRITICAL_SECTION2_MU(a, b) # define Py_BEGIN_CRITICAL_SECTION2(a, b) # define Py_END_CRITICAL_SECTION2() # define _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(mutex) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index 9617b4ebd397f9..ef59960dbab071 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -18,9 +18,6 @@ extern PyObject* _PyDict_GetItemWithError(PyObject *dp, PyObject *key); extern int _PyDict_DelItemIf(PyObject *mp, PyObject *key, int (*predicate)(PyObject *value)); -#ifdef Py_GIL_DISABLED -extern int _PyDict_DelItemIfDeadWeakref(PyObject *mp, PyObject *key); -#endif // "KnownHash" variants // Export for '_asyncio' shared extension diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index f993c95ecbf75a..4c6f4eaf87e71e 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -113,7 +113,7 @@ typedef enum _PyLockFlags { // Lock a mutex with an optional timeout and additional options. See // _PyLockFlags for details. -extern PyLockStatus +PyAPI_FUNC(PyLockStatus) _PyMutex_LockTimed(PyMutex *m, PyTime_t timeout_ns, _PyLockFlags flags); // Lock a mutex with aditional options. See _PyLockFlags for details. diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index a94bb1d2dc85a3..788ea97be8d1c2 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -14,18 +14,15 @@ extern "C" { #ifdef Py_GIL_DISABLED -typedef struct _PyWeakRefClearState { - // Protects the weakref's wr_object and wr_callback. - // Protects the cleared flag below. - PyMutex mutex; +#define WEAKREF_LIST_LOCK(obj) _PyInterpreterState_GET()->weakref_locks[((uintptr_t) obj) % NUM_WEAKREF_LIST_LOCKS] - // Set if the weakref has been cleared. - int cleared; +#define LOCK_WEAKREFS(obj) PyMutex_LockFlags(&WEAKREF_LIST_LOCK(obj), _Py_LOCK_DONT_DETACH) +#define UNLOCK_WEAKREFS(obj) PyMutex_Unlock(&WEAKREF_LIST_LOCK(obj)) - Py_ssize_t refcount; -} _PyWeakRefClearState; +#else -#define WEAKREF_LIST_LOCK(obj) _PyInterpreterState_GET()->weakref_locks[((uintptr_t) obj) % NUM_WEAKREF_LIST_LOCKS] +#define LOCK_WEAKREFS(obj) +#define UNLOCK_WEAKREFS(obj) #endif @@ -48,30 +45,38 @@ static inline int _is_dead(PyObject *obj) static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) { assert(PyWeakref_Check(ref_obj)); - PyObject *ret = NULL; PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); - Py_BEGIN_CRITICAL_SECTION_MU(ref->clear_state->mutex); - PyObject *obj = ref->wr_object; +#if !defined(Py_GIL_DISABLED) + PyObject *obj = ref->wr_object; if (obj == Py_None) { // clear_weakref() was called - goto end; + return NULL; } - -#if !defined(Py_GIL_DISABLED) if (_is_dead(obj)) { - goto end; + return NULL; } assert(Py_REFCNT(obj) > 0); - ret = Py_NewRef(obj); + return Py_NewRef(obj); #else + PyObject *obj = _Py_atomic_load_ptr(&ref->wr_object); + if (obj == Py_None) { + // clear_weakref() was called + return NULL; + } + LOCK_WEAKREFS(obj); + if (_Py_atomic_load_ptr(&ref->wr_object) == Py_None) { + // clear_weakref() was called + UNLOCK_WEAKREFS(obj); + return NULL; + } if (_Py_TryIncref(&obj, obj)) { - ret = obj; + UNLOCK_WEAKREFS(obj); + return obj; } + UNLOCK_WEAKREFS(obj); + return NULL; #endif -end: - Py_END_CRITICAL_SECTION(); - return ret; } static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) @@ -79,17 +84,26 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) assert(PyWeakref_Check(ref_obj)); int ret = 0; PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); - Py_BEGIN_CRITICAL_SECTION_MU(ref->clear_state->mutex); +#ifdef Py_GIL_DISABLED + PyObject *obj = _Py_atomic_load_ptr(&ref->wr_object); +#else PyObject *obj = ref->wr_object; +#endif if (obj == Py_None) { // clear_weakref() was called ret = 1; } else { + LOCK_WEAKREFS(obj); // See _PyWeakref_GET_REF() for the rationale of this test +#ifdef Py_GIL_DISABLED + PyObject *obj_reloaded = _Py_atomic_load_ptr(&ref->wr_object); + ret = (obj_reloaded == Py_None) || _is_dead(obj); +#else ret = _is_dead(obj); +#endif + UNLOCK_WEAKREFS(obj); } - Py_END_CRITICAL_SECTION(); return ret; } diff --git a/Lib/test/test_sys.py b/Lib/test/test_sys.py index 32fbe93e7831bc..37c16cd1047885 100644 --- a/Lib/test/test_sys.py +++ b/Lib/test/test_sys.py @@ -12,7 +12,7 @@ import sysconfig import test.support from test import support -from test.support import os_helper, Py_GIL_DISABLED +from test.support import os_helper from test.support.script_helper import assert_python_ok, assert_python_failure from test.support import threading_helper from test.support import import_helper @@ -1707,15 +1707,11 @@ class newstyleclass(object): pass # TODO: add check that forces layout of unicodefields # weakref import weakref - if Py_GIL_DISABLED: - expected_size = size('2Pn4P') - else: - expected_size = size('2Pn3P') - check(weakref.ref(int), expected_size) + check(weakref.ref(int), size('2Pn3P')) # weakproxy # XXX # weakcallableproxy - check(weakref.proxy(int), expected_size) + check(weakref.proxy(int), size('2Pn3P')) def check_slots(self, obj, base, extra): expected = sys.getsizeof(base) + struct.calcsize(extra) diff --git a/Modules/_weakref.c b/Modules/_weakref.c index 19c6741ecf4906..bee2f38c3f0742 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -32,8 +32,6 @@ _weakref_getweakrefcount_impl(PyObject *module, PyObject *object) return _PyWeakref_GetWeakrefCountThreadsafe(object); } -#ifndef Py_GIL_DISABLED - static int is_dead_weakref(PyObject *value) { @@ -44,8 +42,6 @@ is_dead_weakref(PyObject *value) return _PyWeakref_IS_DEAD(value); } -#endif - /*[clinic input] _weakref._remove_dead_weakref -> object @@ -62,11 +58,6 @@ _weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct, PyObject *key) /*[clinic end generated code: output=d9ff53061fcb875c input=19fc91f257f96a1d]*/ { -#ifdef Py_GIL_DISABLED - if (_PyDict_DelItemIfDeadWeakref(dct, key) < 0) { - return NULL; - } -#else if (_PyDict_DelItemIf(dct, key, is_dead_weakref) < 0) { if (PyErr_ExceptionMatches(PyExc_KeyError)) /* This function is meant to allow safe weak-value dicts @@ -77,7 +68,6 @@ _weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct, else return NULL; } -#endif Py_RETURN_NONE; } @@ -108,7 +98,7 @@ _weakref_getweakrefs(PyObject *module, PyObject *object) #ifdef Py_GIL_DISABLED Py_ssize_t num_added = 0; - Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(object)); + LOCK_WEAKREFS(object); PyWeakReference *current = *list; // Weakrefs may be added or removed since the count was computed. while (num_added < count && current != NULL) { @@ -118,7 +108,8 @@ _weakref_getweakrefs(PyObject *module, PyObject *object) } current = current->wr_next; } - Py_END_CRITICAL_SECTION(); + UNLOCK_WEAKREFS(object); + // Don't return an incomplete list if (num_added != count) { PyObject *new_list = PyList_New(num_added); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 7aeed983c946d7..8b038930050fa8 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2639,86 +2639,6 @@ _PyDict_DelItemIf(PyObject *op, PyObject *key, return res; } -#ifdef Py_GIL_DISABLED - -static int -delitemifdeadweakref_lock_held(PyObject *op, PyObject *key) -{ - Py_ssize_t hashpos, ix; - PyDictObject *mp; - Py_hash_t hash; - PyObject *old_value; - - ASSERT_DICT_LOCKED(op); - - if (!PyDict_Check(op)) { - PyErr_BadInternalCall(); - return -1; - } - assert(key); - hash = PyObject_Hash(key); - if (hash == -1) - return -1; - mp = (PyDictObject *)op; - ix = _Py_dict_lookup(mp, key, hash, &old_value); - if (ix == DKIX_ERROR) - return -1; - if (ix == DKIX_EMPTY || old_value == NULL) { - return 0; - } - - // PyWeakref_Check may suspend; we need to get a strong reference - // to old_value in case its removed from the dictionary while - // we're suspended. - PyObject *orig_old_value = Py_NewRef(old_value); - if (!PyWeakref_Check(old_value)) { - PyErr_SetString(PyExc_TypeError, "not a weakref"); - return -1; - } - - if (!_PyWeakref_IS_DEAD(orig_old_value)) { - Py_DECREF(orig_old_value); - return 0; - } - - // The call to _PyWeakref_IS_DEAD may have suspended and the original value - // removed. - ix = _Py_dict_lookup(mp, key, hash, &old_value); - if (ix == DKIX_ERROR) { - Py_DECREF(orig_old_value); - return -1; - } - if (ix == DKIX_EMPTY || old_value != orig_old_value) { - // The key was removed or a new value was inserted. If the new value - // points to a dead weakref, then a subsequent call to this function - // will remove it. - Py_DECREF(orig_old_value); - return 0; - } - - hashpos = lookdict_index(mp->ma_keys, hash, ix); - assert(hashpos >= 0); - - PyInterpreterState *interp = _PyInterpreterState_GET(); - uint64_t new_version = _PyDict_NotifyEvent( - interp, PyDict_EVENT_DELETED, mp, key, NULL); - int res = delitem_common(mp, hashpos, ix, old_value, new_version); - Py_DECREF(orig_old_value); - return res; -} - -int -_PyDict_DelItemIfDeadWeakref(PyObject *op, PyObject *key) -{ - int res; - Py_BEGIN_CRITICAL_SECTION(op); - res = delitemifdeadweakref_lock_held(op, key); - Py_END_CRITICAL_SECTION(); - return res; -} - -#endif - static void clear_lock_held(PyObject *op) { diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 364791da51697b..838d5c595fcf2e 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -59,65 +59,16 @@ * but for now we've chosen to work around it using a few different strategies: * * - The weakref's hash is protected using the weakref's per-object lock. - * - Each weakref contains a pointer to a separately refcounted piece of state - * that is used to protect its callback, its referenced object, and - * coordinate clearing between the GC, the weakref, and the referenced - * object. Having a separate lifetime means we can ensure it remains alive - * while being locked from the referenced object's destructor. - * - The linked-list of weakrefs and the object's head-of-list pointer are - * protected using a striped lock contained in the interpreter. This ensures - * that the lock remains alive while being locked from the weakref's - * destructor. Although this introduces a scalability bottleneck, it should - * be temporary. - * - All locking is performed using critical sections. - * - * All attempts to clear a weakref, except for the GC, must use the following - * protocol: - * - * 1. Incref its `clear_state`. We need a strong reference because the - * weakref may be freed if we suspend in the next step. - * 2. Acquire a two lock critical section using the striped lock and the - * `clear_state`'s mutex. - * 3. Check the `clear_state->cleared` flag. If it is unset, then we know - * that the weakref is still live, and can be cleared. - * 4. Release the CS from (2) - * 5. Decref the `clear_state` from (1). + * - The other mutable is protected by a striped lock owned by the interpreter. + * - The striped lock must be locked using `_Py_LOCK_DONT_DETACH` in order to + * support atomic deletion from WeakValueDictionaries. As a result, we must + * be careful not to perform any operations that could suspend while the + * lock is held. * * Since the world is stopped when the GC runs, it is free to clear weakrefs - * without acquiring any locks, but must set the `cleared` flag to signal to - * any concurrent attempts at clearing the weakref that the weakref may be - * freed. + * without acquiring any locks. */ -_PyWeakRefClearState * -_PyWeakRefClearState_New(void) -{ - _PyWeakRefClearState *self = (_PyWeakRefClearState *)PyMem_RawCalloc( - 1, sizeof(_PyWeakRefClearState)); - if (self == NULL) { - PyErr_NoMemory(); - return NULL; - } - self->mutex = (PyMutex){0}; - self->cleared = 0; - self->refcount = 1; - return self; -} - -void -_PyWeakRefClearState_Incref(_PyWeakRefClearState *self) -{ - _Py_atomic_add_ssize(&self->refcount, 1); -} - -void -_PyWeakRefClearState_Decref(_PyWeakRefClearState *self) -{ - if (_Py_atomic_add_ssize(&self->refcount, -1) == 1) { - PyMem_RawFree(self); - } -} - #endif #define GET_WEAKREFS_LISTPTR(o) \ @@ -142,15 +93,15 @@ _PyWeakref_GetWeakrefCountThreadsafe(PyObject *obj) return 0; } Py_ssize_t count; - Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(obj)); + LOCK_WEAKREFS(obj); count = _PyWeakref_GetWeakrefCount(*GET_WEAKREFS_LISTPTR(obj)); - Py_END_CRITICAL_SECTION(); + UNLOCK_WEAKREFS(obj); return count; } static PyObject *weakref_vectorcall(PyObject *self, PyObject *const *args, size_t nargsf, PyObject *kwnames); -static int +static void init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) { self->hash = -1; @@ -160,15 +111,9 @@ init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) self->wr_callback = Py_XNewRef(callback); self->vectorcall = weakref_vectorcall; #ifdef Py_GIL_DISABLED - self->clear_state = _PyWeakRefClearState_New(); - if (self->clear_state == NULL) { - Py_XDECREF(self->wr_callback); - return -1; - } _PyObject_SetMaybeWeakref(ob); _PyObject_SetMaybeWeakref((PyObject *)self); #endif - return 0; } static PyWeakReference * @@ -178,10 +123,7 @@ new_weakref(PyObject *ob, PyObject *callback) result = PyObject_GC_New(PyWeakReference, &_PyWeakref_RefType); if (result) { - if (init_weakref(result, ob, callback) < 0) { - Py_DECREF(result); - return NULL; - } + init_weakref(result, ob, callback); PyObject_GC_Track(result); } return result; @@ -194,14 +136,14 @@ static void clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) { // TODO: Assert locks are held or world is stopped - if (self->wr_object != Py_None) { + if (_Py_atomic_load_ptr(&self->wr_object) != Py_None) { PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); if (*list == self) /* If 'self' is the end of the list (and thus self->wr_next == NULL) then the weakref list itself (and thus the value of *list) will end up being set to NULL. */ _Py_atomic_store_ptr(list, self->wr_next); - self->wr_object = Py_None; + _Py_atomic_store_ptr(&self->wr_object, Py_None); if (self->wr_prev != NULL) self->wr_prev->wr_next = self->wr_next; if (self->wr_next != NULL) @@ -213,7 +155,6 @@ clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) *callback = self->wr_callback; self->wr_callback = NULL; } - self->clear_state->cleared = 1; } // Clear the weakref while the world is stopped @@ -225,46 +166,18 @@ gc_clear_weakref(PyWeakReference *self) Py_XDECREF(callback); } -static void -do_clear_weakref(_PyWeakRefClearState *clear_state, PyObject *obj, - PyWeakReference *weakref, PyObject **callback) -{ - Py_BEGIN_CRITICAL_SECTION2_MU(WEAKREF_LIST_LOCK(obj), clear_state->mutex); - // We can only be sure that the memory backing weakref has not been freed - // if the cleared flag is unset. - if (!clear_state->cleared) { - clear_weakref_lock_held(weakref, callback); - } - Py_END_CRITICAL_SECTION2(); -} - -// Clear the weakref, but leave the callback intact -static void -clear_weakref_and_leave_callback(PyWeakReference *self) -{ - _PyWeakRefClearState *clear_state = self->clear_state; - _PyWeakRefClearState_Incref(clear_state); - do_clear_weakref(clear_state, self->wr_object, self, NULL); - _PyWeakRefClearState_Decref(clear_state); -} - -// Clear the weakref and return a strong reference to the callback, if any -static PyObject * -clear_weakref_and_take_callback(PyWeakReference *self) -{ - PyObject *callback = NULL; - _PyWeakRefClearState *clear_state = self->clear_state; - _PyWeakRefClearState_Incref(clear_state); - do_clear_weakref(clear_state, self->wr_object, self, &callback); - _PyWeakRefClearState_Decref(clear_state); - return callback; -} - // Clear the weakref and its callback static void clear_weakref(PyWeakReference *self) { - PyObject *callback = clear_weakref_and_take_callback(self); + PyObject *object = _Py_atomic_load_ptr(&self->wr_object); + if (object == Py_None) { + return; + } + PyObject *callback = NULL; + LOCK_WEAKREFS(object); + clear_weakref_lock_held(self, &callback); + UNLOCK_WEAKREFS(object); Py_XDECREF(callback); } @@ -336,9 +249,6 @@ weakref_dealloc(PyObject *self) { PyObject_GC_UnTrack(self); clear_weakref((PyWeakReference *) self); -#ifdef Py_GIL_DISABLED - _PyWeakRefClearState_Decref(((PyWeakReference *)self)->clear_state); -#endif Py_TYPE(self)->tp_free(self); } @@ -524,53 +434,59 @@ parse_weakref_init_args(const char *funcname, PyObject *args, PyObject *kwargs, return PyArg_UnpackTuple(args, funcname, 1, 2, obp, callbackp); } +#ifdef Py_GIL_DISABLED + static PyWeakReference * -new_weakref_lock_held(PyTypeObject *type, PyObject *ob, PyObject *callback) +get_or_create_weakref_subtype(PyTypeObject *type, PyObject *obj, PyObject *callback) { + PyWeakReference **list = GET_WEAKREFS_LISTPTR(obj); PyWeakReference *ref, *proxy; - PyWeakReference **list = GET_WEAKREFS_LISTPTR(ob); + + LOCK_WEAKREFS(obj); + get_basic_refs(*list, &ref, &proxy); + if ((callback == NULL) && (type == &_PyWeakref_RefType) && (ref != NULL) && _Py_TryIncref((PyObject**) &ref, (PyObject *) ref)) { + // We can reuse the canonical callback-less ref + UNLOCK_WEAKREFS(obj); + return ref; + } + UNLOCK_WEAKREFS(obj); + + // We may have to allocate a new weakref + PyWeakReference *result = (PyWeakReference *) (type->tp_alloc)(type, 0); + if (result == NULL) { + return NULL; + } + init_weakref(result, obj, callback); + + LOCK_WEAKREFS(obj); get_basic_refs(*list, &ref, &proxy); if (callback == NULL && type == &_PyWeakref_RefType) { -#ifdef Py_GIL_DISABLED - if (ref != NULL && _Py_TryIncref((PyObject **)&ref, (PyObject *)ref)) { - /* We can re-use an existing reference. */ + if (ref != NULL && _Py_TryIncref((PyObject **) &ref, (PyObject *) ref)) { + // Someone else created and inserted the canonical callback-less + // weakref while the lock was released. + UNLOCK_WEAKREFS(obj); + Py_DECREF(result); return ref; } -#else - if (ref != NULL) { - /* We can re-use an existing reference. */ - return (PyWeakReference *)Py_NewRef(ref); - } -#endif + // Our newly created weakref is the canonical callback-less weakref. + insert_head(result, list); } - /* We have to create a new reference. */ - /* Note: the tp_alloc() can trigger cyclic GC, so the weakref - list on ob can be mutated. This means that the ref and - proxy pointers we got back earlier may have been collected, - so we need to compute these values again before we use - them. */ - PyWeakReference *self = (PyWeakReference *) (type->tp_alloc(type, 0)); - if (self != NULL) { - if (init_weakref(self, ob, callback) < 0) { - return NULL; - } - if (callback == NULL && type == &_PyWeakref_RefType) { - insert_head(self, list); - } - else { - PyWeakReference *prev; + else { + PyWeakReference *prev; + prev = (proxy == NULL) ? ref : proxy; + if (prev == NULL) + insert_head(result, list); + else + insert_after(result, prev); - get_basic_refs(*list, &ref, &proxy); - prev = (proxy == NULL) ? ref : proxy; - if (prev == NULL) - insert_head(self, list); - else - insert_after(self, prev); - } } - return self; + UNLOCK_WEAKREFS(obj); + + return result; } +#endif + static PyObject * weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) { @@ -578,7 +494,6 @@ weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) PyObject *ob, *callback = NULL; if (parse_weakref_init_args("__new__", args, kwargs, &ob, &callback)) { - if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(ob))) { PyErr_Format(PyExc_TypeError, "cannot create weak reference to '%s' object", @@ -587,9 +502,43 @@ weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) } if (callback == Py_None) callback = NULL; - Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(ob)); - self = new_weakref_lock_held(type, ob, callback); - Py_END_CRITICAL_SECTION(); +#ifdef Py_GIL_DISABLED + self = get_or_create_weakref_subtype(type, ob, callback); +#else + PyWeakReference *ref, *proxy; + PyWeakReference **list; + list = GET_WEAKREFS_LISTPTR(ob); + get_basic_refs(*list, &ref, &proxy); + if (callback == NULL && type == &_PyWeakref_RefType) { + if (ref != NULL) { + /* We can re-use an existing reference. */ + return (PyWeakReference *)Py_NewRef(ref); + } + } + /* We have to create a new reference. */ + /* Note: the tp_alloc() can trigger cyclic GC, so the weakref + list on ob can be mutated. This means that the ref and + proxy pointers we got back earlier may have been collected, + so we need to compute these values again before we use + them. */ + self = (PyWeakReference *) (type->tp_alloc(type, 0)); + if (self != NULL) { + init_weakref(self, ob, callback); + if (callback == NULL && type == &_PyWeakref_RefType) { + insert_head(self, list); + } + else { + PyWeakReference *prev; + + get_basic_refs(*list, &ref, &proxy); + prev = (proxy == NULL) ? ref : proxy; + if (prev == NULL) + insert_head(self, list); + else + insert_after(self, prev); + } + } +#endif } return (PyObject *)self; } @@ -1035,13 +984,62 @@ _PyWeakref_CallableProxyType = { }; +#ifdef Py_GIL_DISABLED + +static PyWeakReference * +get_or_create_weakref(PyObject *obj, PyObject *callback) +{ + PyWeakReference **list = GET_WEAKREFS_LISTPTR(obj); + PyWeakReference *ref, *proxy; + + LOCK_WEAKREFS(obj); + get_basic_refs(*list, &ref, &proxy); + if ((callback == NULL) && (ref != NULL) && _Py_TryIncref((PyObject**) &ref, (PyObject *) ref)) { + // We can reuse the canonical callback-less ref + UNLOCK_WEAKREFS(obj); + return ref; + } + UNLOCK_WEAKREFS(obj); + + // We may have to allocate a new weakref + PyWeakReference *result = new_weakref(obj, callback); + if (result == NULL) { + return NULL; + } + + LOCK_WEAKREFS(obj); + get_basic_refs(*list, &ref, &proxy); + if (callback == NULL) { + if (ref != NULL && _Py_TryIncref((PyObject **) &ref, (PyObject *) ref)) { + // Someone else created and inserted the canonical callback-less + // weakref while the lock was released. + UNLOCK_WEAKREFS(obj); + Py_DECREF(result); + return ref; + } + // Our newly created weakref is the canonical callback-less weakref. + insert_head(result, list); + } + else { + PyWeakReference *prev; + prev = (proxy == NULL) ? ref : proxy; + if (prev == NULL) + insert_head(result, list); + else + insert_after(result, prev); + + } + UNLOCK_WEAKREFS(obj); + + return result; +} + +#endif PyObject * PyWeakref_NewRef(PyObject *ob, PyObject *callback) { PyWeakReference *result = NULL; - PyWeakReference **list; - PyWeakReference *ref, *proxy; if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(ob))) { PyErr_Format(PyExc_TypeError, @@ -1049,40 +1047,27 @@ PyWeakref_NewRef(PyObject *ob, PyObject *callback) Py_TYPE(ob)->tp_name); return NULL; } - list = GET_WEAKREFS_LISTPTR(ob); - /* NB: For free-threaded builds its critical that no code inside the - * critical section can release it. We need to recompute ref/proxy after - * any code that may release the critical section. - */ - Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(ob)); - get_basic_refs(*list, &ref, &proxy); if (callback == Py_None) callback = NULL; +#ifdef Py_GIL_DISABLED + result = get_or_create_weakref(ob, callback); +#else + PyWeakReference *ref, *proxy; + PyWeakReference **list = GET_WEAKREFS_LISTPTR(ob); + get_basic_refs(*list, &ref, &proxy); if (callback == NULL) /* return existing weak reference if it exists */ result = ref; -#ifdef Py_GIL_DISABLED - /* Incref will fail if the existing weakref is being destroyed */ - if (result == NULL || - !_Py_TryIncref((PyObject **)&result, (PyObject *)result)) { -#else if (result != NULL) Py_INCREF(result); else { -#endif /* We do not need to recompute ref/proxy; new_weakref() cannot trigger GC. */ result = new_weakref(ob, callback); if (result != NULL) { if (callback == NULL) { -#ifdef Py_GIL_DISABLED - /* In free-threaded builds ref can be not-NULL if it is being - destroyed concurrently. - */ -#else assert(ref == NULL); -#endif insert_head(result, list); } else { @@ -1096,17 +1081,68 @@ PyWeakref_NewRef(PyObject *ob, PyObject *callback) } } } - Py_END_CRITICAL_SECTION(); +#endif return (PyObject *) result; } +#ifdef Py_GIL_DISABLED + +static PyWeakReference * +get_or_create_proxy(PyObject *obj, PyObject *callback) +{ + PyWeakReference **list = GET_WEAKREFS_LISTPTR(obj); + PyWeakReference *ref, *proxy; + + LOCK_WEAKREFS(obj); + get_basic_refs(*list, &ref, &proxy); + if ((callback == NULL) && (proxy != NULL) && _Py_TryIncref((PyObject**) &proxy, (PyObject *) proxy)) { + // We can reuse the canonical callback-less proxy + UNLOCK_WEAKREFS(obj); + return proxy; + } + UNLOCK_WEAKREFS(obj); + + // We may have to allocate a new weakref + PyWeakReference *result = new_weakref(obj, callback); + if (result == NULL) { + return NULL; + } + if (PyCallable_Check(obj)) { + Py_SET_TYPE(result, &_PyWeakref_CallableProxyType); + } + else { + Py_SET_TYPE(result, &_PyWeakref_ProxyType); + } + + LOCK_WEAKREFS(obj); + get_basic_refs(*list, &ref, &proxy); + if (callback == NULL && proxy != NULL && _Py_TryIncref((PyObject **) &proxy, (PyObject *) proxy)) { + // Someone else created and inserted the canonical callback-less + // proxy while the lock was released. + UNLOCK_WEAKREFS(obj); + Py_DECREF(result); + return proxy; + } + PyWeakReference *prev; + if (callback == NULL) { + prev = ref; + } + else + prev = (proxy == NULL) ? ref : proxy; + if (prev == NULL) + insert_head(result, list); + else + insert_after(result, prev); + UNLOCK_WEAKREFS(obj); + + return result; +} +#endif PyObject * PyWeakref_NewProxy(PyObject *ob, PyObject *callback) { PyWeakReference *result = NULL; - PyWeakReference **list; - PyWeakReference *ref, *proxy; if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(ob))) { PyErr_Format(PyExc_TypeError, @@ -1114,27 +1150,20 @@ PyWeakref_NewProxy(PyObject *ob, PyObject *callback) Py_TYPE(ob)->tp_name); return NULL; } - list = GET_WEAKREFS_LISTPTR(ob); - /* NB: For free-threaded builds its critical that no code inside the - * critical section can release it. We need to recompute ref/proxy after - * any code that may release the critical section. - */ - Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(ob)); - get_basic_refs(*list, &ref, &proxy); if (callback == Py_None) callback = NULL; +#ifdef Py_GIL_DISABLED + result = get_or_create_proxy(ob, callback); +#else + PyWeakReference *ref, *proxy; + PyWeakReference **list = GET_WEAKREFS_LISTPTR(ob); + get_basic_refs(*list, &ref, &proxy); if (callback == NULL) /* attempt to return an existing weak reference if it exists */ result = proxy; -#ifdef Py_GIL_DISABLED - /* Incref will fail if the existing weakref is being destroyed */ - if (result == NULL || - !_Py_TryIncref((PyObject **)&result, (PyObject *)result)) { -#else if (result != NULL) Py_INCREF(result); else { -#endif /* We do not need to recompute ref/proxy; new_weakref cannot trigger GC. */ @@ -1160,7 +1189,7 @@ PyWeakref_NewProxy(PyObject *ob, PyObject *callback) insert_after(result, prev); } } - Py_END_CRITICAL_SECTION(); +#endif return (PyObject *) result; } @@ -1212,56 +1241,6 @@ handle_callback(PyWeakReference *ref, PyObject *callback) Py_DECREF(cbresult); } -#ifdef Py_GIL_DISABLED - -// Clear the weakrefs and invoke their callbacks. -static void -clear_weakrefs_and_invoke_callbacks_lock_held(PyWeakReference **list) -{ - Py_ssize_t num_weakrefs = _PyWeakref_GetWeakrefCount(*list); - if (num_weakrefs == 0) { - return; - } - - PyObject *exc = PyErr_GetRaisedException(); - PyObject *tuple = PyTuple_New(num_weakrefs * 2); - if (tuple == NULL) { - _PyErr_ChainExceptions1(exc); - return; - } - - int num_items = 0; - while (*list != NULL) { - PyWeakReference *cur = *list; - int live = _Py_TryIncref((PyObject **) &cur, (PyObject *) cur); - PyObject *callback = clear_weakref_and_take_callback(cur); - if (live) { - assert(num_items / 2 < num_weakrefs); - PyTuple_SET_ITEM(tuple, num_items, (PyObject *) cur); - PyTuple_SET_ITEM(tuple, num_items + 1, callback); - num_items += 2; - } - else { - Py_DECREF(callback); - } - } - - for (int i = 0; i < num_items; i += 2) { - PyObject *callback = PyTuple_GET_ITEM(tuple, i + 1); - if (callback != NULL) { - PyObject *weakref = PyTuple_GET_ITEM(tuple, i); - handle_callback((PyWeakReference *)weakref, callback); - } - } - - Py_DECREF(tuple); - - assert(!PyErr_Occurred()); - PyErr_SetRaisedException(exc); -} - -#endif - /* This function is called by the tp_dealloc handler to clear weak references. * * This iterates through the weak references for 'object' and calls callbacks @@ -1286,20 +1265,73 @@ PyObject_ClearWeakRefs(PyObject *object) // Fast path for the common case return; } - Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(object)); /* Remove the callback-less basic and proxy references, which always appear at the head of the list. There may be two of each - one live and one in the process of being destroyed. */ - while (*list != NULL && (*list)->wr_callback == NULL) { - clear_weakref(*list); + for (int done = 0; !done;) { + PyObject *callback = NULL; + LOCK_WEAKREFS(object); + if (*list != NULL && (*list)->wr_callback == NULL) { + clear_weakref_lock_held(*list, &callback); + done = (*list == NULL); + } + else { + done = 1; + } + UNLOCK_WEAKREFS(object); + Py_XDECREF(callback); } /* Deal with non-canonical (subtypes or refs with callbacks) references. */ - clear_weakrefs_and_invoke_callbacks_lock_held(list); + Py_ssize_t num_weakrefs = _PyWeakref_GetWeakrefCountThreadsafe(object); + if (num_weakrefs == 0) { + return; + } - Py_END_CRITICAL_SECTION(); + PyObject *exc = PyErr_GetRaisedException(); + PyObject *tuple = PyTuple_New(num_weakrefs * 2); + if (tuple == NULL) { + _PyErr_ChainExceptions1(exc); + return; + } + + int num_items = 0; + for (int done = 0; !done;) { + PyObject *callback = NULL; + LOCK_WEAKREFS(object); + PyWeakReference *cur = *list; + if (cur != NULL) { + clear_weakref_lock_held(cur, &callback); + if (_Py_TryIncref((PyObject **) &cur, (PyObject *) cur)) { + assert(num_items / 2 < num_weakrefs); + PyTuple_SET_ITEM(tuple, num_items, (PyObject *) cur); + PyTuple_SET_ITEM(tuple, num_items + 1, callback); + num_items += 2; + callback = NULL; + } + } + else { + done = 1; + } + UNLOCK_WEAKREFS(object); + + Py_XDECREF(callback); + } + + for (int i = 0; i < num_items; i += 2) { + PyObject *callback = PyTuple_GET_ITEM(tuple, i + 1); + if (callback != NULL) { + PyObject *weakref = PyTuple_GET_ITEM(tuple, i); + handle_callback((PyWeakReference *)weakref, callback); + } + } + + Py_DECREF(tuple); + + assert(!PyErr_Occurred()); + PyErr_SetRaisedException(exc); #else /* Remove the callback-less basic and proxy references */ if (*list != NULL && (*list)->wr_callback == NULL) { @@ -1376,14 +1408,25 @@ _PyStaticType_ClearWeakRefs(PyInterpreterState *interp, PyTypeObject *type) { static_builtin_state *state = _PyStaticType_GetState(interp, type); PyObject **list = _PyStaticType_GET_WEAKREFS_LISTPTR(state); - Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(type)); +#ifdef Py_GIL_DISABLED + for (int done = 0; !done;) { + PyObject *callback = NULL; + LOCK_WEAKREFS(type); + if (*list != NULL) { + clear_weakref_lock_held((PyWeakReference*)*list, &callback); + } + done = (*list == NULL); + UNLOCK_WEAKREFS(type); + Py_XDECREF(callback); + } +#else while (*list != NULL) { /* Note that clear_weakref() pops the first ref off the type's weaklist before clearing its wr_object and wr_callback. That is how we're able to loop over the list. */ clear_weakref((PyWeakReference *)*list); } - Py_END_CRITICAL_SECTION(); +#endif } void @@ -1395,15 +1438,9 @@ _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj) (slightly) more costly _PyObject_GET_WEAKREFS_LISTPTR(). */ PyWeakReference **list = \ _PyObject_GET_WEAKREFS_LISTPTR_FROM_OFFSET(obj); -#ifdef Py_GIL_DISABLED - Py_BEGIN_CRITICAL_SECTION_MU(WEAKREF_LIST_LOCK(obj)); - while (*list != NULL) { - clear_weakref_and_leave_callback((PyWeakReference *)*list); - } - Py_END_CRITICAL_SECTION(); -#else + LOCK_WEAKREFS(obj); while (*list) { _PyWeakref_ClearRef(*list); } -#endif + UNLOCK_WEAKREFS(obj); } diff --git a/Python/pystate.c b/Python/pystate.c index 02c44337954c9f..4b8febab7741c5 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -500,6 +500,13 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) for (size_t i = 0; i < Py_ARRAY_LENGTH(locks); i++) { _PyMutex_at_fork_reinit(locks[i]); } +#ifdef Py_GIL_DISABLED + for (PyInterpreterState *interp = runtime->interpreters.head; interp != NULL; interp = interp->next) { + for (int i = 0; i < NUM_WEAKREF_LIST_LOCKS; i++) { + _PyMutex_at_fork_reinit(&interp->weakref_locks[i]); + } + } +#endif _PyTypes_AfterFork(); From 8c85fd389194c3b54801c6be205aa7b9ad8ef6f6 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 26 Mar 2024 19:15:05 -0700 Subject: [PATCH 28/66] Refactor weakref creation --- Objects/weakrefobject.c | 370 ++++++++++++---------------------------- 1 file changed, 108 insertions(+), 262 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 838d5c595fcf2e..c7ea6acaf873cd 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -427,120 +427,142 @@ insert_head(PyWeakReference *newref, PyWeakReference **list) *list = newref; } +/* See if we can reuse either the basic ref or proxy instead of creating a new + * weakref + */ +static PyWeakReference * +try_reuse_basic_ref(PyWeakReference *ref, PyWeakReference *proxy, PyTypeObject *type, PyObject *callback) +{ + if (callback != NULL) { + return NULL; + } + + PyWeakReference *cand = NULL; + if (type == &_PyWeakref_RefType) { + cand = ref; + } + if (type == &_PyWeakref_ProxyType) { + cand = proxy; + } + +#ifdef Py_GIL_DISABLED + if (cand != NULL && !_Py_TryIncref((PyObject **) &cand, (PyObject *) cand)) { + cand = NULL; + } +#else + Py_XINCREF(cand); +#endif + + return cand; +} + static int -parse_weakref_init_args(const char *funcname, PyObject *args, PyObject *kwargs, - PyObject **obp, PyObject **callbackp) +is_basic_ref(PyWeakReference *ref) { - return PyArg_UnpackTuple(args, funcname, 1, 2, obp, callbackp); + return (ref->wr_callback == NULL) && PyWeakref_CheckRefExact(ref); } -#ifdef Py_GIL_DISABLED +static int +is_basic_proxy(PyWeakReference *proxy) +{ + return (proxy->wr_callback == NULL) && PyWeakref_CheckProxy(proxy); +} + +/* Return the node that `newref` should be inserted after or NULL if `newref` + * should be inserted at the head of the list. + */ +static PyWeakReference * +get_prev(PyWeakReference *newref, PyWeakReference *ref, PyWeakReference *proxy) +{ + if (is_basic_ref(newref)) { + return NULL; + } + if (is_basic_proxy(newref)) { + return ref; + } + return (proxy == NULL) ? ref : proxy; +} + +typedef PyWeakReference * (*weakref_alloc_fn)(PyTypeObject *, PyObject *, PyObject *); static PyWeakReference * -get_or_create_weakref_subtype(PyTypeObject *type, PyObject *obj, PyObject *callback) +get_or_create_weakref(PyTypeObject *type, weakref_alloc_fn allocate, PyObject *obj, PyObject *callback) { + if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(obj))) { + PyErr_Format(PyExc_TypeError, + "cannot create weak reference to '%s' object", + Py_TYPE(obj)->tp_name); + return NULL; + } + if (callback == Py_None) + callback = NULL; + PyWeakReference **list = GET_WEAKREFS_LISTPTR(obj); PyWeakReference *ref, *proxy; LOCK_WEAKREFS(obj); get_basic_refs(*list, &ref, &proxy); - if ((callback == NULL) && (type == &_PyWeakref_RefType) && (ref != NULL) && _Py_TryIncref((PyObject**) &ref, (PyObject *) ref)) { - // We can reuse the canonical callback-less ref + PyWeakReference *basic_ref = try_reuse_basic_ref(ref, proxy, type, callback); + if (basic_ref != NULL) { UNLOCK_WEAKREFS(obj); - return ref; + return basic_ref; } UNLOCK_WEAKREFS(obj); - // We may have to allocate a new weakref - PyWeakReference *result = (PyWeakReference *) (type->tp_alloc)(type, 0); - if (result == NULL) { + PyWeakReference *newref = allocate(type, obj, callback); + if (newref == NULL) { return NULL; } - init_weakref(result, obj, callback); LOCK_WEAKREFS(obj); + /* A new basic ref or proxy may be inserted if the GIL is released during + * allocation in default builds or while the weakref lock is released in + * free-threaded builds. + */ get_basic_refs(*list, &ref, &proxy); - if (callback == NULL && type == &_PyWeakref_RefType) { - if (ref != NULL && _Py_TryIncref((PyObject **) &ref, (PyObject *) ref)) { - // Someone else created and inserted the canonical callback-less - // weakref while the lock was released. - UNLOCK_WEAKREFS(obj); - Py_DECREF(result); - return ref; - } - // Our newly created weakref is the canonical callback-less weakref. - insert_head(result, list); - } - else { - PyWeakReference *prev; - prev = (proxy == NULL) ? ref : proxy; - if (prev == NULL) - insert_head(result, list); - else - insert_after(result, prev); - + basic_ref = try_reuse_basic_ref(ref, proxy, type, callback); + if (basic_ref != NULL) { + UNLOCK_WEAKREFS(obj); + Py_DECREF(newref); + return basic_ref; } + PyWeakReference *prev = get_prev(newref, ref, proxy); + if (prev == NULL) + insert_head(newref, list); + else + insert_after(newref, prev); UNLOCK_WEAKREFS(obj); - return result; + return newref; +} + +static int +parse_weakref_init_args(const char *funcname, PyObject *args, PyObject *kwargs, + PyObject **obp, PyObject **callbackp) +{ + return PyArg_UnpackTuple(args, funcname, 1, 2, obp, callbackp); } -#endif + +static PyWeakReference * +allocate_ref_subtype(PyTypeObject *type, PyObject *obj, PyObject *callback) +{ + PyWeakReference *result = (PyWeakReference *) (type->tp_alloc)(type, 0); + if (result == NULL) { + return NULL; + } + init_weakref(result, obj, callback); + return result; +} static PyObject * weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) { - PyWeakReference *self = NULL; PyObject *ob, *callback = NULL; - if (parse_weakref_init_args("__new__", args, kwargs, &ob, &callback)) { - if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(ob))) { - PyErr_Format(PyExc_TypeError, - "cannot create weak reference to '%s' object", - Py_TYPE(ob)->tp_name); - return NULL; - } - if (callback == Py_None) - callback = NULL; -#ifdef Py_GIL_DISABLED - self = get_or_create_weakref_subtype(type, ob, callback); -#else - PyWeakReference *ref, *proxy; - PyWeakReference **list; - list = GET_WEAKREFS_LISTPTR(ob); - get_basic_refs(*list, &ref, &proxy); - if (callback == NULL && type == &_PyWeakref_RefType) { - if (ref != NULL) { - /* We can re-use an existing reference. */ - return (PyWeakReference *)Py_NewRef(ref); - } - } - /* We have to create a new reference. */ - /* Note: the tp_alloc() can trigger cyclic GC, so the weakref - list on ob can be mutated. This means that the ref and - proxy pointers we got back earlier may have been collected, - so we need to compute these values again before we use - them. */ - self = (PyWeakReference *) (type->tp_alloc(type, 0)); - if (self != NULL) { - init_weakref(self, ob, callback); - if (callback == NULL && type == &_PyWeakref_RefType) { - insert_head(self, list); - } - else { - PyWeakReference *prev; - - get_basic_refs(*list, &ref, &proxy); - prev = (proxy == NULL) ? ref : proxy; - if (prev == NULL) - insert_head(self, list); - else - insert_after(self, prev); - } - } -#endif + return (PyObject *) get_or_create_weakref(type, allocate_ref_subtype, ob, callback); } - return (PyObject *)self; + return NULL; } static int @@ -983,126 +1005,21 @@ _PyWeakref_CallableProxyType = { proxy_iternext, /* tp_iternext */ }; - -#ifdef Py_GIL_DISABLED - static PyWeakReference * -get_or_create_weakref(PyObject *obj, PyObject *callback) +allocate_ref(PyTypeObject *type, PyObject *obj, PyObject *callback) { - PyWeakReference **list = GET_WEAKREFS_LISTPTR(obj); - PyWeakReference *ref, *proxy; - - LOCK_WEAKREFS(obj); - get_basic_refs(*list, &ref, &proxy); - if ((callback == NULL) && (ref != NULL) && _Py_TryIncref((PyObject**) &ref, (PyObject *) ref)) { - // We can reuse the canonical callback-less ref - UNLOCK_WEAKREFS(obj); - return ref; - } - UNLOCK_WEAKREFS(obj); - - // We may have to allocate a new weakref - PyWeakReference *result = new_weakref(obj, callback); - if (result == NULL) { - return NULL; - } - - LOCK_WEAKREFS(obj); - get_basic_refs(*list, &ref, &proxy); - if (callback == NULL) { - if (ref != NULL && _Py_TryIncref((PyObject **) &ref, (PyObject *) ref)) { - // Someone else created and inserted the canonical callback-less - // weakref while the lock was released. - UNLOCK_WEAKREFS(obj); - Py_DECREF(result); - return ref; - } - // Our newly created weakref is the canonical callback-less weakref. - insert_head(result, list); - } - else { - PyWeakReference *prev; - prev = (proxy == NULL) ? ref : proxy; - if (prev == NULL) - insert_head(result, list); - else - insert_after(result, prev); - - } - UNLOCK_WEAKREFS(obj); - - return result; + return new_weakref(obj, callback); } -#endif - PyObject * PyWeakref_NewRef(PyObject *ob, PyObject *callback) { - PyWeakReference *result = NULL; - - if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(ob))) { - PyErr_Format(PyExc_TypeError, - "cannot create weak reference to '%s' object", - Py_TYPE(ob)->tp_name); - return NULL; - } - if (callback == Py_None) - callback = NULL; -#ifdef Py_GIL_DISABLED - result = get_or_create_weakref(ob, callback); -#else - PyWeakReference *ref, *proxy; - PyWeakReference **list = GET_WEAKREFS_LISTPTR(ob); - get_basic_refs(*list, &ref, &proxy); - if (callback == NULL) - /* return existing weak reference if it exists */ - result = ref; - if (result != NULL) - Py_INCREF(result); - else { - /* We do not need to recompute ref/proxy; new_weakref() cannot - trigger GC. - */ - result = new_weakref(ob, callback); - if (result != NULL) { - if (callback == NULL) { - assert(ref == NULL); - insert_head(result, list); - } - else { - PyWeakReference *prev; - - prev = (proxy == NULL) ? ref : proxy; - if (prev == NULL) - insert_head(result, list); - else - insert_after(result, prev); - } - } - } -#endif - return (PyObject *) result; + return (PyObject *) get_or_create_weakref(&_PyWeakref_RefType, allocate_ref, ob, callback); } -#ifdef Py_GIL_DISABLED - static PyWeakReference * -get_or_create_proxy(PyObject *obj, PyObject *callback) +allocate_proxy(PyTypeObject *type, PyObject *obj, PyObject *callback) { - PyWeakReference **list = GET_WEAKREFS_LISTPTR(obj); - PyWeakReference *ref, *proxy; - - LOCK_WEAKREFS(obj); - get_basic_refs(*list, &ref, &proxy); - if ((callback == NULL) && (proxy != NULL) && _Py_TryIncref((PyObject**) &proxy, (PyObject *) proxy)) { - // We can reuse the canonical callback-less proxy - UNLOCK_WEAKREFS(obj); - return proxy; - } - UNLOCK_WEAKREFS(obj); - - // We may have to allocate a new weakref PyWeakReference *result = new_weakref(obj, callback); if (result == NULL) { return NULL; @@ -1113,84 +1030,13 @@ get_or_create_proxy(PyObject *obj, PyObject *callback) else { Py_SET_TYPE(result, &_PyWeakref_ProxyType); } - - LOCK_WEAKREFS(obj); - get_basic_refs(*list, &ref, &proxy); - if (callback == NULL && proxy != NULL && _Py_TryIncref((PyObject **) &proxy, (PyObject *) proxy)) { - // Someone else created and inserted the canonical callback-less - // proxy while the lock was released. - UNLOCK_WEAKREFS(obj); - Py_DECREF(result); - return proxy; - } - PyWeakReference *prev; - if (callback == NULL) { - prev = ref; - } - else - prev = (proxy == NULL) ? ref : proxy; - if (prev == NULL) - insert_head(result, list); - else - insert_after(result, prev); - UNLOCK_WEAKREFS(obj); - return result; } -#endif PyObject * PyWeakref_NewProxy(PyObject *ob, PyObject *callback) { - PyWeakReference *result = NULL; - - if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(ob))) { - PyErr_Format(PyExc_TypeError, - "cannot create weak reference to '%s' object", - Py_TYPE(ob)->tp_name); - return NULL; - } - if (callback == Py_None) - callback = NULL; -#ifdef Py_GIL_DISABLED - result = get_or_create_proxy(ob, callback); -#else - PyWeakReference *ref, *proxy; - PyWeakReference **list = GET_WEAKREFS_LISTPTR(ob); - get_basic_refs(*list, &ref, &proxy); - if (callback == NULL) - /* attempt to return an existing weak reference if it exists */ - result = proxy; - if (result != NULL) - Py_INCREF(result); - else { - /* We do not need to recompute ref/proxy; new_weakref cannot - trigger GC. - */ - result = new_weakref(ob, callback); - if (result != NULL) { - PyWeakReference *prev; - - if (PyCallable_Check(ob)) { - Py_SET_TYPE(result, &_PyWeakref_CallableProxyType); - } - else { - Py_SET_TYPE(result, &_PyWeakref_ProxyType); - } - if (callback == NULL) { - prev = ref; - } - else - prev = (proxy == NULL) ? ref : proxy; - - if (prev == NULL) - insert_head(result, list); - else - insert_after(result, prev); - } - } -#endif - return (PyObject *) result; + return (PyObject *) get_or_create_weakref(&_PyWeakref_ProxyType, allocate_proxy, ob, callback); } From 0021bc5101ff2302a98198b42c4605c04d0c5eb9 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 27 Mar 2024 11:33:01 -0700 Subject: [PATCH 29/66] Fixups --- Include/internal/pycore_weakref.h | 1 + Modules/_weakref.c | 1 + Objects/weakrefobject.c | 22 +++++++++++----------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 788ea97be8d1c2..8a8e55fd63c8a9 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -53,6 +53,7 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) // clear_weakref() was called return NULL; } + if (_is_dead(obj)) { return NULL; } diff --git a/Modules/_weakref.c b/Modules/_weakref.c index bee2f38c3f0742..73a5fec6d5252a 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -32,6 +32,7 @@ _weakref_getweakrefcount_impl(PyObject *module, PyObject *object) return _PyWeakref_GetWeakrefCountThreadsafe(object); } + static int is_dead_weakref(PyObject *value) { diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index c7ea6acaf873cd..8e988ded3c4162 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -54,9 +54,7 @@ * them, but we only hold borrowed references and they may also be destroyed * concurrently. * - * We can probably solve this by using QSBR to ensure that the memory backing - * either the weakref or the referenced object is not freed until we're ready, - * but for now we've chosen to work around it using a few different strategies: + * For now we've chosen to address this in a straightforward way: * * - The weakref's hash is protected using the weakref's per-object lock. * - The other mutable is protected by a striped lock owned by the interpreter. @@ -79,6 +77,7 @@ Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head) { Py_ssize_t count = 0; + while (head != NULL) { ++count; head = head->wr_next; @@ -468,6 +467,12 @@ is_basic_proxy(PyWeakReference *proxy) return (proxy->wr_callback == NULL) && PyWeakref_CheckProxy(proxy); } +static int +is_basic_ref_or_proxy(PyWeakReference *wr) +{ + return is_basic_ref(wr) || is_basic_proxy(wr); +} + /* Return the node that `newref` should be inserted after or NULL if `newref` * should be inserted at the head of the list. */ @@ -1119,13 +1124,10 @@ PyObject_ClearWeakRefs(PyObject *object) for (int done = 0; !done;) { PyObject *callback = NULL; LOCK_WEAKREFS(object); - if (*list != NULL && (*list)->wr_callback == NULL) { + if (*list != NULL && is_basic_ref_or_proxy(*list)) { clear_weakref_lock_held(*list, &callback); - done = (*list == NULL); - } - else { - done = 1; } + done = (*list == NULL) || !is_basic_ref_or_proxy(*list); UNLOCK_WEAKREFS(object); Py_XDECREF(callback); } @@ -1158,9 +1160,7 @@ PyObject_ClearWeakRefs(PyObject *object) callback = NULL; } } - else { - done = 1; - } + done = (*list == NULL); UNLOCK_WEAKREFS(object); Py_XDECREF(callback); From 40d99d486b2f87ea141f3d6b39651dbafea04150 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 27 Mar 2024 11:36:37 -0700 Subject: [PATCH 30/66] Fix formatting --- Include/internal/pycore_weakref.h | 9 ++++++--- Objects/weakrefobject.c | 27 +++++++++++++++++---------- Python/pystate.c | 3 ++- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 8a8e55fd63c8a9..3fef1e0ea60115 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -14,9 +14,12 @@ extern "C" { #ifdef Py_GIL_DISABLED -#define WEAKREF_LIST_LOCK(obj) _PyInterpreterState_GET()->weakref_locks[((uintptr_t) obj) % NUM_WEAKREF_LIST_LOCKS] +#define WEAKREF_LIST_LOCK(obj) \ + _PyInterpreterState_GET() \ + ->weakref_locks[((uintptr_t)obj) % NUM_WEAKREF_LIST_LOCKS] -#define LOCK_WEAKREFS(obj) PyMutex_LockFlags(&WEAKREF_LIST_LOCK(obj), _Py_LOCK_DONT_DETACH) +#define LOCK_WEAKREFS(obj) \ + PyMutex_LockFlags(&WEAKREF_LIST_LOCK(obj), _Py_LOCK_DONT_DETACH) #define UNLOCK_WEAKREFS(obj) PyMutex_Unlock(&WEAKREF_LIST_LOCK(obj)) #else @@ -58,7 +61,7 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) return NULL; } assert(Py_REFCNT(obj) > 0); - return Py_NewRef(obj); + return Py_NewRef(obj); #else PyObject *obj = _Py_atomic_load_ptr(&ref->wr_object); if (obj == Py_None) { diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 8e988ded3c4162..d1908b16aa2bf8 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -138,9 +138,9 @@ clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) if (_Py_atomic_load_ptr(&self->wr_object) != Py_None) { PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); if (*list == self) - /* If 'self' is the end of the list (and thus self->wr_next == NULL) - then the weakref list itself (and thus the value of *list) will - end up being set to NULL. */ + /* If 'self' is the end of the list (and thus self->wr_next == + NULL) then the weakref list itself (and thus the value of *list) + will end up being set to NULL. */ _Py_atomic_store_ptr(list, self->wr_next); _Py_atomic_store_ptr(&self->wr_object, Py_None); if (self->wr_prev != NULL) @@ -430,7 +430,8 @@ insert_head(PyWeakReference *newref, PyWeakReference **list) * weakref */ static PyWeakReference * -try_reuse_basic_ref(PyWeakReference *ref, PyWeakReference *proxy, PyTypeObject *type, PyObject *callback) +try_reuse_basic_ref(PyWeakReference *ref, PyWeakReference *proxy, + PyTypeObject *type, PyObject *callback) { if (callback != NULL) { return NULL; @@ -488,10 +489,12 @@ get_prev(PyWeakReference *newref, PyWeakReference *ref, PyWeakReference *proxy) return (proxy == NULL) ? ref : proxy; } -typedef PyWeakReference * (*weakref_alloc_fn)(PyTypeObject *, PyObject *, PyObject *); +typedef PyWeakReference *(*weakref_alloc_fn)(PyTypeObject *, PyObject *, + PyObject *); static PyWeakReference * -get_or_create_weakref(PyTypeObject *type, weakref_alloc_fn allocate, PyObject *obj, PyObject *callback) +get_or_create_weakref(PyTypeObject *type, weakref_alloc_fn allocate, + PyObject *obj, PyObject *callback) { if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(obj))) { PyErr_Format(PyExc_TypeError, @@ -507,7 +510,8 @@ get_or_create_weakref(PyTypeObject *type, weakref_alloc_fn allocate, PyObject *o LOCK_WEAKREFS(obj); get_basic_refs(*list, &ref, &proxy); - PyWeakReference *basic_ref = try_reuse_basic_ref(ref, proxy, type, callback); + PyWeakReference *basic_ref = + try_reuse_basic_ref(ref, proxy, type, callback); if (basic_ref != NULL) { UNLOCK_WEAKREFS(obj); return basic_ref; @@ -565,7 +569,8 @@ weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) { PyObject *ob, *callback = NULL; if (parse_weakref_init_args("__new__", args, kwargs, &ob, &callback)) { - return (PyObject *) get_or_create_weakref(type, allocate_ref_subtype, ob, callback); + return (PyObject *)get_or_create_weakref(type, allocate_ref_subtype, + ob, callback); } return NULL; } @@ -1019,7 +1024,8 @@ allocate_ref(PyTypeObject *type, PyObject *obj, PyObject *callback) PyObject * PyWeakref_NewRef(PyObject *ob, PyObject *callback) { - return (PyObject *) get_or_create_weakref(&_PyWeakref_RefType, allocate_ref, ob, callback); + return (PyObject *)get_or_create_weakref(&_PyWeakref_RefType, allocate_ref, + ob, callback); } static PyWeakReference * @@ -1041,7 +1047,8 @@ allocate_proxy(PyTypeObject *type, PyObject *obj, PyObject *callback) PyObject * PyWeakref_NewProxy(PyObject *ob, PyObject *callback) { - return (PyObject *) get_or_create_weakref(&_PyWeakref_ProxyType, allocate_proxy, ob, callback); + return (PyObject *)get_or_create_weakref(&_PyWeakref_ProxyType, + allocate_proxy, ob, callback); } diff --git a/Python/pystate.c b/Python/pystate.c index 4b8febab7741c5..eab8318e1e08ea 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -501,7 +501,8 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) _PyMutex_at_fork_reinit(locks[i]); } #ifdef Py_GIL_DISABLED - for (PyInterpreterState *interp = runtime->interpreters.head; interp != NULL; interp = interp->next) { + for (PyInterpreterState *interp = runtime->interpreters.head; + interp != NULL; interp = interp->next) { for (int i = 0; i < NUM_WEAKREF_LIST_LOCKS; i++) { _PyMutex_at_fork_reinit(&interp->weakref_locks[i]); } From dd922b1f3b72e92c76942a3da1d6e486e11860e5 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 26 Mar 2024 16:01:16 -0700 Subject: [PATCH 31/66] Use QSBR to avoid locking in PyType_IsSubtype --- Include/internal/pycore_pyatomic_ft_wrappers.h | 3 +++ Objects/typeobject.c | 12 +++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index e441600d54e1aa..969cda7514c1d2 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -23,6 +23,8 @@ extern "C" { #define FT_ATOMIC_LOAD_SSIZE(value) _Py_atomic_load_ssize(&value) #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) \ _Py_atomic_load_ssize_relaxed(&value) +#define FT_ATOMIC_STORE_PTR(value, new_value) \ + _Py_atomic_store_ptr(&value, new_value) #define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) \ _Py_atomic_store_ptr_relaxed(&value, new_value) #define FT_ATOMIC_STORE_PTR_RELEASE(value, new_value) \ @@ -32,6 +34,7 @@ extern "C" { #else #define FT_ATOMIC_LOAD_SSIZE(value) value #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) value +#define FT_ATOMIC_STORE_PTR(value, new_value) value = new_value #define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_STORE_PTR_RELEASE(value, new_value) value = new_value #define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) value = new_value diff --git a/Objects/typeobject.c b/Objects/typeobject.c index c3965d0ab379af..1876e970a640bc 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -2,6 +2,7 @@ #include "Python.h" #include "pycore_abstract.h" // _PySequence_IterSearch() +#include "pycore_pyatomic_ft_wrappers.h" #include "pycore_call.h" // _PyObject_VectorcallTstate() #include "pycore_code.h" // CO_FAST_FREE #include "pycore_dict.h" // _PyDict_KeysSize() @@ -404,7 +405,13 @@ set_tp_mro(PyTypeObject *self, PyObject *mro) /* Other checks are done via set_tp_bases. */ _Py_SetImmortal(mro); } - self->tp_mro = mro; +#ifdef Py_GIL_DISABLED + if (self->tp_mro != NULL) { + // Allow concurrent reads from PyType_IsSubtype + _PyObject_GC_SET_SHARED(self->tp_mro); + } +#endif + FT_ATOMIC_STORE_PTR(self->tp_mro, mro); } static inline void @@ -2334,9 +2341,8 @@ int PyType_IsSubtype(PyTypeObject *a, PyTypeObject *b) { #ifdef Py_GIL_DISABLED - PyObject *mro = _PyType_GetMRO(a); + PyObject *mro = _Py_atomic_load_ptr(&a->tp_mro); int res = is_subtype_with_mro(mro, a, b); - Py_XDECREF(mro); return res; #else return is_subtype_with_mro(lookup_tp_mro(a), a, b); From 681872e2a7e6aec43f985d3ce67690db919f72ea Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 27 Mar 2024 11:47:46 -0700 Subject: [PATCH 32/66] Remove unnecessary include --- Objects/dictobject.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 8b038930050fa8..536746ca41eed5 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -129,7 +129,6 @@ As a consequence of this, split keys have a maximum size of 16. #include "pycore_pyerrors.h" // _PyErr_GetRaisedException() #include "pycore_pystate.h" // _PyThreadState_GET() #include "pycore_setobject.h" // _PySet_NextEntry() -#include "pycore_weakref.h" #include "stringlib/eq.h" // unicode_eq() #include From 89828318b8483b0f976fe4c3d62bc2a1d3833200 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 27 Mar 2024 11:50:44 -0700 Subject: [PATCH 33/66] Fix unused function warning in default builds --- Objects/weakrefobject.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index d1908b16aa2bf8..3c90eddd0a2f37 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -468,12 +468,16 @@ is_basic_proxy(PyWeakReference *proxy) return (proxy->wr_callback == NULL) && PyWeakref_CheckProxy(proxy); } +#ifdef Py_GIL_DISABLED + static int is_basic_ref_or_proxy(PyWeakReference *wr) { return is_basic_ref(wr) || is_basic_proxy(wr); } +#endif + /* Return the node that `newref` should be inserted after or NULL if `newref` * should be inserted at the head of the list. */ From 334c766c57e5346368f5277ab904a69dbf47aa22 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 28 Mar 2024 16:14:50 -0700 Subject: [PATCH 34/66] Check for callable proxy --- Objects/weakrefobject.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 3c90eddd0a2f37..2317ba58937980 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -441,7 +441,8 @@ try_reuse_basic_ref(PyWeakReference *ref, PyWeakReference *proxy, if (type == &_PyWeakref_RefType) { cand = ref; } - if (type == &_PyWeakref_ProxyType) { + if ((type == &_PyWeakref_ProxyType) || + (type == &_PyWeakref_CallableProxyType)) { cand = proxy; } From 05494454b8ab09db61b7fc4b235a976a773f082a Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 28 Mar 2024 17:04:48 -0700 Subject: [PATCH 35/66] Refactor weakref creation --- Objects/weakrefobject.c | 96 +++++++++++++++++++++-------------------- 1 file changed, 50 insertions(+), 46 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 2317ba58937980..c29a9a11ffb42b 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -426,17 +426,19 @@ insert_head(PyWeakReference *newref, PyWeakReference **list) *list = newref; } -/* See if we can reuse either the basic ref or proxy instead of creating a new - * weakref +/* See if we can reuse either the basic ref or proxy in list instead of + * creating a new weakref */ static PyWeakReference * -try_reuse_basic_ref(PyWeakReference *ref, PyWeakReference *proxy, - PyTypeObject *type, PyObject *callback) +try_reuse_basic_ref(PyWeakReference *list, PyTypeObject *type, PyObject *callback) { if (callback != NULL) { return NULL; } + PyWeakReference *ref, *proxy; + get_basic_refs(list, &ref, &proxy); + PyWeakReference *cand = NULL; if (type == &_PyWeakref_RefType) { cand = ref; @@ -479,19 +481,30 @@ is_basic_ref_or_proxy(PyWeakReference *wr) #endif -/* Return the node that `newref` should be inserted after or NULL if `newref` - * should be inserted at the head of the list. - */ -static PyWeakReference * -get_prev(PyWeakReference *newref, PyWeakReference *ref, PyWeakReference *proxy) +/* Insert `newref` in the appropriate position in `list` */ +static void +insert_weakref(PyWeakReference *newref, PyWeakReference **list) { + PyWeakReference *ref, *proxy; + get_basic_refs(*list, &ref, &proxy); + + PyWeakReference *prev; if (is_basic_ref(newref)) { - return NULL; + prev = NULL; } - if (is_basic_proxy(newref)) { - return ref; + else if (is_basic_proxy(newref)) { + prev = ref; + } + else { + prev = (proxy == NULL) ? ref : proxy; + } + + if (prev == NULL) { + insert_head(newref, list); + } + else { + insert_after(newref, prev); } - return (proxy == NULL) ? ref : proxy; } typedef PyWeakReference *(*weakref_alloc_fn)(PyTypeObject *, PyObject *, @@ -511,43 +524,34 @@ get_or_create_weakref(PyTypeObject *type, weakref_alloc_fn allocate, callback = NULL; PyWeakReference **list = GET_WEAKREFS_LISTPTR(obj); - PyWeakReference *ref, *proxy; - - LOCK_WEAKREFS(obj); - get_basic_refs(*list, &ref, &proxy); - PyWeakReference *basic_ref = - try_reuse_basic_ref(ref, proxy, type, callback); - if (basic_ref != NULL) { + if ((type == &_PyWeakref_RefType) || + (type == &_PyWeakref_ProxyType) || + (type == &_PyWeakref_CallableProxyType)) { + LOCK_WEAKREFS(obj); + PyWeakReference *basic_ref = try_reuse_basic_ref(*list, type, callback); + if (basic_ref != NULL) { + UNLOCK_WEAKREFS(obj); + return basic_ref; + } + PyWeakReference *newref = allocate(type, obj, callback); + if (newref == NULL) { + return NULL; + } + insert_weakref(newref, list); UNLOCK_WEAKREFS(obj); - return basic_ref; + return newref; } - UNLOCK_WEAKREFS(obj); - - PyWeakReference *newref = allocate(type, obj, callback); - if (newref == NULL) { - return NULL; - } - - LOCK_WEAKREFS(obj); - /* A new basic ref or proxy may be inserted if the GIL is released during - * allocation in default builds or while the weakref lock is released in - * free-threaded builds. - */ - get_basic_refs(*list, &ref, &proxy); - basic_ref = try_reuse_basic_ref(ref, proxy, type, callback); - if (basic_ref != NULL) { + else { + // We may not be able to safely allocate inside the lock + PyWeakReference *newref = allocate(type, obj, callback); + if (newref == NULL) { + return NULL; + } + LOCK_WEAKREFS(obj); + insert_weakref(newref, list); UNLOCK_WEAKREFS(obj); - Py_DECREF(newref); - return basic_ref; + return newref; } - PyWeakReference *prev = get_prev(newref, ref, proxy); - if (prev == NULL) - insert_head(newref, list); - else - insert_after(newref, prev); - UNLOCK_WEAKREFS(obj); - - return newref; } static int From eb12c06a4d1995080d0448fa39bb6f13e4175bc3 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 28 Mar 2024 17:10:17 -0700 Subject: [PATCH 36/66] Simplify comment --- Objects/weakrefobject.c | 41 +++-------------------------------------- 1 file changed, 3 insertions(+), 38 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index c29a9a11ffb42b..be413dfade3ac6 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -14,50 +14,15 @@ * * In free-threaded builds we need to protect mutable state of: * - * - The weakref (the referenced object, the hash) + * - The weakref (wr_object, wr_hash) * - The referenced object (its head-of-list pointer) * - The linked list of weakrefs * - * The above may be modified concurrently when creating weakrefs, destroying - * weakrefs, or destroying the referenced object. - * - * To avoid diverging from the default implementation too much, we must - * implement the following: - * - * - When a weakref is destroyed it must be removed from the linked list and - * its borrowed reference must be cleared. - * - When the referenced object is destroyed each of its weak references must - * be removed from the linked list and their internal reference cleared. - * - * A natural arrangement would be to use the per-object lock in the referenced - * object to protect its mutable state and the linked list. Similarly, we could - * use the per-object in each weakref to protect the weakref's mutable - * state. This is complicated by a few things: - * - * - The referenced object and the weakrefs only hold borrowed references to - * each other. - * - The referenced object and its weakrefs may be destroyed concurrently. - * - The GC may run while the referenced object or the weakrefs are being - * destroyed and free their counterpart. - * - * If a weakref needs to be unlinked from the linked list during destruction, - * we need to ensure that the memory backing the referenced object is not freed - * during this process. Since the referenced object may be destroyed - * concurrently, we cannot rely on being able to incref it. To complicate - * matters further, if we do not hold a strong reference we cannot guarantee - * that the if the GC runs it won't reclaim the referenced object, and we - * cannot guarantee that GC won't run (unless it is disabled) because lock - * acquisition may suspend. - * - * The inverse is true when destroying the referenced object. We must ensure - * that the memory backing the weakrefs remains alive while we're unlinking - * them, but we only hold borrowed references and they may also be destroyed - * concurrently. - * * For now we've chosen to address this in a straightforward way: * * - The weakref's hash is protected using the weakref's per-object lock. - * - The other mutable is protected by a striped lock owned by the interpreter. + * - The other mutable is protected by a striped lock keyed on the referenced + * object's address. * - The striped lock must be locked using `_Py_LOCK_DONT_DETACH` in order to * support atomic deletion from WeakValueDictionaries. As a result, we must * be careful not to perform any operations that could suspend while the From 700cf4c3e90e7816faf1dc4bc057fd0a8eabe873 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 28 Mar 2024 17:28:42 -0700 Subject: [PATCH 37/66] Get rid of GetWeakrefCountThreadsafe --- Include/internal/pycore_weakref.h | 6 +----- Modules/_weakref.c | 7 ++----- Objects/weakrefobject.c | 25 +++++++++---------------- 3 files changed, 12 insertions(+), 26 deletions(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 3fef1e0ea60115..7cc2a924529ae1 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -111,11 +111,7 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) return ret; } -// NB: In free-threaded builds the weakref list lock for the referenced object -// must be held around calls to this function. -extern Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head); - -extern Py_ssize_t _PyWeakref_GetWeakrefCountThreadsafe(PyObject *obj); +extern Py_ssize_t _PyWeakref_GetWeakrefCount(PyObject *obj); // Clear all the weak references to obj but leave their callbacks uncalled and // intact. diff --git a/Modules/_weakref.c b/Modules/_weakref.c index 73a5fec6d5252a..cfc4cd00d9259a 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -26,10 +26,7 @@ static Py_ssize_t _weakref_getweakrefcount_impl(PyObject *module, PyObject *object) /*[clinic end generated code: output=301806d59558ff3e input=7d4d04fcaccf64d5]*/ { - if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(object))) { - return 0; - } - return _PyWeakref_GetWeakrefCountThreadsafe(object); + return _PyWeakref_GetWeakrefCount(object); } @@ -90,7 +87,7 @@ _weakref_getweakrefs(PyObject *module, PyObject *object) } PyWeakReference **list = GET_WEAKREFS_LISTPTR(object); - Py_ssize_t count = _PyWeakref_GetWeakrefCountThreadsafe(object); + Py_ssize_t count = _PyWeakref_GetWeakrefCount(object); PyObject *result = PyList_New(count); if (result == NULL) { diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index be413dfade3ac6..fc7fb61ed8c7f2 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -39,26 +39,19 @@ Py_ssize_t -_PyWeakref_GetWeakrefCount(PyWeakReference *head) +_PyWeakref_GetWeakrefCount(PyObject *obj) { - Py_ssize_t count = 0; + if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(obj))) { + return 0; + } + LOCK_WEAKREFS(obj); + Py_ssize_t count = 0; + PyWeakReference *head = *GET_WEAKREFS_LISTPTR(obj); while (head != NULL) { ++count; head = head->wr_next; } - return count; -} - -Py_ssize_t -_PyWeakref_GetWeakrefCountThreadsafe(PyObject *obj) -{ - if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(obj))) { - return 0; - } - Py_ssize_t count; - LOCK_WEAKREFS(obj); - count = _PyWeakref_GetWeakrefCount(*GET_WEAKREFS_LISTPTR(obj)); UNLOCK_WEAKREFS(obj); return count; } @@ -1114,7 +1107,7 @@ PyObject_ClearWeakRefs(PyObject *object) } /* Deal with non-canonical (subtypes or refs with callbacks) references. */ - Py_ssize_t num_weakrefs = _PyWeakref_GetWeakrefCountThreadsafe(object); + Py_ssize_t num_weakrefs = _PyWeakref_GetWeakrefCount(object); if (num_weakrefs == 0) { return; } @@ -1168,7 +1161,7 @@ PyObject_ClearWeakRefs(PyObject *object) } if (*list != NULL) { PyWeakReference *current = *list; - Py_ssize_t count = _PyWeakref_GetWeakrefCount(current); + Py_ssize_t count = _PyWeakref_GetWeakrefCount(object); PyObject *exc = PyErr_GetRaisedException(); if (count == 1) { From 63200cdb9f6f5c026f7f9708a83247304c9f1def Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 28 Mar 2024 17:35:39 -0700 Subject: [PATCH 38/66] Don't explicitly zero initialize striped lock --- Python/pystate.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index eab8318e1e08ea..7b2e8a4ae9fcc4 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -625,9 +625,6 @@ init_interpreter(PyInterpreterState *interp, _PyType_InitCache(interp); #ifdef Py_GIL_DISABLED _Py_brc_init_state(interp); - for (int i = 0; i < NUM_WEAKREF_LIST_LOCKS; i++) { - interp->weakref_locks[i] = (PyMutex){0}; - } #endif llist_init(&interp->mem_free_queue.head); for (int i = 0; i < _PY_MONITORING_UNGROUPED_EVENTS; i++) { From e9eed727244252167a938ce9044a2eb504dfa686 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 28 Mar 2024 17:39:20 -0700 Subject: [PATCH 39/66] Indent conditional define --- Include/internal/pycore_interp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index b459405578cc90..80e874a9d90625 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -62,7 +62,7 @@ struct _stoptheworld_state { #ifdef Py_GIL_DISABLED // This should be prime but otherwise the choice is arbitrary. A larger value // increases concurrency at the expense of memory. -#define NUM_WEAKREF_LIST_LOCKS 127 +# define NUM_WEAKREF_LIST_LOCKS 127 #endif /* cross-interpreter data registry */ From e3d5779aea8b34e205ec7e6dab8ec20435d3b0c2 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 28 Mar 2024 17:55:07 -0700 Subject: [PATCH 40/66] Use non-atomic loads while we hold the lock --- Include/internal/pycore_weakref.h | 5 ++--- Objects/weakrefobject.c | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 7cc2a924529ae1..39a9c91466e5f5 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -69,7 +69,7 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) return NULL; } LOCK_WEAKREFS(obj); - if (_Py_atomic_load_ptr(&ref->wr_object) == Py_None) { + if (ref->wr_object == Py_None) { // clear_weakref() was called UNLOCK_WEAKREFS(obj); return NULL; @@ -101,8 +101,7 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) LOCK_WEAKREFS(obj); // See _PyWeakref_GET_REF() for the rationale of this test #ifdef Py_GIL_DISABLED - PyObject *obj_reloaded = _Py_atomic_load_ptr(&ref->wr_object); - ret = (obj_reloaded == Py_None) || _is_dead(obj); + ret = (ref->wr_object == Py_None) || _is_dead(obj); #else ret = _is_dead(obj); #endif diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index fc7fb61ed8c7f2..b82e12d9be7be8 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -93,7 +93,7 @@ static void clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) { // TODO: Assert locks are held or world is stopped - if (_Py_atomic_load_ptr(&self->wr_object) != Py_None) { + if (self->wr_object != Py_None) { PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); if (*list == self) /* If 'self' is the end of the list (and thus self->wr_next == From f31f1e553f937dd4f9ef3dbe5bac6622d5b8f156 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 28 Mar 2024 18:02:37 -0700 Subject: [PATCH 41/66] Refactor a _PyWeakref_GET_REF and _PyWeakref_IS_DEAD to share more code --- Include/internal/pycore_pyatomic_ft_wrappers.h | 2 ++ Include/internal/pycore_weakref.h | 16 ++++------------ 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index 969cda7514c1d2..2514f51f1b0086 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -20,6 +20,7 @@ extern "C" { #endif #ifdef Py_GIL_DISABLED +#define FT_ATOMIC_LOAD_PTR(value) _Py_atomic_load_ptr(&value) #define FT_ATOMIC_LOAD_SSIZE(value) _Py_atomic_load_ssize(&value) #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) \ _Py_atomic_load_ssize_relaxed(&value) @@ -32,6 +33,7 @@ extern "C" { #define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) \ _Py_atomic_store_ssize_relaxed(&value, new_value) #else +#define FT_ATOMIC_LOAD_PTR(value) value #define FT_ATOMIC_LOAD_SSIZE(value) value #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) value #define FT_ATOMIC_STORE_PTR(value, new_value) value = new_value diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 39a9c91466e5f5..58b91a6b4f9810 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -11,6 +11,7 @@ extern "C" { #include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION() #include "pycore_lock.h" #include "pycore_object.h" // _Py_REF_IS_MERGED() +#include "pycore_pyatomic_ft_wrappers.h" #ifdef Py_GIL_DISABLED @@ -50,24 +51,19 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) assert(PyWeakref_Check(ref_obj)); PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); -#if !defined(Py_GIL_DISABLED) - PyObject *obj = ref->wr_object; + PyObject *obj = FT_ATOMIC_LOAD_PTR(ref->wr_object); if (obj == Py_None) { // clear_weakref() was called return NULL; } +#if !defined(Py_GIL_DISABLED) if (_is_dead(obj)) { return NULL; } assert(Py_REFCNT(obj) > 0); return Py_NewRef(obj); #else - PyObject *obj = _Py_atomic_load_ptr(&ref->wr_object); - if (obj == Py_None) { - // clear_weakref() was called - return NULL; - } LOCK_WEAKREFS(obj); if (ref->wr_object == Py_None) { // clear_weakref() was called @@ -88,11 +84,7 @@ static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) assert(PyWeakref_Check(ref_obj)); int ret = 0; PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj); -#ifdef Py_GIL_DISABLED - PyObject *obj = _Py_atomic_load_ptr(&ref->wr_object); -#else - PyObject *obj = ref->wr_object; -#endif + PyObject *obj = FT_ATOMIC_LOAD_PTR(ref->wr_object); if (obj == Py_None) { // clear_weakref() was called ret = 1; From b5c95617d204fd4adee2547878d9e6b853b218ae Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 09:16:47 -0700 Subject: [PATCH 42/66] Merge helper functions for clearing weakrefs --- Objects/weakrefobject.c | 51 +++++++---------------------------------- 1 file changed, 8 insertions(+), 43 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index b82e12d9be7be8..5c4cf5e1afd2c8 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -86,7 +86,6 @@ new_weakref(PyObject *ob, PyObject *callback) return result; } -#ifdef Py_GIL_DISABLED // Clear the weakref and steal its callback into `callback`, if provided. static void @@ -99,8 +98,8 @@ clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) /* If 'self' is the end of the list (and thus self->wr_next == NULL) then the weakref list itself (and thus the value of *list) will end up being set to NULL. */ - _Py_atomic_store_ptr(list, self->wr_next); - _Py_atomic_store_ptr(&self->wr_object, Py_None); + FT_ATOMIC_STORE_PTR(*list, self->wr_next); + FT_ATOMIC_STORE_PTR(self->wr_object, Py_None); if (self->wr_prev != NULL) self->wr_prev->wr_next = self->wr_next; if (self->wr_next != NULL) @@ -114,7 +113,10 @@ clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) } } -// Clear the weakref while the world is stopped +#ifdef Py_GIL_DISABLED + +// Clear the weakref while the world is stopped. This is called during GC in +// free-threaded builds and can't lock. static void gc_clear_weakref(PyWeakReference *self) { @@ -123,14 +125,12 @@ gc_clear_weakref(PyWeakReference *self) Py_XDECREF(callback); } +#endif + // Clear the weakref and its callback static void clear_weakref(PyWeakReference *self) { - PyObject *object = _Py_atomic_load_ptr(&self->wr_object); - if (object == Py_None) { - return; - } PyObject *callback = NULL; LOCK_WEAKREFS(object); clear_weakref_lock_held(self, &callback); @@ -138,41 +138,6 @@ clear_weakref(PyWeakReference *self) Py_XDECREF(callback); } -#else - -/* This function clears the passed-in reference and removes it from the - * list of weak references for the referent. This is the only code that - * removes an item from the doubly-linked list of weak references for an - * object; it is also responsible for clearing the callback slot. - */ -static void -clear_weakref(PyWeakReference *self) -{ - PyObject *callback = self->wr_callback; - - if (self->wr_object != Py_None) { - PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); - - if (*list == self) - /* If 'self' is the end of the list (and thus self->wr_next == NULL) - then the weakref list itself (and thus the value of *list) will - end up being set to NULL. */ - *list = self->wr_next; - self->wr_object = Py_None; - if (self->wr_prev != NULL) - self->wr_prev->wr_next = self->wr_next; - if (self->wr_next != NULL) - self->wr_next->wr_prev = self->wr_prev; - self->wr_prev = NULL; - self->wr_next = NULL; - } - if (callback != NULL) { - Py_DECREF(callback); - self->wr_callback = NULL; - } -} - -#endif // Py_GIL_DISABLED /* Cyclic gc uses this to *just* clear the passed-in reference, leaving * the callback intact and uncalled. It must be possible to call self's From 93425e676edcdb529b93a947c09047f8c89b2d92 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 09:17:26 -0700 Subject: [PATCH 43/66] Make _PyStaticType_ClearWeakRefs have one implementation for default and free-threaded builds --- Objects/weakrefobject.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 5c4cf5e1afd2c8..26b7359d481930 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1193,7 +1193,6 @@ _PyStaticType_ClearWeakRefs(PyInterpreterState *interp, PyTypeObject *type) { static_builtin_state *state = _PyStaticType_GetState(interp, type); PyObject **list = _PyStaticType_GET_WEAKREFS_LISTPTR(state); -#ifdef Py_GIL_DISABLED for (int done = 0; !done;) { PyObject *callback = NULL; LOCK_WEAKREFS(type); @@ -1204,14 +1203,6 @@ _PyStaticType_ClearWeakRefs(PyInterpreterState *interp, PyTypeObject *type) UNLOCK_WEAKREFS(type); Py_XDECREF(callback); } -#else - while (*list != NULL) { - /* Note that clear_weakref() pops the first ref off the type's - weaklist before clearing its wr_object and wr_callback. - That is how we're able to loop over the list. */ - clear_weakref((PyWeakReference *)*list); - } -#endif } void From 89edf028e00ee8de8d7790afc8a2b6243e831482 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 11:30:05 -0700 Subject: [PATCH 44/66] Make sure we're able to index into the striped lock even if GC clears us --- Include/cpython/weakrefobject.h | 7 +++++++ Lib/test/test_sys.py | 8 ++++++-- Objects/weakrefobject.c | 12 ++++++++++-- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/Include/cpython/weakrefobject.h b/Include/cpython/weakrefobject.h index 1559e2def61260..626f33dc733a19 100644 --- a/Include/cpython/weakrefobject.h +++ b/Include/cpython/weakrefobject.h @@ -30,6 +30,13 @@ struct _PyWeakReference { PyWeakReference *wr_prev; PyWeakReference *wr_next; vectorcallfunc vectorcall; + +#ifdef Py_GIL_DISABLED + /* Used in free-threaded builds to provide a stable index into the striped + * lock. Always contains the original value of wr_object. + */ + PyObject *orig_object; +#endif }; Py_DEPRECATED(3.13) static inline PyObject* PyWeakref_GET_OBJECT(PyObject *ref_obj) diff --git a/Lib/test/test_sys.py b/Lib/test/test_sys.py index 37c16cd1047885..9da536c1686bbf 100644 --- a/Lib/test/test_sys.py +++ b/Lib/test/test_sys.py @@ -1707,11 +1707,15 @@ class newstyleclass(object): pass # TODO: add check that forces layout of unicodefields # weakref import weakref - check(weakref.ref(int), size('2Pn3P')) + if support.Py_GIL_DISABLED: + expected = size('2Pn4P') + else: + expected = size('2Pn3P') + check(weakref.ref(int), expected) # weakproxy # XXX # weakcallableproxy - check(weakref.proxy(int), size('2Pn3P')) + check(weakref.proxy(int), expected) def check_slots(self, obj, base, extra): expected = sys.getsizeof(base) + struct.calcsize(extra) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 26b7359d481930..6b40afa70c270c 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -68,6 +68,7 @@ init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) self->wr_callback = Py_XNewRef(callback); self->vectorcall = weakref_vectorcall; #ifdef Py_GIL_DISABLED + self->orig_object = ob; _PyObject_SetMaybeWeakref(ob); _PyObject_SetMaybeWeakref((PyObject *)self); #endif @@ -132,9 +133,16 @@ static void clear_weakref(PyWeakReference *self) { PyObject *callback = NULL; - LOCK_WEAKREFS(object); +#ifdef Py_GIL_DISABLED + // self->wr_object may be Py_None if the GC cleared the weakref, however, + // we still need the original value of wr_object so that we can lock the + // appropriate lock to clear the callback. + LOCK_WEAKREFS(self->orig_object); +#endif clear_weakref_lock_held(self, &callback); - UNLOCK_WEAKREFS(object); +#ifdef Py_GIL_DISABLED + UNLOCK_WEAKREFS(self->orig_object); +#endif Py_XDECREF(callback); } From 7f734807ea432a2ff73b7a8afd8e3dc65e88cd1b Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 11:36:56 -0700 Subject: [PATCH 45/66] Consolidate implementations of _PyWeakref_ClearRef and gc_clear --- Objects/weakrefobject.c | 32 +++++--------------------------- 1 file changed, 5 insertions(+), 27 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 6b40afa70c270c..13a2701e3d3416 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -114,20 +114,6 @@ clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) } } -#ifdef Py_GIL_DISABLED - -// Clear the weakref while the world is stopped. This is called during GC in -// free-threaded builds and can't lock. -static void -gc_clear_weakref(PyWeakReference *self) -{ - PyObject *callback = NULL; - clear_weakref_lock_held(self, &callback); - Py_XDECREF(callback); -} - -#endif - // Clear the weakref and its callback static void clear_weakref(PyWeakReference *self) @@ -163,15 +149,7 @@ _PyWeakref_ClearRef(PyWeakReference *self) { assert(self != NULL); assert(PyWeakref_Check(self)); -#ifdef Py_GIL_DISABLED clear_weakref_lock_held(self, NULL); -#else - /* Preserve and restore the callback around clear_weakref. */ - PyObject *callback = self->wr_callback; - self->wr_callback = NULL; - clear_weakref(self); - self->wr_callback = callback; -#endif } static void @@ -194,11 +172,11 @@ gc_traverse(PyWeakReference *self, visitproc visit, void *arg) static int gc_clear(PyWeakReference *self) { -#ifdef Py_GIL_DISABLED - gc_clear_weakref(self); -#else - clear_weakref(self); -#endif + PyObject *callback; + // The world is stopped during GC in free-threaded builds. It's safe to + // call this without holding the lock. + clear_weakref_lock_held(self, &callback); + Py_XDECREF(callback); return 0; } From d742ed13a7dcb309aee4cdc27f3c488e86f8f197 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 11:47:20 -0700 Subject: [PATCH 46/66] Rename `_Py_TryIncref` to `_Py_TryIncrefCompare` --- Include/internal/pycore_object.h | 6 +++--- Include/internal/pycore_weakref.h | 2 +- Modules/_weakref.c | 2 +- Objects/dictobject.c | 8 ++++---- Objects/typeobject.c | 2 +- Objects/weakrefobject.c | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 8126eea9983a71..2ab72f5c698c0e 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -426,7 +426,7 @@ _Py_TryIncRefShared(PyObject *op) /* Tries to incref the object op and ensures that *src still points to it. */ static inline int -_Py_TryIncref(PyObject **src, PyObject *op) +_Py_TryIncrefCompare(PyObject **src, PyObject *op) { if (_Py_TryIncrefFast(op)) { return 1; @@ -452,7 +452,7 @@ _Py_XGetRef(PyObject **ptr) if (value == NULL) { return value; } - if (_Py_TryIncref(ptr, value)) { + if (_Py_TryIncrefCompare(ptr, value)) { return value; } } @@ -467,7 +467,7 @@ _Py_TryXGetRef(PyObject **ptr) if (value == NULL) { return value; } - if (_Py_TryIncref(ptr, value)) { + if (_Py_TryIncrefCompare(ptr, value)) { return value; } return NULL; diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 58b91a6b4f9810..bb6f9c0e99472b 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -70,7 +70,7 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) UNLOCK_WEAKREFS(obj); return NULL; } - if (_Py_TryIncref(&obj, obj)) { + if (_Py_TryIncrefCompare(&obj, obj)) { UNLOCK_WEAKREFS(obj); return obj; } diff --git a/Modules/_weakref.c b/Modules/_weakref.c index cfc4cd00d9259a..d205b578911bf9 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -100,7 +100,7 @@ _weakref_getweakrefs(PyObject *module, PyObject *object) PyWeakReference *current = *list; // Weakrefs may be added or removed since the count was computed. while (num_added < count && current != NULL) { - if (_Py_TryIncref((PyObject**) ¤t, (PyObject *) current)) { + if (_Py_TryIncrefCompare((PyObject**) ¤t, (PyObject *) current)) { PyList_SET_ITEM(result, num_added, current); num_added++; } diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 536746ca41eed5..66e284bd9d5137 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1285,7 +1285,7 @@ Py_ssize_t compare_unicode_generic_threadsafe(PyDictObject *mp, PyDictKeysObject assert(!PyUnicode_CheckExact(key)); if (startkey != NULL) { - if (!_Py_TryIncref(&ep->me_key, startkey)) { + if (!_Py_TryIncrefCompare(&ep->me_key, startkey)) { return DKIX_KEY_CHANGED; } @@ -1333,7 +1333,7 @@ compare_unicode_unicode_threadsafe(PyDictObject *mp, PyDictKeysObject *dk, return unicode_get_hash(startkey) == hash && unicode_eq(startkey, key); } else { - if (!_Py_TryIncref(&ep->me_key, startkey)) { + if (!_Py_TryIncrefCompare(&ep->me_key, startkey)) { return DKIX_KEY_CHANGED; } if (unicode_get_hash(startkey) == hash && unicode_eq(startkey, key)) { @@ -1363,7 +1363,7 @@ Py_ssize_t compare_generic_threadsafe(PyDictObject *mp, PyDictKeysObject *dk, } Py_ssize_t ep_hash = _Py_atomic_load_ssize_relaxed(&ep->me_hash); if (ep_hash == hash) { - if (startkey == NULL || !_Py_TryIncref(&ep->me_key, startkey)) { + if (startkey == NULL || !_Py_TryIncrefCompare(&ep->me_key, startkey)) { return DKIX_KEY_CHANGED; } int cmp = PyObject_RichCompareBool(startkey, key, Py_EQ); @@ -5284,7 +5284,7 @@ acquire_key_value(PyObject **key_loc, PyObject *value, PyObject **value_loc, } if (out_value) { - if (!_Py_TryIncref(value_loc, value)) { + if (!_Py_TryIncrefCompare(value_loc, value)) { if (out_key) { Py_DECREF(*out_key); } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 1876e970a640bc..0f61749c60bf5d 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -379,7 +379,7 @@ _PyType_GetMRO(PyTypeObject *self) if (mro == NULL) { return NULL; } - if (_Py_TryIncref(&self->tp_mro, mro)) { + if (_Py_TryIncrefCompare(&self->tp_mro, mro)) { return mro; } diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 13a2701e3d3416..092bacc83e988e 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -358,7 +358,7 @@ try_reuse_basic_ref(PyWeakReference *list, PyTypeObject *type, PyObject *callbac } #ifdef Py_GIL_DISABLED - if (cand != NULL && !_Py_TryIncref((PyObject **) &cand, (PyObject *) cand)) { + if (cand != NULL && !_Py_TryIncrefCompare((PyObject **) &cand, (PyObject *) cand)) { cand = NULL; } #else @@ -1077,7 +1077,7 @@ PyObject_ClearWeakRefs(PyObject *object) PyWeakReference *cur = *list; if (cur != NULL) { clear_weakref_lock_held(cur, &callback); - if (_Py_TryIncref((PyObject **) &cur, (PyObject *) cur)) { + if (_Py_TryIncrefCompare((PyObject **) &cur, (PyObject *) cur)) { assert(num_items / 2 < num_weakrefs); PyTuple_SET_ITEM(tuple, num_items, (PyObject *) cur); PyTuple_SET_ITEM(tuple, num_items + 1, callback); From 37f9cebb1bf4922ef06d2358561254fd1a9dfc74 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 12:04:10 -0700 Subject: [PATCH 47/66] Add `_Py_TryIncref` --- Include/internal/pycore_object.h | 15 +++++++++++++++ Include/internal/pycore_weakref.h | 12 +++--------- Modules/_weakref.c | 2 +- Objects/weakrefobject.c | 4 ++-- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 2ab72f5c698c0e..38e7b2ad981591 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -527,6 +527,21 @@ _PyObject_SetMaybeWeakref(PyObject *op) #endif +/* Tries to incref op and returns 1 if successful or 0 otherwise. */ +static inline int +_Py_TryIncref(PyObject *op) +{ +#ifdef Py_GIL_DISABLED + return _Py_TryIncrefFast(op) || _Py_TryIncRefShared(op); +#else + if (Py_REFCNT(op) > 0) { + Py_INCREF(op); + return 1; + } + return 0; +#endif +} + #ifdef Py_REF_DEBUG extern void _PyInterpreterState_FinalizeRefTotal(PyInterpreterState *); extern void _Py_FinalizeRefTotal(_PyRuntimeState *); diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index bb6f9c0e99472b..217f8074198d4e 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -57,26 +57,20 @@ static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj) return NULL; } -#if !defined(Py_GIL_DISABLED) - if (_is_dead(obj)) { - return NULL; - } - assert(Py_REFCNT(obj) > 0); - return Py_NewRef(obj); -#else LOCK_WEAKREFS(obj); +#ifdef Py_GIL_DISABLED if (ref->wr_object == Py_None) { // clear_weakref() was called UNLOCK_WEAKREFS(obj); return NULL; } - if (_Py_TryIncrefCompare(&obj, obj)) { +#endif + if (_Py_TryIncref(obj)) { UNLOCK_WEAKREFS(obj); return obj; } UNLOCK_WEAKREFS(obj); return NULL; -#endif } static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj) diff --git a/Modules/_weakref.c b/Modules/_weakref.c index d205b578911bf9..b7384ade362c2e 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -100,7 +100,7 @@ _weakref_getweakrefs(PyObject *module, PyObject *object) PyWeakReference *current = *list; // Weakrefs may be added or removed since the count was computed. while (num_added < count && current != NULL) { - if (_Py_TryIncrefCompare((PyObject**) ¤t, (PyObject *) current)) { + if (_Py_TryIncref((PyObject *) current)) { PyList_SET_ITEM(result, num_added, current); num_added++; } diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 092bacc83e988e..27d3d56aff99e5 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -358,7 +358,7 @@ try_reuse_basic_ref(PyWeakReference *list, PyTypeObject *type, PyObject *callbac } #ifdef Py_GIL_DISABLED - if (cand != NULL && !_Py_TryIncrefCompare((PyObject **) &cand, (PyObject *) cand)) { + if (cand != NULL && !_Py_TryIncref((PyObject *) cand)) { cand = NULL; } #else @@ -1077,7 +1077,7 @@ PyObject_ClearWeakRefs(PyObject *object) PyWeakReference *cur = *list; if (cur != NULL) { clear_weakref_lock_held(cur, &callback); - if (_Py_TryIncrefCompare((PyObject **) &cur, (PyObject *) cur)) { + if (_Py_TryIncref((PyObject *) cur)) { assert(num_items / 2 < num_weakrefs); PyTuple_SET_ITEM(tuple, num_items, (PyObject *) cur); PyTuple_SET_ITEM(tuple, num_items + 1, callback); From fb9eee5ba824f68731a1f2f79956c91fd145fada Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 12:22:02 -0700 Subject: [PATCH 48/66] Refactor `weakref._getweakrefs` to have one implementation --- Modules/_weakref.c | 52 +++++++++++++++------------------------------- 1 file changed, 17 insertions(+), 35 deletions(-) diff --git a/Modules/_weakref.c b/Modules/_weakref.c index b7384ade362c2e..1ea3ed5e40b761 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -86,50 +86,32 @@ _weakref_getweakrefs(PyObject *module, PyObject *object) return PyList_New(0); } - PyWeakReference **list = GET_WEAKREFS_LISTPTR(object); - Py_ssize_t count = _PyWeakref_GetWeakrefCount(object); - - PyObject *result = PyList_New(count); + PyObject *result = PyList_New(0); if (result == NULL) { return NULL; } -#ifdef Py_GIL_DISABLED - Py_ssize_t num_added = 0; LOCK_WEAKREFS(object); - PyWeakReference *current = *list; - // Weakrefs may be added or removed since the count was computed. - while (num_added < count && current != NULL) { - if (_Py_TryIncref((PyObject *) current)) { - PyList_SET_ITEM(result, num_added, current); - num_added++; + PyWeakReference *current = *GET_WEAKREFS_LISTPTR(object); + while (current != NULL) { + PyObject *curobj = (PyObject *) current; + if (_Py_TryIncref(curobj)) { + if (PyList_Append(result, curobj)) { + UNLOCK_WEAKREFS(object); + Py_DECREF(curobj); + Py_DECREF(result); + return NULL; + } + else { + // Undo our _Py_TryIncref. This is safe to do with the lock + // held in free-threaded builds; the list holds a reference to + // curobj so we're guaranteed not to invoke the destructor. + Py_DECREF(curobj); + } } current = current->wr_next; } UNLOCK_WEAKREFS(object); - - // Don't return an incomplete list - if (num_added != count) { - PyObject *new_list = PyList_New(num_added); - if (new_list == NULL) { - Py_DECREF(result); - return NULL; - } - for (Py_ssize_t i = 0; i < num_added; i++) { - PyObject *obj = PyList_GET_ITEM(result, i); - PyList_SET_ITEM(new_list, i, obj); - PyList_SET_ITEM(result, i, NULL); - } - Py_DECREF(result); - result = new_list; - } -#else - PyWeakReference *current = *list; - for (Py_ssize_t i = 0; i < count; ++i) { - PyList_SET_ITEM(result, i, Py_NewRef(current)); - current = current->wr_next; - } -#endif return result; } From d760b99f5d33d9a6e8e16adc31e5b4399bb1310c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 12:43:15 -0700 Subject: [PATCH 49/66] Refactor to have one implementation for `PyObject_ClearWeakRefs` --- Objects/weakrefobject.c | 74 ++--------------------------------------- 1 file changed, 3 insertions(+), 71 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 27d3d56aff99e5..754e187a2c70e5 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -380,16 +380,12 @@ is_basic_proxy(PyWeakReference *proxy) return (proxy->wr_callback == NULL) && PyWeakref_CheckProxy(proxy); } -#ifdef Py_GIL_DISABLED - static int is_basic_ref_or_proxy(PyWeakReference *wr) { return is_basic_ref(wr) || is_basic_proxy(wr); } -#endif - /* Insert `newref` in the appropriate position in `list` */ static void insert_weakref(PyWeakReference *newref, PyWeakReference **list) @@ -1035,16 +1031,15 @@ PyObject_ClearWeakRefs(PyObject *object) PyErr_BadInternalCall(); return; } + list = GET_WEAKREFS_LISTPTR(object); -#ifdef Py_GIL_DISABLED - if (_Py_atomic_load_ptr(list) == NULL) { + if (FT_ATOMIC_LOAD_PTR(list) == NULL) { // Fast path for the common case return; } /* Remove the callback-less basic and proxy references, which always appear - at the head of the list. There may be two of each - one live and one in - the process of being destroyed. + at the head of the list. */ for (int done = 0; !done;) { PyObject *callback = NULL; @@ -1103,69 +1098,6 @@ PyObject_ClearWeakRefs(PyObject *object) assert(!PyErr_Occurred()); PyErr_SetRaisedException(exc); -#else - /* Remove the callback-less basic and proxy references */ - if (*list != NULL && (*list)->wr_callback == NULL) { - clear_weakref(*list); - if (*list != NULL && (*list)->wr_callback == NULL) - clear_weakref(*list); - } - if (*list != NULL) { - PyWeakReference *current = *list; - Py_ssize_t count = _PyWeakref_GetWeakrefCount(object); - PyObject *exc = PyErr_GetRaisedException(); - - if (count == 1) { - PyObject *callback = current->wr_callback; - - current->wr_callback = NULL; - clear_weakref(current); - if (callback != NULL) { - if (Py_REFCNT((PyObject *)current) > 0) { - handle_callback(current, callback); - } - Py_DECREF(callback); - } - } - else { - PyObject *tuple; - Py_ssize_t i = 0; - - tuple = PyTuple_New(count * 2); - if (tuple == NULL) { - _PyErr_ChainExceptions1(exc); - return; - } - - for (i = 0; i < count; ++i) { - PyWeakReference *next = current->wr_next; - - if (Py_REFCNT((PyObject *)current) > 0) { - PyTuple_SET_ITEM(tuple, i * 2, Py_NewRef(current)); - PyTuple_SET_ITEM(tuple, i * 2 + 1, current->wr_callback); - } - else { - Py_DECREF(current->wr_callback); - } - current->wr_callback = NULL; - clear_weakref(current); - current = next; - } - for (i = 0; i < count; ++i) { - PyObject *callback = PyTuple_GET_ITEM(tuple, i * 2 + 1); - - /* The tuple may have slots left to NULL */ - if (callback != NULL) { - PyObject *item = PyTuple_GET_ITEM(tuple, i * 2); - handle_callback((PyWeakReference *)item, callback); - } - } - Py_DECREF(tuple); - } - assert(!PyErr_Occurred()); - PyErr_SetRaisedException(exc); - } -#endif // Py_GIL_DISABLED } /* This function is called by _PyStaticType_Dealloc() to clear weak references. From 491e4ff78c8c8d0ff98f3fc99a38b19f003f1709 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 12:44:46 -0700 Subject: [PATCH 50/66] Revert "Use QSBR to avoid locking in PyType_IsSubtype" This reverts commit dd922b1f3b72e92c76942a3da1d6e486e11860e5. --- Objects/typeobject.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 0f61749c60bf5d..722d3fbb3cf89c 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -2,7 +2,6 @@ #include "Python.h" #include "pycore_abstract.h" // _PySequence_IterSearch() -#include "pycore_pyatomic_ft_wrappers.h" #include "pycore_call.h" // _PyObject_VectorcallTstate() #include "pycore_code.h" // CO_FAST_FREE #include "pycore_dict.h" // _PyDict_KeysSize() @@ -405,13 +404,7 @@ set_tp_mro(PyTypeObject *self, PyObject *mro) /* Other checks are done via set_tp_bases. */ _Py_SetImmortal(mro); } -#ifdef Py_GIL_DISABLED - if (self->tp_mro != NULL) { - // Allow concurrent reads from PyType_IsSubtype - _PyObject_GC_SET_SHARED(self->tp_mro); - } -#endif - FT_ATOMIC_STORE_PTR(self->tp_mro, mro); + self->tp_mro = mro; } static inline void @@ -2341,8 +2334,9 @@ int PyType_IsSubtype(PyTypeObject *a, PyTypeObject *b) { #ifdef Py_GIL_DISABLED - PyObject *mro = _Py_atomic_load_ptr(&a->tp_mro); + PyObject *mro = _PyType_GetMRO(a); int res = is_subtype_with_mro(mro, a, b); + Py_XDECREF(mro); return res; #else return is_subtype_with_mro(lookup_tp_mro(a), a, b); From 531a076cfe66dd19b2fbdb626e8ee136abc8a234 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 12:47:46 -0700 Subject: [PATCH 51/66] Accept thread unsafety when checking subtype for now --- Objects/typeobject.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 722d3fbb3cf89c..a3ee898a03d519 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -2333,14 +2333,7 @@ is_subtype_with_mro(PyObject *a_mro, PyTypeObject *a, PyTypeObject *b) int PyType_IsSubtype(PyTypeObject *a, PyTypeObject *b) { -#ifdef Py_GIL_DISABLED - PyObject *mro = _PyType_GetMRO(a); - int res = is_subtype_with_mro(mro, a, b); - Py_XDECREF(mro); - return res; -#else - return is_subtype_with_mro(lookup_tp_mro(a), a, b); -#endif + return is_subtype_with_mro(a->tp_mro, a, b); } /* Routines to do a method lookup in the type without looking in the From ed280d8187418507af066a1f8591fa43201bd299 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 13:22:05 -0700 Subject: [PATCH 52/66] Update modules to use PyWeakref_GetRef So we don't need to export PyMutex_LockTimed and _Py_IncRefTotal --- Include/internal/pycore_lock.h | 2 +- Include/internal/pycore_object.h | 1 + Include/object.h | 1 - Modules/_sqlite/blob.c | 5 ++--- Modules/_sqlite/connection.c | 5 +++-- Modules/_ssl.c | 13 ++++++------- Modules/_ssl/debughelpers.c | 6 +++--- 7 files changed, 16 insertions(+), 17 deletions(-) diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index 4c6f4eaf87e71e..f993c95ecbf75a 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -113,7 +113,7 @@ typedef enum _PyLockFlags { // Lock a mutex with an optional timeout and additional options. See // _PyLockFlags for details. -PyAPI_FUNC(PyLockStatus) +extern PyLockStatus _PyMutex_LockTimed(PyMutex *m, PyTime_t timeout_ns, _PyLockFlags flags); // Lock a mutex with aditional options. See _PyLockFlags for details. diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 38e7b2ad981591..ba10764d1f8297 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -87,6 +87,7 @@ PyAPI_FUNC(void) _Py_NO_RETURN _Py_FatalRefcountErrorFunc( PyAPI_DATA(Py_ssize_t) _Py_RefTotal; extern void _Py_AddRefTotal(PyInterpreterState *, Py_ssize_t); +extern void _Py_IncRefTotal(PyInterpreterState *); extern void _Py_DecRefTotal(PyInterpreterState *); # define _Py_DEC_REFTOTAL(interp) \ diff --git a/Include/object.h b/Include/object.h index 207fce17d96489..67a5e514c421c3 100644 --- a/Include/object.h +++ b/Include/object.h @@ -766,7 +766,6 @@ PyAPI_FUNC(void) _Py_NegativeRefcount(const char *filename, int lineno, PyObject *op); PyAPI_FUNC(void) _Py_INCREF_IncRefTotal(void); PyAPI_FUNC(void) _Py_DECREF_DecRefTotal(void); -PyAPI_FUNC(void) _Py_IncRefTotal(PyInterpreterState *); #endif // Py_REF_DEBUG && !Py_LIMITED_API PyAPI_FUNC(void) _Py_Dealloc(PyObject *); diff --git a/Modules/_sqlite/blob.c b/Modules/_sqlite/blob.c index f099020c5f4e6f..7deb58bf1b9b82 100644 --- a/Modules/_sqlite/blob.c +++ b/Modules/_sqlite/blob.c @@ -4,7 +4,6 @@ #include "blob.h" #include "util.h" -#include "pycore_weakref.h" // _PyWeakref_GET_REF() #define clinic_state() (pysqlite_get_state_by_type(Py_TYPE(self))) #include "clinic/blob.c.h" @@ -102,8 +101,8 @@ pysqlite_close_all_blobs(pysqlite_Connection *self) { for (int i = 0; i < PyList_GET_SIZE(self->blobs); i++) { PyObject *weakref = PyList_GET_ITEM(self->blobs, i); - PyObject *blob = _PyWeakref_GET_REF(weakref); - if (blob == NULL) { + PyObject *blob; + if (!PyWeakref_GetRef(weakref, &blob)) { continue; } close_blob((pysqlite_Blob *)blob); diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index f97afcf5fcf16e..bd4f28ecda0176 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -38,7 +38,6 @@ #include "pycore_modsupport.h" // _PyArg_NoKeywords() #include "pycore_pyerrors.h" // _PyErr_ChainExceptions1() #include "pycore_pylifecycle.h" // _Py_IsInterpreterFinalizing() -#include "pycore_weakref.h" // _PyWeakref_IS_DEAD() #include @@ -1065,9 +1064,11 @@ static void _pysqlite_drop_unused_cursor_references(pysqlite_Connection* self) for (Py_ssize_t i = 0; i < PyList_Size(self->cursors); i++) { PyObject* weakref = PyList_GetItem(self->cursors, i); - if (_PyWeakref_IS_DEAD(weakref)) { + PyObject* obj; + if (!PyWeakref_GetRef(weakref, &obj)) { continue; } + Py_DECREF(obj); if (PyList_Append(new_list, weakref) != 0) { Py_DECREF(new_list); return; diff --git a/Modules/_ssl.c b/Modules/_ssl.c index d00f407b569fb6..9d45898aac0997 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -29,7 +29,6 @@ #include "pycore_fileutils.h" // _PyIsSelectable_fd() #include "pycore_pyerrors.h" // _PyErr_ChainExceptions1() #include "pycore_time.h" // _PyDeadline_Init() -#include "pycore_weakref.h" // _PyWeakref_GET_REF() /* Include symbols from _socket module */ #include "socketmodule.h" @@ -392,8 +391,8 @@ typedef enum { // Return a borrowed reference. static inline PySocketSockObject* GET_SOCKET(PySSLSocket *obj) { if (obj->Socket) { - PyObject *sock = _PyWeakref_GET_REF(obj->Socket); - if (sock != NULL) { + PyObject *sock; + if (PyWeakref_GetRef(obj->Socket, &sock)) { // GET_SOCKET() returns a borrowed reference Py_DECREF(sock); } @@ -2217,8 +2216,8 @@ PySSL_get_owner(PySSLSocket *self, void *c) if (self->owner == NULL) { Py_RETURN_NONE; } - PyObject *owner = _PyWeakref_GET_REF(self->owner); - if (owner == NULL) { + PyObject *owner; + if (!PyWeakref_GetRef(self->owner, &owner)) { Py_RETURN_NONE; } return owner; @@ -4446,9 +4445,9 @@ _servername_callback(SSL *s, int *al, void *args) * will be passed. If both do not exist only then the C-level object is * passed. */ if (ssl->owner) - ssl_socket = _PyWeakref_GET_REF(ssl->owner); + PyWeakref_GetRef(ssl->owner, &ssl_socket); else if (ssl->Socket) - ssl_socket = _PyWeakref_GET_REF(ssl->Socket); + PyWeakref_GetRef(ssl->Socket, &ssl_socket); else ssl_socket = Py_NewRef(ssl); diff --git a/Modules/_ssl/debughelpers.c b/Modules/_ssl/debughelpers.c index 07e9ce7a6fce2d..9c87f8b4d21e68 100644 --- a/Modules/_ssl/debughelpers.c +++ b/Modules/_ssl/debughelpers.c @@ -28,12 +28,12 @@ _PySSL_msg_callback(int write_p, int version, int content_type, PyObject *ssl_socket; /* ssl.SSLSocket or ssl.SSLObject */ if (ssl_obj->owner) - ssl_socket = _PyWeakref_GET_REF(ssl_obj->owner); + PyWeakref_GetRef(ssl_obj->owner, &ssl_socket); else if (ssl_obj->Socket) - ssl_socket = _PyWeakref_GET_REF(ssl_obj->Socket); + PyWeakref_GetRef(ssl_obj->Socket, &ssl_socket); else ssl_socket = (PyObject *)Py_NewRef(ssl_obj); - assert(ssl_socket != NULL); // _PyWeakref_GET_REF() can return NULL + assert(ssl_socket != NULL); // PyWeakref_GetRef() can return NULL /* assume that OpenSSL verifies all payload and buf len is of sufficient length */ From 5e7c8bbfe72ce27b206c1f50dae159c6d64c67ae Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 13:44:29 -0700 Subject: [PATCH 53/66] Use a pointer to the appropriate mutex rather than a copy of the object ptr --- Include/cpython/weakrefobject.h | 7 ++++--- Include/internal/pycore_weakref.h | 9 +++++++++ Objects/weakrefobject.c | 15 +++++---------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/Include/cpython/weakrefobject.h b/Include/cpython/weakrefobject.h index 626f33dc733a19..53df91469d69b2 100644 --- a/Include/cpython/weakrefobject.h +++ b/Include/cpython/weakrefobject.h @@ -32,10 +32,11 @@ struct _PyWeakReference { vectorcallfunc vectorcall; #ifdef Py_GIL_DISABLED - /* Used in free-threaded builds to provide a stable index into the striped - * lock. Always contains the original value of wr_object. + /* Pointer to the lock used when clearing in free-threaded builds. + * Normally this can be derived from wr_object, but in some cases we need + * to lock after wr_object has been set to Py_None. */ - PyObject *orig_object; + struct _PyMutex *weakrefs_lock; #endif }; diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index 217f8074198d4e..bb0d632b28749b 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -19,15 +19,24 @@ extern "C" { _PyInterpreterState_GET() \ ->weakref_locks[((uintptr_t)obj) % NUM_WEAKREF_LIST_LOCKS] +// Lock using the referenced object #define LOCK_WEAKREFS(obj) \ PyMutex_LockFlags(&WEAKREF_LIST_LOCK(obj), _Py_LOCK_DONT_DETACH) #define UNLOCK_WEAKREFS(obj) PyMutex_Unlock(&WEAKREF_LIST_LOCK(obj)) +// Lock using a weakref +#define LOCK_WEAKREFS_FOR_WR(wr) \ + PyMutex_LockFlags(wr->weakrefs_lock, _Py_LOCK_DONT_DETACH) +#define UNLOCK_WEAKREFS_FOR_WR(wr) PyMutex_Unlock(wr->weakrefs_lock) + #else #define LOCK_WEAKREFS(obj) #define UNLOCK_WEAKREFS(obj) +#define LOCK_WEAKREFS_FOR_WR(wr) +#define UNLOCK_WEAKREFS_FOR_WR(wr) + #endif static inline int _is_dead(PyObject *obj) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 754e187a2c70e5..b82eb81aa4cab7 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -68,7 +68,7 @@ init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) self->wr_callback = Py_XNewRef(callback); self->vectorcall = weakref_vectorcall; #ifdef Py_GIL_DISABLED - self->orig_object = ob; + self->weakrefs_lock = &WEAKREF_LIST_LOCK(ob); _PyObject_SetMaybeWeakref(ob); _PyObject_SetMaybeWeakref((PyObject *)self); #endif @@ -119,16 +119,11 @@ static void clear_weakref(PyWeakReference *self) { PyObject *callback = NULL; -#ifdef Py_GIL_DISABLED - // self->wr_object may be Py_None if the GC cleared the weakref, however, - // we still need the original value of wr_object so that we can lock the - // appropriate lock to clear the callback. - LOCK_WEAKREFS(self->orig_object); -#endif + // self->wr_object may be Py_None if the GC cleared the weakref, so lock + // using the pointer in the weakref. + LOCK_WEAKREFS_FOR_WR(self); clear_weakref_lock_held(self, &callback); -#ifdef Py_GIL_DISABLED - UNLOCK_WEAKREFS(self->orig_object); -#endif + UNLOCK_WEAKREFS_FOR_WR(self); Py_XDECREF(callback); } From 37c952982422e92540a6c353c01d867ea8fc3a9e Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 13:46:56 -0700 Subject: [PATCH 54/66] Fix formatting --- Include/cpython/weakrefobject.h | 2 +- Objects/weakrefobject.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Include/cpython/weakrefobject.h b/Include/cpython/weakrefobject.h index 53df91469d69b2..9a796098c6b48f 100644 --- a/Include/cpython/weakrefobject.h +++ b/Include/cpython/weakrefobject.h @@ -36,7 +36,7 @@ struct _PyWeakReference { * Normally this can be derived from wr_object, but in some cases we need * to lock after wr_object has been set to Py_None. */ - struct _PyMutex *weakrefs_lock; + struct _PyMutex *weakrefs_lock; #endif }; diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index b82eb81aa4cab7..fe7e6c2d735f52 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -334,7 +334,8 @@ insert_head(PyWeakReference *newref, PyWeakReference **list) * creating a new weakref */ static PyWeakReference * -try_reuse_basic_ref(PyWeakReference *list, PyTypeObject *type, PyObject *callback) +try_reuse_basic_ref(PyWeakReference *list, PyTypeObject *type, + PyObject *callback) { if (callback != NULL) { return NULL; @@ -1125,8 +1126,7 @@ _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj) This is never triggered for static types so we can avoid the (slightly) more costly _PyObject_GET_WEAKREFS_LISTPTR(). */ - PyWeakReference **list = \ - _PyObject_GET_WEAKREFS_LISTPTR_FROM_OFFSET(obj); + PyWeakReference **list = _PyObject_GET_WEAKREFS_LISTPTR_FROM_OFFSET(obj); LOCK_WEAKREFS(obj); while (*list) { _PyWeakref_ClearRef(*list); From fece88d7f003d9136e04ad5384108b06ca3d234d Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 29 Mar 2024 14:14:54 -0700 Subject: [PATCH 55/66] Merge implementation of try_reuse_basic_ref --- Objects/weakrefobject.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index fe7e6c2d735f52..480f4433b6a9f6 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -353,15 +353,10 @@ try_reuse_basic_ref(PyWeakReference *list, PyTypeObject *type, cand = proxy; } -#ifdef Py_GIL_DISABLED - if (cand != NULL && !_Py_TryIncref((PyObject *) cand)) { - cand = NULL; + if (cand != NULL && _Py_TryIncref((PyObject *) cand)) { + return cand; } -#else - Py_XINCREF(cand); -#endif - - return cand; + return NULL; } static int From 7a86e35f6b44213c5348e1a0bb14ec18100aea4c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 09:20:49 -0700 Subject: [PATCH 56/66] Remove redundant gc untrack --- Objects/weakrefobject.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 480f4433b6a9f6..c409f6e5e3f44e 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -698,8 +698,6 @@ static void proxy_dealloc(PyWeakReference *self) { PyObject_GC_UnTrack(self); - if (self->wr_callback != NULL) - PyObject_GC_UnTrack((PyObject *)self); clear_weakref(self); PyObject_GC_Del(self); } From f89edd185a8b96ef58e0d444261d69a5af81d559 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 09:23:56 -0700 Subject: [PATCH 57/66] Update comment --- Objects/weakrefobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index c409f6e5e3f44e..db67a27b7de857 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -14,7 +14,7 @@ * * In free-threaded builds we need to protect mutable state of: * - * - The weakref (wr_object, wr_hash) + * - The weakref (wr_object, hash, wr_callback) * - The referenced object (its head-of-list pointer) * - The linked list of weakrefs * From 5a0b976b53919d0785332cf91372596649da6f8c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 09:30:29 -0700 Subject: [PATCH 58/66] Add missing braces --- Objects/weakrefobject.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index db67a27b7de857..ba2ffcd47f124b 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -95,16 +95,19 @@ clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) // TODO: Assert locks are held or world is stopped if (self->wr_object != Py_None) { PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); - if (*list == self) + if (*list == self) { /* If 'self' is the end of the list (and thus self->wr_next == NULL) then the weakref list itself (and thus the value of *list) will end up being set to NULL. */ FT_ATOMIC_STORE_PTR(*list, self->wr_next); + } FT_ATOMIC_STORE_PTR(self->wr_object, Py_None); - if (self->wr_prev != NULL) + if (self->wr_prev != NULL) { self->wr_prev->wr_next = self->wr_next; - if (self->wr_next != NULL) + } + if (self->wr_next != NULL) { self->wr_next->wr_prev = self->wr_prev; + } self->wr_prev = NULL; self->wr_next = NULL; } From 9e67740d8f2b8c95f0dd2696dce53aff4f4c31a6 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 09:48:05 -0700 Subject: [PATCH 59/66] Use Py_ssize_t for tracking number of items --- Objects/weakrefobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index ba2ffcd47f124b..74d8a4fe874c09 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1057,7 +1057,7 @@ PyObject_ClearWeakRefs(PyObject *object) return; } - int num_items = 0; + Py_ssize_t num_items = 0; for (int done = 0; !done;) { PyObject *callback = NULL; LOCK_WEAKREFS(object); @@ -1078,7 +1078,7 @@ PyObject_ClearWeakRefs(PyObject *object) Py_XDECREF(callback); } - for (int i = 0; i < num_items; i += 2) { + for (Py_ssize_t i = 0; i < num_items; i += 2) { PyObject *callback = PyTuple_GET_ITEM(tuple, i + 1); if (callback != NULL) { PyObject *weakref = PyTuple_GET_ITEM(tuple, i); From 55a211fcd398f861ca7408d51edea68a65525fb1 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 09:52:40 -0700 Subject: [PATCH 60/66] Tighten up clearing basic refs/proxies --- Objects/weakrefobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 74d8a4fe874c09..2e26108f5be99c 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1034,14 +1034,14 @@ PyObject_ClearWeakRefs(PyObject *object) at the head of the list. */ for (int done = 0; !done;) { - PyObject *callback = NULL; LOCK_WEAKREFS(object); if (*list != NULL && is_basic_ref_or_proxy(*list)) { + PyObject *callback; clear_weakref_lock_held(*list, &callback); + assert(callback == NULL); } done = (*list == NULL) || !is_basic_ref_or_proxy(*list); UNLOCK_WEAKREFS(object); - Py_XDECREF(callback); } /* Deal with non-canonical (subtypes or refs with callbacks) references. */ From cdb9290416a6332562a9e72a0e7e770ac5c11d7e Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 10:02:28 -0700 Subject: [PATCH 61/66] Simplify _PyStaticType_ClearWeakRefs --- Objects/weakrefobject.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 2e26108f5be99c..9eeb45eafac36b 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1103,15 +1103,10 @@ _PyStaticType_ClearWeakRefs(PyInterpreterState *interp, PyTypeObject *type) { static_builtin_state *state = _PyStaticType_GetState(interp, type); PyObject **list = _PyStaticType_GET_WEAKREFS_LISTPTR(state); - for (int done = 0; !done;) { - PyObject *callback = NULL; - LOCK_WEAKREFS(type); - if (*list != NULL) { - clear_weakref_lock_held((PyWeakReference*)*list, &callback); - } - done = (*list == NULL); - UNLOCK_WEAKREFS(type); - Py_XDECREF(callback); + // Since this is called at finalization when there's only one thread left + // it's safe to do this without locking in free-threaded builds. + while (*list) { + _PyWeakref_ClearRef((PyWeakReference *)*list); } } From c93c3365aa50c53f7cc9b2ef98a3c6f33bddb2dd Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 10:20:55 -0700 Subject: [PATCH 62/66] Simplify get_or_create_weakref --- Objects/weakrefobject.c | 90 ++++++++++------------------------------- 1 file changed, 22 insertions(+), 68 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 9eeb45eafac36b..717491bd506b77 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -74,20 +74,6 @@ init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) #endif } -static PyWeakReference * -new_weakref(PyObject *ob, PyObject *callback) -{ - PyWeakReference *result; - - result = PyObject_GC_New(PyWeakReference, &_PyWeakref_RefType); - if (result) { - init_weakref(result, ob, callback); - PyObject_GC_Track(result); - } - return result; -} - - // Clear the weakref and steal its callback into `callback`, if provided. static void clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) @@ -406,12 +392,19 @@ insert_weakref(PyWeakReference *newref, PyWeakReference **list) } } -typedef PyWeakReference *(*weakref_alloc_fn)(PyTypeObject *, PyObject *, - PyObject *); +static PyWeakReference * +allocate_weakref(PyTypeObject *type, PyObject *obj, PyObject *callback) +{ + PyWeakReference *newref = (PyWeakReference *) type->tp_alloc(type, 0); + if (newref == NULL) { + return NULL; + } + init_weakref(newref, obj, callback); + return newref; +} static PyWeakReference * -get_or_create_weakref(PyTypeObject *type, weakref_alloc_fn allocate, - PyObject *obj, PyObject *callback) +get_or_create_weakref(PyTypeObject *type, PyObject *obj, PyObject *callback) { if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(obj))) { PyErr_Format(PyExc_TypeError, @@ -425,27 +418,22 @@ get_or_create_weakref(PyTypeObject *type, weakref_alloc_fn allocate, PyWeakReference **list = GET_WEAKREFS_LISTPTR(obj); if ((type == &_PyWeakref_RefType) || (type == &_PyWeakref_ProxyType) || - (type == &_PyWeakref_CallableProxyType)) { + (type == &_PyWeakref_CallableProxyType)) + { LOCK_WEAKREFS(obj); PyWeakReference *basic_ref = try_reuse_basic_ref(*list, type, callback); if (basic_ref != NULL) { UNLOCK_WEAKREFS(obj); return basic_ref; } - PyWeakReference *newref = allocate(type, obj, callback); - if (newref == NULL) { - return NULL; - } + PyWeakReference *newref = allocate_weakref(type, obj, callback); insert_weakref(newref, list); UNLOCK_WEAKREFS(obj); return newref; } else { // We may not be able to safely allocate inside the lock - PyWeakReference *newref = allocate(type, obj, callback); - if (newref == NULL) { - return NULL; - } + PyWeakReference *newref = allocate_weakref(type, obj, callback); LOCK_WEAKREFS(obj); insert_weakref(newref, list); UNLOCK_WEAKREFS(obj); @@ -460,25 +448,12 @@ parse_weakref_init_args(const char *funcname, PyObject *args, PyObject *kwargs, return PyArg_UnpackTuple(args, funcname, 1, 2, obp, callbackp); } - -static PyWeakReference * -allocate_ref_subtype(PyTypeObject *type, PyObject *obj, PyObject *callback) -{ - PyWeakReference *result = (PyWeakReference *) (type->tp_alloc)(type, 0); - if (result == NULL) { - return NULL; - } - init_weakref(result, obj, callback); - return result; -} - static PyObject * weakref___new__(PyTypeObject *type, PyObject *args, PyObject *kwargs) { PyObject *ob, *callback = NULL; if (parse_weakref_init_args("__new__", args, kwargs, &ob, &callback)) { - return (PyObject *)get_or_create_weakref(type, allocate_ref_subtype, - ob, callback); + return (PyObject *)get_or_create_weakref(type, ob, callback); } return NULL; } @@ -921,43 +896,22 @@ _PyWeakref_CallableProxyType = { proxy_iternext, /* tp_iternext */ }; -static PyWeakReference * -allocate_ref(PyTypeObject *type, PyObject *obj, PyObject *callback) -{ - return new_weakref(obj, callback); -} - PyObject * PyWeakref_NewRef(PyObject *ob, PyObject *callback) { - return (PyObject *)get_or_create_weakref(&_PyWeakref_RefType, allocate_ref, - ob, callback); -} - -static PyWeakReference * -allocate_proxy(PyTypeObject *type, PyObject *obj, PyObject *callback) -{ - PyWeakReference *result = new_weakref(obj, callback); - if (result == NULL) { - return NULL; - } - if (PyCallable_Check(obj)) { - Py_SET_TYPE(result, &_PyWeakref_CallableProxyType); - } - else { - Py_SET_TYPE(result, &_PyWeakref_ProxyType); - } - return result; + return (PyObject *)get_or_create_weakref(&_PyWeakref_RefType, ob, callback); } PyObject * PyWeakref_NewProxy(PyObject *ob, PyObject *callback) { - return (PyObject *)get_or_create_weakref(&_PyWeakref_ProxyType, - allocate_proxy, ob, callback); + PyTypeObject *type = &_PyWeakref_ProxyType; + if (PyCallable_Check(ob)) { + type = &_PyWeakref_CallableProxyType; + } + return (PyObject *)get_or_create_weakref(type, ob, callback); } - int PyWeakref_GetRef(PyObject *ref, PyObject **pobj) { From 0366f012b66ad333f783f7e1da76e3d752ca1de5 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 10:44:43 -0700 Subject: [PATCH 63/66] Add _PyWeakref_IsDead --- Include/internal/pycore_weakref.h | 2 ++ Modules/_sqlite/connection.c | 5 ++--- Objects/weakrefobject.c | 6 ++++++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/Include/internal/pycore_weakref.h b/Include/internal/pycore_weakref.h index bb0d632b28749b..e057a27340f718 100644 --- a/Include/internal/pycore_weakref.h +++ b/Include/internal/pycore_weakref.h @@ -113,6 +113,8 @@ extern void _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj); extern void _PyWeakref_ClearRef(PyWeakReference *self); +PyAPI_FUNC(int) _PyWeakref_IsDead(PyObject *weakref); + #ifdef __cplusplus } #endif diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index bd4f28ecda0176..74984ca5365743 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -38,6 +38,7 @@ #include "pycore_modsupport.h" // _PyArg_NoKeywords() #include "pycore_pyerrors.h" // _PyErr_ChainExceptions1() #include "pycore_pylifecycle.h" // _Py_IsInterpreterFinalizing() +#include "pycore_weakref.h" #include @@ -1064,11 +1065,9 @@ static void _pysqlite_drop_unused_cursor_references(pysqlite_Connection* self) for (Py_ssize_t i = 0; i < PyList_Size(self->cursors); i++) { PyObject* weakref = PyList_GetItem(self->cursors, i); - PyObject* obj; - if (!PyWeakref_GetRef(weakref, &obj)) { + if (_PyWeakref_IsDead(weakref)) { continue; } - Py_DECREF(obj); if (PyList_Append(new_list, weakref) != 0) { Py_DECREF(new_list); return; diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 717491bd506b77..5aa72713562260 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1078,3 +1078,9 @@ _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj) } UNLOCK_WEAKREFS(obj); } + +int +_PyWeakref_IsDead(PyObject *weakref) +{ + return _PyWeakref_IS_DEAD(weakref); +} From 3e135e4efd05a01855052ef505c46ff75a92cac7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 11:03:04 -0700 Subject: [PATCH 64/66] Remove TODO We cannot assert now that we clear weakrefs without holding the lock in _PyStaticType_ClearWeakRefs --- Objects/weakrefobject.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 5aa72713562260..60364060fdb22f 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -78,7 +78,6 @@ init_weakref(PyWeakReference *self, PyObject *ob, PyObject *callback) static void clear_weakref_lock_held(PyWeakReference *self, PyObject **callback) { - // TODO: Assert locks are held or world is stopped if (self->wr_object != Py_None) { PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object); if (*list == self) { @@ -1057,8 +1056,8 @@ _PyStaticType_ClearWeakRefs(PyInterpreterState *interp, PyTypeObject *type) { static_builtin_state *state = _PyStaticType_GetState(interp, type); PyObject **list = _PyStaticType_GET_WEAKREFS_LISTPTR(state); - // Since this is called at finalization when there's only one thread left - // it's safe to do this without locking in free-threaded builds. + // This is safe to do without holding the lock in free-threaded builds; + // there is only one thread running and no new threads can be created. while (*list) { _PyWeakref_ClearRef((PyWeakReference *)*list); } From dbf7c580b72b13411ba5514ce14ca85d74990871 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 2 Apr 2024 11:39:56 -0700 Subject: [PATCH 65/66] Fix formatting --- Objects/weakrefobject.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 60364060fdb22f..3d74fd55e9af3b 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -898,7 +898,8 @@ _PyWeakref_CallableProxyType = { PyObject * PyWeakref_NewRef(PyObject *ob, PyObject *callback) { - return (PyObject *)get_or_create_weakref(&_PyWeakref_RefType, ob, callback); + return (PyObject *)get_or_create_weakref(&_PyWeakref_RefType, ob, + callback); } PyObject * @@ -911,6 +912,7 @@ PyWeakref_NewProxy(PyObject *ob, PyObject *callback) return (PyObject *)get_or_create_weakref(type, ob, callback); } + int PyWeakref_GetRef(PyObject *ref, PyObject **pobj) { From 73466bf7d3577cd6de2d095d356e35bf190d22c9 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 8 Apr 2024 10:16:54 -0400 Subject: [PATCH 66/66] Update Python/pystate.c Co-authored-by: Erlend E. Aasland --- Python/pystate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Python/pystate.c b/Python/pystate.c index 3fee0d41be25aa..66c28498aad7c5 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -508,7 +508,8 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) } #ifdef Py_GIL_DISABLED for (PyInterpreterState *interp = runtime->interpreters.head; - interp != NULL; interp = interp->next) { + interp != NULL; interp = interp->next) + { for (int i = 0; i < NUM_WEAKREF_LIST_LOCKS; i++) { _PyMutex_at_fork_reinit(&interp->weakref_locks[i]); }