From c00cd862bdb2012b4b1b22ad3463e589bf814435 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Tue, 4 Feb 2025 17:02:36 -0800 Subject: [PATCH 1/3] Avoid locking when clearing objects --- Objects/dictobject.c | 80 ++++++++++++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 29 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 900d001d4dd56a..ffdf494085b0c0 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -7256,8 +7256,8 @@ decref_maybe_delay(PyObject *obj, bool delay) } } -static int -set_or_clear_managed_dict(PyObject *obj, PyObject *new_dict, bool clear) +int +_PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict) { assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT); #ifndef NDEBUG @@ -7292,8 +7292,7 @@ set_or_clear_managed_dict(PyObject *obj, PyObject *new_dict, bool clear) // Decref for the dictionary we incref'd in try_set_dict_inline_only_or_other_dict // while the object was locked - decref_maybe_delay((PyObject *)prev_dict, - !clear && prev_dict != cur_dict); + decref_maybe_delay((PyObject *)prev_dict, prev_dict != cur_dict); if (err != 0) { return err; } @@ -7303,7 +7302,7 @@ set_or_clear_managed_dict(PyObject *obj, PyObject *new_dict, bool clear) if (prev_dict != NULL) { // decref for the dictionary that we replaced - decref_maybe_delay((PyObject *)prev_dict, !clear); + decref_maybe_delay((PyObject *)prev_dict, true); } return 0; @@ -7333,45 +7332,68 @@ set_or_clear_managed_dict(PyObject *obj, PyObject *new_dict, bool clear) (PyDictObject *)Py_XNewRef(new_dict)); Py_END_CRITICAL_SECTION(); - decref_maybe_delay((PyObject *)dict, !clear); + decref_maybe_delay((PyObject *)dict, true); } assert(_PyObject_InlineValuesConsistencyCheck(obj)); return err; } -int -_PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict) -{ - return set_or_clear_managed_dict(obj, new_dict, false); -} - void PyObject_ClearManagedDict(PyObject *obj) { - if (set_or_clear_managed_dict(obj, NULL, true) < 0) { - /* Must be out of memory */ - assert(PyErr_Occurred() == PyExc_MemoryError); - PyErr_FormatUnraisable("Exception ignored while " - "clearing an object managed dict"); - /* Clear the dict */ + // This is called when the object is being freed and therefore + // has no other references or during GC when the world is stopped + // so we don't need any locking. + if (Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES) { PyDictObject *dict = _PyObject_GetManagedDict(obj); - Py_BEGIN_CRITICAL_SECTION2(dict, obj); - dict = _PyObject_ManagedDictPointer(obj)->dict; - PyInterpreterState *interp = _PyInterpreterState_GET(); - PyDictKeysObject *oldkeys = dict->ma_keys; - set_keys(dict, Py_EMPTY_KEYS); - dict->ma_values = NULL; - dictkeys_decref(interp, oldkeys, IS_DICT_SHARED(dict)); - STORE_USED(dict, 0); - set_dict_inline_values(obj, NULL); - Py_END_CRITICAL_SECTION2(); + if (dict == NULL) { + // We have no materialized dictionary and inline values + // that just need to be cleared. + PyDictValues *values = _PyObject_InlineValues(obj); + if (values->valid) { + values->valid = 0; + for (Py_ssize_t i = 0; i < values->capacity; i++) { + Py_CLEAR(values->values[i]); + } + } + // No dict to clear, we're done + return; + } else if (FT_ATOMIC_LOAD_PTR_RELAXED(dict->ma_values) == + _PyObject_InlineValues(obj)) { + // We have a materialized object which points at the inline + // values. We need to materialize the keys. Nothing can modify + // this object, but we need to lock the dictionary. + int err; + Py_BEGIN_CRITICAL_SECTION(dict); + err = _PyDict_DetachFromObject(dict, obj); + Py_END_CRITICAL_SECTION(); + + if (err) { + /* Must be out of memory */ + assert(PyErr_Occurred() == PyExc_MemoryError); + PyErr_FormatUnraisable("Exception ignored while " + "clearing an object managed dict"); + /* Clear the dict */ + Py_BEGIN_CRITICAL_SECTION2(dict, obj); + PyInterpreterState *interp = _PyInterpreterState_GET(); + PyDictKeysObject *oldkeys = dict->ma_keys; + set_keys(dict, Py_EMPTY_KEYS); + dict->ma_values = NULL; + dictkeys_decref(interp, oldkeys, IS_DICT_SHARED(dict)); + STORE_USED(dict, 0); + set_dict_inline_values(obj, NULL); + Py_END_CRITICAL_SECTION2(); + } + } + // Else we have a materialized dict which doesn't point at the inline + // values, we can just clear it. } + Py_CLEAR(_PyObject_ManagedDictPointer(obj)->dict); } int _PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj) { - ASSERT_WORLD_STOPPED_OR_OBJ_LOCKED(obj); assert(_PyObject_ManagedDictPointer(obj)->dict == mp); assert(_PyObject_InlineValuesConsistencyCheck(obj)); From 71ddb723d7eb733af76d0f3e9c07e30b73332b61 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Wed, 19 Feb 2025 13:30:48 -0800 Subject: [PATCH 2/3] Small cleanups --- Objects/dictobject.c | 107 +++++++++++++++++++++++-------------------- 1 file changed, 57 insertions(+), 50 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index ffdf494085b0c0..b32f5b0e3b5061 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -7163,6 +7163,17 @@ PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg) return 0; } +static void +clear_inline_values(PyDictValues *values) +{ + if (values->valid) { + values->valid = 0; + for (Py_ssize_t i = 0; i < values->capacity; i++) { + Py_CLEAR(values->values[i]); + } + } +} + static void set_dict_inline_values(PyObject *obj, PyDictObject *new_dict) { @@ -7173,12 +7184,7 @@ set_dict_inline_values(PyObject *obj, PyDictObject *new_dict) Py_XINCREF(new_dict); FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict, new_dict); - if (values->valid) { - FT_ATOMIC_STORE_UINT8(values->valid, 0); - for (Py_ssize_t i = 0; i < values->capacity; i++) { - Py_CLEAR(values->values[i]); - } - } + clear_inline_values(values); } #ifdef Py_GIL_DISABLED @@ -7338,55 +7344,80 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict) return err; } +static int +detach_dict_from_object(PyDictObject *mp, PyObject *obj) +{ + assert(_PyObject_ManagedDictPointer(obj)->dict == mp); + assert(_PyObject_InlineValuesConsistencyCheck(obj)); + + if (FT_ATOMIC_LOAD_PTR_RELAXED(mp->ma_values) != _PyObject_InlineValues(obj)) { + return 0; + } + + // We could be called with an unlocked dict when the caller knows the + // values are already detached, so we assert after inline values check. + ASSERT_WORLD_STOPPED_OR_OBJ_LOCKED(mp); + assert(mp->ma_values->embedded == 1); + assert(mp->ma_values->valid == 1); + assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES); + + PyDictValues *values = copy_values(mp->ma_values); + + if (values == NULL) { + PyErr_NoMemory(); + return -1; + } + mp->ma_values = values; + + invalidate_and_clear_inline_values(_PyObject_InlineValues(obj)); + + assert(_PyObject_InlineValuesConsistencyCheck(obj)); + ASSERT_CONSISTENT(mp); + return 0; +} + + void PyObject_ClearManagedDict(PyObject *obj) { - // This is called when the object is being freed and therefore - // has no other references or during GC when the world is stopped - // so we don't need any locking. + // This is called when the object is being freed or cleared + // by the GC and therefore known to have no references. if (Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES) { PyDictObject *dict = _PyObject_GetManagedDict(obj); if (dict == NULL) { // We have no materialized dictionary and inline values // that just need to be cleared. - PyDictValues *values = _PyObject_InlineValues(obj); - if (values->valid) { - values->valid = 0; - for (Py_ssize_t i = 0; i < values->capacity; i++) { - Py_CLEAR(values->values[i]); - } - } // No dict to clear, we're done + clear_inline_values(_PyObject_InlineValues(obj)); return; - } else if (FT_ATOMIC_LOAD_PTR_RELAXED(dict->ma_values) == + } + else if (FT_ATOMIC_LOAD_PTR_RELAXED(dict->ma_values) == _PyObject_InlineValues(obj)) { // We have a materialized object which points at the inline // values. We need to materialize the keys. Nothing can modify // this object, but we need to lock the dictionary. int err; Py_BEGIN_CRITICAL_SECTION(dict); - err = _PyDict_DetachFromObject(dict, obj); + err = detach_dict_from_object(dict, obj); Py_END_CRITICAL_SECTION(); if (err) { /* Must be out of memory */ assert(PyErr_Occurred() == PyExc_MemoryError); PyErr_FormatUnraisable("Exception ignored while " - "clearing an object managed dict"); + "clearing an object managed dict"); /* Clear the dict */ - Py_BEGIN_CRITICAL_SECTION2(dict, obj); + Py_BEGIN_CRITICAL_SECTION(dict); PyInterpreterState *interp = _PyInterpreterState_GET(); PyDictKeysObject *oldkeys = dict->ma_keys; set_keys(dict, Py_EMPTY_KEYS); dict->ma_values = NULL; dictkeys_decref(interp, oldkeys, IS_DICT_SHARED(dict)); STORE_USED(dict, 0); - set_dict_inline_values(obj, NULL); - Py_END_CRITICAL_SECTION2(); + clear_inline_values(_PyObject_InlineValues(obj)); + Py_END_CRITICAL_SECTION(); } } - // Else we have a materialized dict which doesn't point at the inline - // values, we can just clear it. } Py_CLEAR(_PyObject_ManagedDictPointer(obj)->dict); } @@ -7394,33 +7425,9 @@ PyObject_ClearManagedDict(PyObject *obj) int _PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj) { - assert(_PyObject_ManagedDictPointer(obj)->dict == mp); - assert(_PyObject_InlineValuesConsistencyCheck(obj)); - - if (FT_ATOMIC_LOAD_PTR_RELAXED(mp->ma_values) != _PyObject_InlineValues(obj)) { - return 0; - } - - // We could be called with an unlocked dict when the caller knows the - // values are already detached, so we assert after inline values check. - ASSERT_WORLD_STOPPED_OR_OBJ_LOCKED(mp); - assert(mp->ma_values->embedded == 1); - assert(mp->ma_values->valid == 1); - assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - - PyDictValues *values = copy_values(mp->ma_values); - - if (values == NULL) { - PyErr_NoMemory(); - return -1; - } - mp->ma_values = values; - - invalidate_and_clear_inline_values(_PyObject_InlineValues(obj)); + ASSERT_WORLD_STOPPED_OR_OBJ_LOCKED(obj); - assert(_PyObject_InlineValuesConsistencyCheck(obj)); - ASSERT_CONSISTENT(mp); - return 0; + return detach_dict_from_object(mp, obj); } static inline PyObject * From 427cfac110e22ee023801299cdb75a1fc48c6027 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Thu, 20 Feb 2025 11:09:38 -0800 Subject: [PATCH 3/3] set valid atomically --- Objects/dictobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index b32f5b0e3b5061..e30d626439e7f8 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -7167,7 +7167,7 @@ static void clear_inline_values(PyDictValues *values) { if (values->valid) { - values->valid = 0; + FT_ATOMIC_STORE_UINT8(values->valid, 0); for (Py_ssize_t i = 0; i < values->capacity; i++) { Py_CLEAR(values->values[i]); }