From 048daa74a661eb37883af8d5bbd5c22c5e8ce52d Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Tue, 2 Apr 2024 11:29:09 -0700 Subject: [PATCH 1/8] Make instance attributes stored in inline "dict" thread safe --- Include/cpython/object.h | 1 + Include/internal/pycore_dict.h | 19 +- Include/internal/pycore_object.h | 16 +- .../internal/pycore_pyatomic_ft_wrappers.h | 9 + Objects/dictobject.c | 305 ++++++++++++++---- Objects/object.c | 49 ++- Objects/typeobject.c | 22 +- Python/bytecodes.c | 15 +- Python/executor_cases.c.h | 10 +- Python/generated_cases.c.h | 13 +- Python/specialize.c | 3 +- Tools/cases_generator/analyzer.py | 1 + 12 files changed, 335 insertions(+), 128 deletions(-) diff --git a/Include/cpython/object.h b/Include/cpython/object.h index 2797051281f3b4..a8f57827a964cd 100644 --- a/Include/cpython/object.h +++ b/Include/cpython/object.h @@ -493,6 +493,7 @@ do { \ PyAPI_FUNC(void *) PyObject_GetItemData(PyObject *obj); PyAPI_FUNC(int) PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg); +PyAPI_FUNC(void) _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict); PyAPI_FUNC(void) PyObject_ClearManagedDict(PyObject *obj); #define TYPE_MAX_WATCHERS 8 diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index fba0dfc40714ec..f33026dbd6be58 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -1,4 +1,3 @@ - #ifndef Py_INTERNAL_DICT_H #define Py_INTERNAL_DICT_H #ifdef __cplusplus @@ -9,9 +8,10 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif -#include "pycore_freelist.h" // _PyFreeListState -#include "pycore_identifier.h" // _Py_Identifier -#include "pycore_object.h" // PyManagedDictPointer +#include "pycore_freelist.h" // _PyFreeListState +#include "pycore_identifier.h" // _Py_Identifier +#include "pycore_object.h" // PyManagedDictPointer +#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_SSIZE_ACQUIRE // Unsafe flavor of PyDict_GetItemWithError(): no error checking extern PyObject* _PyDict_GetItemWithError(PyObject *dp, PyObject *key); @@ -249,7 +249,7 @@ _PyDict_NotifyEvent(PyInterpreterState *interp, return DICT_NEXT_VERSION(interp) | (mp->ma_version_tag & DICT_WATCHER_AND_MODIFICATION_MASK); } -extern PyDictObject *_PyObject_MakeDictFromInstanceAttributes(PyObject *obj); +extern PyDictObject *_PyObject_MaterializeManagedDict(PyObject *obj); PyAPI_FUNC(PyObject *)_PyDict_FromItems( PyObject *const *keys, Py_ssize_t keys_offset, @@ -277,7 +277,6 @@ _PyDictValues_AddToInsertionOrder(PyDictValues *values, Py_ssize_t ix) static inline size_t shared_keys_usable_size(PyDictKeysObject *keys) { -#ifdef Py_GIL_DISABLED // dk_usable will decrease for each instance that is created and each // value that is added. dk_nentries will increase for each value that // is added. We want to always return the right value or larger. @@ -285,11 +284,9 @@ shared_keys_usable_size(PyDictKeysObject *keys) // second, and conversely here we read dk_usable first and dk_entries // second (to avoid the case where we read entries before the increment // and read usable after the decrement) - return (size_t)(_Py_atomic_load_ssize_acquire(&keys->dk_usable) + - _Py_atomic_load_ssize_acquire(&keys->dk_nentries)); -#else - return (size_t)keys->dk_nentries + (size_t)keys->dk_usable; -#endif + Py_ssize_t dk_usable = FT_ATOMIC_LOAD_SSIZE_ACQUIRE(keys->dk_usable); + Py_ssize_t dk_nentries = FT_ATOMIC_LOAD_SSIZE_ACQUIRE(keys->dk_nentries); + return dk_nentries + dk_usable; } static inline size_t diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 512f7a35f50e38..8ae83879562e9a 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -12,6 +12,7 @@ extern "C" { #include "pycore_gc.h" // _PyObject_GC_IS_TRACKED() #include "pycore_emscripten_trampoline.h" // _PyCFunction_TrampolineCall() #include "pycore_interp.h" // PyInterpreterState.gc +#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_STORE_PTR_RELAXED #include "pycore_pystate.h" // _PyInterpreterState_GET() /* Check if an object is consistent. For example, ensure that the reference @@ -659,10 +660,10 @@ extern PyObject* _PyType_GetDocFromInternalDoc(const char *, const char *); extern PyObject* _PyType_GetTextSignatureFromInternalDoc(const char *, const char *, int); void _PyObject_InitInlineValues(PyObject *obj, PyTypeObject *tp); -extern int _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values, - PyObject *name, PyObject *value); -PyObject * _PyObject_GetInstanceAttribute(PyObject *obj, PyDictValues *values, - PyObject *name); +extern int _PyObject_TryStoreInstanceAttribute(PyObject *obj, + PyObject *name, PyObject *value); +extern int _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, + PyObject **attr); #ifdef Py_GIL_DISABLED # define MANAGED_DICT_OFFSET (((Py_ssize_t)sizeof(PyObject *))*-1) @@ -683,6 +684,13 @@ _PyObject_ManagedDictPointer(PyObject *obj) return (PyManagedDictPointer *)((char *)obj + MANAGED_DICT_OFFSET); } +static inline PyDictObject * +_PyObject_GetManagedDict(PyObject *obj) +{ + PyManagedDictPointer *dorv = _PyObject_ManagedDictPointer(obj); + return (PyDictObject *)FT_ATOMIC_LOAD_PTR_RELAXED(dorv->dict); +} + static inline PyDictValues * _PyObject_InlineValues(PyObject *obj) { diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index fed5d6e0ec2c54..3adb9ae99f581e 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -22,6 +22,8 @@ extern "C" { #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_ACQUIRE(value) \ + _Py_atomic_load_ssize_acquire(&value) #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) \ _Py_atomic_load_ssize_relaxed(&value) #define FT_ATOMIC_STORE_PTR(value, new_value) \ @@ -30,6 +32,10 @@ extern "C" { _Py_atomic_load_ptr_acquire(&value) #define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) \ _Py_atomic_load_uintptr_acquire(&value) +#define FT_ATOMIC_LOAD_PTR_RELAXED(value) \ + _Py_atomic_load_ptr_relaxed(&value) +#define FT_ATOMIC_LOAD_UINT8_RELAXED(value) \ + _Py_atomic_load_uint8_relaxed(&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) \ @@ -44,10 +50,13 @@ extern "C" { #else #define FT_ATOMIC_LOAD_PTR(value) value #define FT_ATOMIC_LOAD_SSIZE(value) value +#define FT_ATOMIC_LOAD_SSIZE_ACQUIRE(value) value #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) value #define FT_ATOMIC_STORE_PTR(value, new_value) value = new_value #define FT_ATOMIC_LOAD_PTR_ACQUIRE(value) value #define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) value +#define FT_ATOMIC_LOAD_PTR_RELAXED(value) value +#define FT_ATOMIC_LOAD_UINT8_RELAXED(value) 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_UINTPTR_RELEASE(value, new_value) value = new_value diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 4e696419eb5eb0..62542e7b9501a9 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1752,7 +1752,7 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp, uint64_t new_version = _PyDict_NotifyEvent( interp, PyDict_EVENT_MODIFIED, mp, key, value); if (_PyDict_HasSplitTable(mp)) { - mp->ma_values->values[ix] = value; + STORE_SPLIT_VALUE(mp, ix, value); if (old_value == NULL) { _PyDictValues_AddToInsertionOrder(mp->ma_values, ix); mp->ma_used++; @@ -2514,7 +2514,7 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix, mp->ma_version_tag = new_version; if (_PyDict_HasSplitTable(mp)) { assert(old_value == mp->ma_values->values[ix]); - mp->ma_values->values[ix] = NULL; + STORE_SPLIT_VALUE(mp, ix, NULL); assert(ix < SHARED_KEYS_MAX_SIZE); /* Update order */ delete_index_from_values(mp->ma_values, ix); @@ -4226,7 +4226,7 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu assert(_PyDict_HasSplitTable(mp)); assert(mp->ma_values->values[ix] == NULL); MAINTAIN_TRACKING(mp, key, value); - mp->ma_values->values[ix] = Py_NewRef(value); + STORE_SPLIT_VALUE(mp, ix, Py_NewRef(value)); _PyDictValues_AddToInsertionOrder(mp->ma_values, ix); mp->ma_used++; mp->ma_version_tag = new_version; @@ -6617,27 +6617,54 @@ make_dict_from_instance_attributes(PyInterpreterState *interp, } PyDictObject * -_PyObject_MakeDictFromInstanceAttributes(PyObject *obj) +_PyObject_MaterializeManagedDict(PyObject *obj) { + PyDictObject *dict = _PyObject_GetManagedDict(obj); + if (dict != NULL) { + return dict; + } + + Py_BEGIN_CRITICAL_SECTION(obj); + +#ifdef Py_GIL_DISABLED + dict = _PyObject_GetManagedDict(obj); + if (dict != NULL) { + // We raced with another thread creating the dict + goto exit; + } +#endif + + OBJECT_STAT_INC(dict_materialized_on_request); + PyDictValues *values = _PyObject_InlineValues(obj); PyInterpreterState *interp = _PyInterpreterState_GET(); PyDictKeysObject *keys = CACHED_KEYS(Py_TYPE(obj)); OBJECT_STAT_INC(dict_materialized_on_request); - return make_dict_from_instance_attributes(interp, keys, values); + dict = make_dict_from_instance_attributes(interp, keys, values); + + FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict, + (PyDictObject *)dict); + +#ifdef Py_GIL_DISABLED +exit: +#endif + Py_END_CRITICAL_SECTION(); + return dict; } -int -_PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values, +static int +store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, PyObject *name, PyObject *value) { - PyInterpreterState *interp = _PyInterpreterState_GET(); PyDictKeysObject *keys = CACHED_KEYS(Py_TYPE(obj)); assert(keys != NULL); assert(values != NULL); assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES); Py_ssize_t ix = DKIX_EMPTY; + PyDictObject *dict = _PyObject_GetManagedDict(obj); + assert(dict == NULL || ((PyDictObject *)dict)->ma_values == values); if (PyUnicode_CheckExact(name)) { Py_hash_t hash = unicode_get_hash(name); if (hash == -1) { @@ -6674,25 +6701,21 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values, } #endif } - PyDictObject *dict = _PyObject_ManagedDictPointer(obj)->dict; + if (ix == DKIX_EMPTY) { if (dict == NULL) { - dict = make_dict_from_instance_attributes( - interp, keys, values); + dict = _PyObject_MaterializeManagedDict(obj); if (dict == NULL) { return -1; } - _PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)dict; - } - if (value == NULL) { - return PyDict_DelItem((PyObject *)dict, name); - } - else { - return PyDict_SetItem((PyObject *)dict, name, value); } + // Caller needs to insert into materialized dict + return 1; } + PyObject *old_value = values->values[ix]; - values->values[ix] = Py_XNewRef(value); + FT_ATOMIC_STORE_PTR_RELEASE(values->values[ix], Py_XNewRef(value)); + if (old_value == NULL) { if (value == NULL) { PyErr_Format(PyExc_AttributeError, @@ -6719,6 +6742,63 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values, return 0; } +int +_PyObject_TryStoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value) +{ + PyDictValues *values = _PyObject_InlineValues(obj); + if (!FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { + return 1; + } + +#ifdef Py_GIL_DISABLED + int res; + // We have a valid inline values, at least for now... There are two potential + // races with having the values become invalid. One is the dictionary + // being detached from the object. The other is if someone is inserting + // into the dictionary directly and therefore causing it to resize. + // + // If we haven't materialized the dictionary yet we lock on the object, which + // will also be used to prevent the dictionary from being materialized while + // we're doing the insertion. If we race and the dictionary gets created + // then we'll need to release the object lock and lock the dictionary to + // prevent resizing. + PyDictObject *dict = _PyObject_GetManagedDict(obj); + if (dict == NULL) { + int retry_with_dict = 0; + Py_BEGIN_CRITICAL_SECTION(obj); + dict = _PyObject_GetManagedDict(obj); + + if (dict == NULL) { + res = store_instance_attr_lock_held(obj, values, name, value); + } + else { + // We lost a race with the materialization of the dict, we'll + // try the insert with it... + retry_with_dict = 1; + } + Py_END_CRITICAL_SECTION(); + if (retry_with_dict) { + goto with_dict; + } + } + else { +with_dict: + Py_BEGIN_CRITICAL_SECTION(dict); + if (dict->ma_values == values) { + res = store_instance_attr_lock_held(obj, values, name, value); + } + else { + // Caller needs to insert into dict + res = 1; + } + Py_END_CRITICAL_SECTION(); + } + return res; +#else + return store_instance_attr_lock_held(obj, values, name, value); +#endif +} + /* Sanity check for managed dicts */ #if 0 #define CHECK(val) assert(val); if (!(val)) { return 0; } @@ -6750,19 +6830,80 @@ _PyObject_ManagedDictValidityCheck(PyObject *obj) } #endif -PyObject * -_PyObject_GetInstanceAttribute(PyObject *obj, PyDictValues *values, - PyObject *name) +// Attempts to get an instance attribute from the inline values. Returns 0 if +// the lookup from the inline values was successful or 1 if the inline values +// are no longer valid. No error is set in either case. +int +_PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr) { assert(PyUnicode_CheckExact(name)); + PyDictValues *values = _PyObject_InlineValues(obj); + if (!FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { + return 1; + } + PyDictKeysObject *keys = CACHED_KEYS(Py_TYPE(obj)); assert(keys != NULL); Py_ssize_t ix = _PyDictKeys_StringLookup(keys, name); if (ix == DKIX_EMPTY) { - return NULL; + *attr = NULL; + return 0; + } + +#ifdef Py_GIL_DISABLED + PyObject *value = _Py_atomic_load_ptr_relaxed(&values->values[ix]); + if (value == NULL || _Py_TryIncrefCompare(&values->values[ix], value)) { + *attr = value; + return 0; + } + + PyDictObject *dict = _PyObject_GetManagedDict(obj); + if (dict == NULL) { + // No dict, lock the object to prevent one from being + // materialized... + bool success = false; + Py_BEGIN_CRITICAL_SECTION(obj); + + dict = _PyObject_GetManagedDict(obj); + if (dict == NULL) { + // Still no dict, we can read from the values + assert(values->valid); + value = _Py_atomic_load_ptr_relaxed(&values->values[ix]); + *attr = Py_XNewRef(value); + success = true; + } + + Py_END_CRITICAL_SECTION(); + + if (success) { + return 0; + } + } + + // We have a dictionary, we'll need to lock it to prevent + // the values from being resized. + assert(dict != NULL); + int res; + Py_BEGIN_CRITICAL_SECTION(dict); + + if (dict->ma_values == values && + FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { + value = _Py_atomic_load_ptr_relaxed(&values->values[ix]); + *attr = Py_XNewRef(value); + res = 0; + } else { + // Caller needs to lookup from the dictionary + res = 1; } + + Py_END_CRITICAL_SECTION(); + + return res; +#else PyObject *value = values->values[ix]; - return Py_XNewRef(value); + *attr = Py_XNewRef(value); + return 0; +#endif } int @@ -6775,20 +6916,19 @@ _PyObject_IsInstanceDictEmpty(PyObject *obj) PyDictObject *dict; if (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) { PyDictValues *values = _PyObject_InlineValues(obj); - if (values->valid) { + if (FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { PyDictKeysObject *keys = CACHED_KEYS(tp); for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) { - if (values->values[i] != NULL) { + if (FT_ATOMIC_LOAD_PTR_RELAXED(values->values[i]) != NULL) { return 0; } } return 1; } - dict = _PyObject_ManagedDictPointer(obj)->dict; + dict = _PyObject_GetManagedDict(obj); } else if (tp->tp_flags & Py_TPFLAGS_MANAGED_DICT) { - PyManagedDictPointer* managed_dict = _PyObject_ManagedDictPointer(obj); - dict = managed_dict->dict; + dict = _PyObject_GetManagedDict(obj); } else { PyObject **dictptr = _PyObject_ComputedDictPointer(obj); @@ -6820,8 +6960,33 @@ PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg) return 0; } +static bool +set_dict_inline_values(PyObject *obj, PyObject *new_dict) +{ + PyDictValues *values = _PyObject_InlineValues(obj); + + if (values->valid) { + for (Py_ssize_t i = 0; i < values->capacity; i++) { + Py_CLEAR(values->values[i]); + } + values->valid = 0; + } + +#ifdef Py_GIL_DISABLED + PyDictObject *dict = _PyObject_ManagedDictPointer(obj)->dict; + if (dict != NULL) { + // We lost a race with materialization of the dict + return false; + } +#endif + + Py_XINCREF(new_dict); + _PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)new_dict; + return true; +} + void -PyObject_ClearManagedDict(PyObject *obj) +_PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict) { assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT); assert(_PyObject_InlineValuesConsistencyCheck(obj)); @@ -6829,29 +6994,61 @@ PyObject_ClearManagedDict(PyObject *obj) if (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) { PyDictObject *dict = _PyObject_ManagedDictPointer(obj)->dict; if (dict) { +#ifdef Py_GIL_DISABLED +clear_dict: +#endif + Py_BEGIN_CRITICAL_SECTION2(dict, obj); + _PyDict_DetachFromObject(dict, obj); - _PyObject_ManagedDictPointer(obj)->dict = NULL; + + Py_XINCREF(new_dict); + _PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)new_dict; + + Py_END_CRITICAL_SECTION2(); + Py_DECREF(dict); } else { - PyDictValues *values = _PyObject_InlineValues(obj); - if (values->valid) { - for (Py_ssize_t i = 0; i < values->capacity; i++) { - Py_CLEAR(values->values[i]); - } - values->valid = 0; + bool success; + Py_BEGIN_CRITICAL_SECTION(obj); + + success = set_dict_inline_values(obj, new_dict); + + Py_END_CRITICAL_SECTION(); + + (void)success; // suppress warning when GIL isn't disabled + +#ifdef Py_GIL_DISABLED + if (!success) { + dict = _PyObject_ManagedDictPointer(obj)->dict; + assert(dict != NULL); + goto clear_dict; } +#endif } } else { - Py_CLEAR(_PyObject_ManagedDictPointer(obj)->dict); + Py_BEGIN_CRITICAL_SECTION(obj); + + Py_XSETREF(_PyObject_ManagedDictPointer(obj)->dict, + (PyDictObject *)Py_XNewRef(new_dict)); + + Py_END_CRITICAL_SECTION(); } assert(_PyObject_InlineValuesConsistencyCheck(obj)); } +void +PyObject_ClearManagedDict(PyObject *obj) +{ + _PyObject_SetManagedDict(obj, NULL); +} + int _PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(mp); + assert(_PyObject_ManagedDictPointer(obj)->dict == mp); assert(_PyObject_InlineValuesConsistencyCheck(obj)); if (mp->ma_values == NULL || mp->ma_values != _PyObject_InlineValues(obj)) { @@ -6867,6 +7064,7 @@ _PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj) if (mp->ma_values == NULL) { return -1; } + assert(_PyObject_InlineValuesConsistencyCheck(obj)); ASSERT_CONSISTENT(mp); return 0; @@ -6877,29 +7075,28 @@ PyObject_GenericGetDict(PyObject *obj, void *context) { PyInterpreterState *interp = _PyInterpreterState_GET(); PyTypeObject *tp = Py_TYPE(obj); + PyDictObject *dict; if (_PyType_HasFeature(tp, Py_TPFLAGS_MANAGED_DICT)) { - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(obj); - PyDictObject *dict = managed_dict->dict; + dict = _PyObject_GetManagedDict(obj); if (dict == NULL && (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && - _PyObject_InlineValues(obj)->valid - ) { - PyDictValues *values = _PyObject_InlineValues(obj); - OBJECT_STAT_INC(dict_materialized_on_request); - dict = make_dict_from_instance_attributes( - interp, CACHED_KEYS(tp), values); - if (dict != NULL) { - managed_dict->dict = (PyDictObject *)dict; - } + _PyObject_InlineValues(obj)->valid) { + dict = _PyObject_MaterializeManagedDict(obj); } - else { - dict = managed_dict->dict; + else if (dict == NULL) { + Py_BEGIN_CRITICAL_SECTION(obj); + + // Check again that we're not racing with someone else creating the dict + dict = _PyObject_GetManagedDict(obj); if (dict == NULL) { - dictkeys_incref(CACHED_KEYS(tp)); OBJECT_STAT_INC(dict_materialized_on_request); + dictkeys_incref(CACHED_KEYS(tp)); dict = (PyDictObject *)new_dict_with_shared_keys(interp, CACHED_KEYS(tp)); - managed_dict->dict = (PyDictObject *)dict; + FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict, + (PyDictObject *)dict); } + + Py_END_CRITICAL_SECTION(); } return Py_XNewRef((PyObject *)dict); } @@ -7109,7 +7306,7 @@ _PyObject_InlineValuesConsistencyCheck(PyObject *obj) return 1; } assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT); - PyDictObject *dict = (PyDictObject *)_PyObject_ManagedDictPointer(obj)->dict; + PyDictObject *dict = _PyObject_GetManagedDict(obj); if (dict == NULL) { return 1; } diff --git a/Objects/object.c b/Objects/object.c index 73a1927263cdcb..b0f07b02f38849 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -6,6 +6,7 @@ #include "pycore_call.h" // _PyObject_CallNoArgs() #include "pycore_ceval.h" // _Py_EnterRecursiveCallTstate() #include "pycore_context.h" // _PyContextTokenMissing_Type +#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION, Py_END_CRITICAL_SECTION #include "pycore_descrobject.h" // _PyMethodWrapper_Type #include "pycore_dict.h" // _PyObject_MakeDictFromInstanceAttributes() #include "pycore_floatobject.h" // _PyFloat_DebugMallocStats() @@ -25,6 +26,7 @@ #include "pycore_typevarobject.h" // _PyTypeAlias_Type, _Py_initialize_generic #include "pycore_unionobject.h" // _PyUnion_Type + #ifdef Py_LIMITED_API // Prevent recursive call _Py_IncRef() <=> Py_INCREF() # error "Py_LIMITED_API macro must not be defined" @@ -1403,16 +1405,15 @@ _PyObject_GetDictPtr(PyObject *obj) if ((Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT) == 0) { return _PyObject_ComputedDictPointer(obj); } - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(obj); - if (managed_dict->dict == NULL && Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES) { - PyDictObject *dict = (PyDictObject *)_PyObject_MakeDictFromInstanceAttributes(obj); + PyDictObject *dict = _PyObject_GetManagedDict(obj); + if (dict == NULL && Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES) { + dict = _PyObject_MaterializeManagedDict(obj); if (dict == NULL) { PyErr_Clear(); return NULL; } - managed_dict->dict = dict; } - return (PyObject **)&managed_dict->dict; + return (PyObject **)&_PyObject_ManagedDictPointer(obj)->dict; } PyObject * @@ -1480,10 +1481,9 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method) } } } - PyObject *dict; - if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && _PyObject_InlineValues(obj)->valid) { - PyDictValues *values = _PyObject_InlineValues(obj); - PyObject *attr = _PyObject_GetInstanceAttribute(obj, values, name); + PyObject *dict, *attr; + if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && + !_PyObject_TryGetInstanceAttribute(obj, name, &attr)) { if (attr != NULL) { *method = attr; Py_XDECREF(descr); @@ -1492,8 +1492,7 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method) dict = NULL; } else if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) { - PyManagedDictPointer* managed_dict = _PyObject_ManagedDictPointer(obj); - dict = (PyObject *)managed_dict->dict; + dict = (PyObject *)_PyObject_GetManagedDict(obj); } else { PyObject **dictptr = _PyObject_ComputedDictPointer(obj); @@ -1586,26 +1585,23 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name, } } if (dict == NULL) { - if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && _PyObject_InlineValues(obj)->valid) { - PyDictValues *values = _PyObject_InlineValues(obj); - if (PyUnicode_CheckExact(name)) { - res = _PyObject_GetInstanceAttribute(obj, values, name); + if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES)) { + if (PyUnicode_CheckExact(name) && + !_PyObject_TryGetInstanceAttribute(obj, name, &res)) { if (res != NULL) { goto done; } } else { - dict = (PyObject *)_PyObject_MakeDictFromInstanceAttributes(obj); + dict = (PyObject *)_PyObject_MaterializeManagedDict(obj); if (dict == NULL) { res = NULL; goto done; } - _PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)dict; } } else if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) { - PyManagedDictPointer* managed_dict = _PyObject_ManagedDictPointer(obj); - dict = (PyObject *)managed_dict->dict; + dict = (PyObject *)_PyObject_GetManagedDict(obj); } else { PyObject **dictptr = _PyObject_ComputedDictPointer(obj); @@ -1700,12 +1696,15 @@ _PyObject_GenericSetAttrWithDict(PyObject *obj, PyObject *name, if (dict == NULL) { PyObject **dictptr; - if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && _PyObject_InlineValues(obj)->valid) { - res = _PyObject_StoreInstanceAttribute( - obj, _PyObject_InlineValues(obj), name, value); - goto error_check; + + if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES)) { + res = _PyObject_TryStoreInstanceAttribute(obj, name, value); + if (res <= 0) { + goto error_check; + } } - else if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) { + + if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) { PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(obj); dictptr = (PyObject **)&managed_dict->dict; } @@ -1779,7 +1778,7 @@ PyObject_GenericSetDict(PyObject *obj, PyObject *value, void *context) PyObject **dictptr = _PyObject_GetDictPtr(obj); if (dictptr == NULL) { if (_PyType_HasFeature(Py_TYPE(obj), Py_TPFLAGS_INLINE_VALUES) && - _PyObject_ManagedDictPointer(obj)->dict == NULL + _PyObject_GetManagedDict(obj) == NULL ) { /* Was unable to convert to dict */ PyErr_NoMemory(); diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 2f356388785665..fc82b6ae6dfc4d 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -3165,9 +3165,9 @@ subtype_setdict(PyObject *obj, PyObject *value, void *context) "not a '%.200s'", Py_TYPE(value)->tp_name); return -1; } + if (Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT) { - PyObject_ClearManagedDict(obj); - _PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)Py_XNewRef(value); + _PyObject_SetManagedDict(obj, value); } else { dictptr = _PyObject_ComputedDictPointer(obj); @@ -6194,15 +6194,17 @@ object_set_class(PyObject *self, PyObject *value, void *closure) /* Changing the class will change the implicit dict keys, * so we must materialize the dictionary first. */ if (oldto->tp_flags & Py_TPFLAGS_INLINE_VALUES) { - PyDictObject *dict = _PyObject_ManagedDictPointer(self)->dict; - if (dict == NULL) { - dict = (PyDictObject *)_PyObject_MakeDictFromInstanceAttributes(self); - if (dict == NULL) { - return -1; - } - _PyObject_ManagedDictPointer(self)->dict = dict; + PyDictObject *dict = _PyObject_MaterializeManagedDict(self); + bool error = false; + + Py_BEGIN_CRITICAL_SECTION2(self, dict); + + if (dict == NULL || _PyDict_DetachFromObject(dict, self)) { + error = true; } - if (_PyDict_DetachFromObject(dict, self)) { + + Py_END_CRITICAL_SECTION2(); + if (error) { return -1; } } diff --git a/Python/bytecodes.c b/Python/bytecodes.c index c1fbd3c7d26e01..b7511b9107fdf6 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1947,15 +1947,13 @@ dummy_func( op(_CHECK_ATTR_WITH_HINT, (owner -- owner)) { assert(Py_TYPE(owner)->tp_flags & Py_TPFLAGS_MANAGED_DICT); - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(owner); - PyDictObject *dict = managed_dict->dict; + PyDictObject *dict = _PyObject_GetManagedDict(owner); DEOPT_IF(dict == NULL); assert(PyDict_CheckExact((PyObject *)dict)); } op(_LOAD_ATTR_WITH_HINT, (hint/1, owner -- attr, null if (oparg & 1))) { - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(owner); - PyDictObject *dict = managed_dict->dict; + PyDictObject *dict = _PyObject_GetManagedDict(owner); DEOPT_IF(hint >= (size_t)dict->ma_keys->dk_nentries); PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); if (DK_IS_UNICODE(dict->ma_keys)) { @@ -2072,14 +2070,15 @@ dummy_func( op(_GUARD_DORV_NO_DICT, (owner -- owner)) { assert(Py_TYPE(owner)->tp_dictoffset < 0); assert(Py_TYPE(owner)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - DEOPT_IF(_PyObject_ManagedDictPointer(owner)->dict); + DEOPT_IF(_PyObject_GetManagedDict(owner)); DEOPT_IF(_PyObject_InlineValues(owner)->valid == 0); } op(_STORE_ATTR_INSTANCE_VALUE, (index/1, value, owner --)) { STAT_INC(STORE_ATTR, hit); - assert(_PyObject_ManagedDictPointer(owner)->dict == NULL); + assert(_PyObject_GetManagedDict(owner) == NULL); PyDictValues *values = _PyObject_InlineValues(owner); + PyObject *old_value = values->values[index]; values->values[index] = value; if (old_value == NULL) { @@ -2088,6 +2087,7 @@ dummy_func( else { Py_DECREF(old_value); } + Py_DECREF(owner); } @@ -2102,8 +2102,7 @@ dummy_func( assert(type_version != 0); DEOPT_IF(tp->tp_version_tag != type_version); assert(tp->tp_flags & Py_TPFLAGS_MANAGED_DICT); - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(owner); - PyDictObject *dict = managed_dict->dict; + PyDictObject *dict = _PyObject_GetManagedDict(owner); DEOPT_IF(dict == NULL); assert(PyDict_CheckExact((PyObject *)dict)); PyObject *name = GETITEM(FRAME_CO_NAMES, oparg); diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index df87f9178f17cf..841ce8cbedb3fb 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -1998,8 +1998,7 @@ PyObject *owner; owner = stack_pointer[-1]; assert(Py_TYPE(owner)->tp_flags & Py_TPFLAGS_MANAGED_DICT); - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(owner); - PyDictObject *dict = managed_dict->dict; + PyDictObject *dict = _PyObject_GetManagedDict(owner); if (dict == NULL) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); @@ -2015,8 +2014,7 @@ oparg = CURRENT_OPARG(); owner = stack_pointer[-1]; uint16_t hint = (uint16_t)CURRENT_OPERAND(); - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(owner); - PyDictObject *dict = managed_dict->dict; + PyDictObject *dict = _PyObject_GetManagedDict(owner); if (hint >= (size_t)dict->ma_keys->dk_nentries) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); @@ -2159,7 +2157,7 @@ owner = stack_pointer[-1]; assert(Py_TYPE(owner)->tp_dictoffset < 0); assert(Py_TYPE(owner)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - if (_PyObject_ManagedDictPointer(owner)->dict) { + if (_PyObject_GetManagedDict(owner)) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } @@ -2177,7 +2175,7 @@ value = stack_pointer[-2]; uint16_t index = (uint16_t)CURRENT_OPERAND(); STAT_INC(STORE_ATTR, hit); - assert(_PyObject_ManagedDictPointer(owner)->dict == NULL); + assert(_PyObject_GetManagedDict(owner) == NULL); PyDictValues *values = _PyObject_InlineValues(owner); PyObject *old_value = values->values[index]; values->values[index] = value; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index a426d9e208492e..058cac8bedd917 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -4017,16 +4017,14 @@ // _CHECK_ATTR_WITH_HINT { assert(Py_TYPE(owner)->tp_flags & Py_TPFLAGS_MANAGED_DICT); - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(owner); - PyDictObject *dict = managed_dict->dict; + PyDictObject *dict = _PyObject_GetManagedDict(owner); DEOPT_IF(dict == NULL, LOAD_ATTR); assert(PyDict_CheckExact((PyObject *)dict)); } // _LOAD_ATTR_WITH_HINT { uint16_t hint = read_u16(&this_instr[4].cache); - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(owner); - PyDictObject *dict = managed_dict->dict; + PyDictObject *dict = _PyObject_GetManagedDict(owner); DEOPT_IF(hint >= (size_t)dict->ma_keys->dk_nentries, LOAD_ATTR); PyObject *name = GETITEM(FRAME_CO_NAMES, oparg>>1); if (DK_IS_UNICODE(dict->ma_keys)) { @@ -5309,7 +5307,7 @@ { assert(Py_TYPE(owner)->tp_dictoffset < 0); assert(Py_TYPE(owner)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - DEOPT_IF(_PyObject_ManagedDictPointer(owner)->dict, STORE_ATTR); + DEOPT_IF(_PyObject_GetManagedDict(owner), STORE_ATTR); DEOPT_IF(_PyObject_InlineValues(owner)->valid == 0, STORE_ATTR); } // _STORE_ATTR_INSTANCE_VALUE @@ -5317,7 +5315,7 @@ { uint16_t index = read_u16(&this_instr[4].cache); STAT_INC(STORE_ATTR, hit); - assert(_PyObject_ManagedDictPointer(owner)->dict == NULL); + assert(_PyObject_GetManagedDict(owner) == NULL); PyDictValues *values = _PyObject_InlineValues(owner); PyObject *old_value = values->values[index]; values->values[index] = value; @@ -5380,8 +5378,7 @@ assert(type_version != 0); DEOPT_IF(tp->tp_version_tag != type_version, STORE_ATTR); assert(tp->tp_flags & Py_TPFLAGS_MANAGED_DICT); - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(owner); - PyDictObject *dict = managed_dict->dict; + PyDictObject *dict = _PyObject_GetManagedDict(owner); DEOPT_IF(dict == NULL, STORE_ATTR); assert(PyDict_CheckExact((PyObject *)dict)); PyObject *name = GETITEM(FRAME_CO_NAMES, oparg); diff --git a/Python/specialize.c b/Python/specialize.c index 5e14bb56b30036..ee51781372166a 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -852,8 +852,7 @@ specialize_dict_access( instr->op.code = values_op; } else { - PyManagedDictPointer *managed_dict = _PyObject_ManagedDictPointer(owner); - PyDictObject *dict = managed_dict->dict; + PyDictObject *dict = _PyObject_GetManagedDict(owner); if (dict == NULL || !PyDict_CheckExact(dict)) { SPECIALIZATION_FAIL(base_op, SPEC_FAIL_NO_DICT); return 0; diff --git a/Tools/cases_generator/analyzer.py b/Tools/cases_generator/analyzer.py index d17b2b9b024b99..18cefa08328804 100644 --- a/Tools/cases_generator/analyzer.py +++ b/Tools/cases_generator/analyzer.py @@ -354,6 +354,7 @@ def has_error_without_pop(op: parser.InstDef) -> bool: NON_ESCAPING_FUNCTIONS = ( "Py_INCREF", "_PyManagedDictPointer_IsValues", + "_PyObject_GetManagedDict", "_PyObject_ManagedDictPointer", "_PyObject_InlineValues", "_PyDictValues_AddToInsertionOrder", From 322914f4cc10952c9ff5d39885349c49d1ee97c6 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Thu, 4 Apr 2024 15:48:26 -0700 Subject: [PATCH 2/8] Insert into dict on store if dict is materialized and other feedback --- Objects/dictobject.c | 108 +++++++++++++++++++++++++++++-------------- Objects/object.c | 4 +- 2 files changed, 75 insertions(+), 37 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 62542e7b9501a9..91e8f86551206a 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -6616,6 +6616,23 @@ make_dict_from_instance_attributes(PyInterpreterState *interp, return res; } +static PyDictObject * +materialize_managed_dict_lock_held(PyObject *obj) +{ + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(obj); + + OBJECT_STAT_INC(dict_materialized_on_request); + + PyDictValues *values = _PyObject_InlineValues(obj); + PyInterpreterState *interp = _PyInterpreterState_GET(); + PyDictKeysObject *keys = CACHED_KEYS(Py_TYPE(obj)); + OBJECT_STAT_INC(dict_materialized_on_request); + PyDictObject *dict = make_dict_from_instance_attributes(interp, keys, values); + FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict, + (PyDictObject *)dict); + return dict; +} + PyDictObject * _PyObject_MaterializeManagedDict(PyObject *obj) { @@ -6633,17 +6650,7 @@ _PyObject_MaterializeManagedDict(PyObject *obj) goto exit; } #endif - - OBJECT_STAT_INC(dict_materialized_on_request); - - PyDictValues *values = _PyObject_InlineValues(obj); - PyInterpreterState *interp = _PyInterpreterState_GET(); - PyDictKeysObject *keys = CACHED_KEYS(Py_TYPE(obj)); - OBJECT_STAT_INC(dict_materialized_on_request); - dict = make_dict_from_instance_attributes(interp, keys, values); - - FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict, - (PyDictObject *)dict); + dict = materialize_managed_dict_lock_held(obj); #ifdef Py_GIL_DISABLED exit: @@ -6652,8 +6659,9 @@ _PyObject_MaterializeManagedDict(PyObject *obj) return dict; } - - +// Called with either the object's lock or the dict's lock held +// depending on whether or not a dict has been materialized for +// the object. static int store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, PyObject *name, PyObject *value) @@ -6703,14 +6711,38 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, } if (ix == DKIX_EMPTY) { + int res; if (dict == NULL) { - dict = _PyObject_MaterializeManagedDict(obj); + // Make the dict but don't publish it in the object + // so that no one else will see it. + dict = materialize_managed_dict_lock_held(obj); if (dict == NULL) { return -1; } + + if (value == NULL) { + res = PyDict_DelItem((PyObject *)dict, name); + } else { + res = PyDict_SetItem((PyObject *)dict, name, value); + } + + return res; } - // Caller needs to insert into materialized dict - return 1; + + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(dict); + + if (value == NULL) { + Py_hash_t hash; + if (!PyUnicode_CheckExact(name) || (hash = unicode_get_hash(name)) == -1) { + hash = PyObject_Hash(name); + if (hash == -1) + return -1; + } + res = delitem_knownhash_lock_held((PyObject *)dict, name, hash); + } else { + res = setitem_lock_held(dict, name, value); + } + return res; } PyObject *old_value = values->values[ix]; @@ -6742,12 +6774,32 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, return 0; } +static inline int +store_instance_attr_dict(PyObject *obj, PyDictObject *dict, PyObject *name, PyObject *value) +{ + PyDictValues *values = _PyObject_InlineValues(obj); + int res; + Py_BEGIN_CRITICAL_SECTION(dict); + if (dict->ma_values == values) { + res = store_instance_attr_lock_held(obj, values, name, value); + } + else { + if (value == NULL) { + res = PyDict_DelItem((PyObject *)dict, name); + } else { + res = PyDict_SetItem((PyObject *)dict, name, value); + } + } + Py_END_CRITICAL_SECTION(); + return res; +} + int _PyObject_TryStoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value) { PyDictValues *values = _PyObject_InlineValues(obj); if (!FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { - return 1; + return store_instance_attr_dict(obj, _PyObject_GetManagedDict(obj), name, value); } #ifdef Py_GIL_DISABLED @@ -6764,7 +6816,6 @@ _PyObject_TryStoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *val // prevent resizing. PyDictObject *dict = _PyObject_GetManagedDict(obj); if (dict == NULL) { - int retry_with_dict = 0; Py_BEGIN_CRITICAL_SECTION(obj); dict = _PyObject_GetManagedDict(obj); @@ -6774,24 +6825,13 @@ _PyObject_TryStoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *val else { // We lost a race with the materialization of the dict, we'll // try the insert with it... - retry_with_dict = 1; - } - Py_END_CRITICAL_SECTION(); - if (retry_with_dict) { goto with_dict; } + Py_END_CRITICAL_SECTION(); } else { with_dict: - Py_BEGIN_CRITICAL_SECTION(dict); - if (dict->ma_values == values) { - res = store_instance_attr_lock_held(obj, values, name, value); - } - else { - // Caller needs to insert into dict - res = 1; - } - Py_END_CRITICAL_SECTION(); + res = store_instance_attr_dict(obj, dict, name, value); } return res; #else @@ -6868,7 +6908,7 @@ _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr if (dict == NULL) { // Still no dict, we can read from the values assert(values->valid); - value = _Py_atomic_load_ptr_relaxed(&values->values[ix]); + value = values->values[ix]; *attr = Py_XNewRef(value); success = true; } @@ -7057,10 +7097,10 @@ _PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj) assert(mp->ma_values->embedded == 1); assert(mp->ma_values->valid == 1); assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - Py_BEGIN_CRITICAL_SECTION(mp); + mp->ma_values = copy_values(mp->ma_values); _PyObject_InlineValues(obj)->valid = 0; - Py_END_CRITICAL_SECTION(); + if (mp->ma_values == NULL) { return -1; } diff --git a/Objects/object.c b/Objects/object.c index b0f07b02f38849..9329cd5ad07139 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1699,9 +1699,7 @@ _PyObject_GenericSetAttrWithDict(PyObject *obj, PyObject *name, if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES)) { res = _PyObject_TryStoreInstanceAttribute(obj, name, value); - if (res <= 0) { - goto error_check; - } + goto error_check; } if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) { From 106685ac1c44e44e519ec25baa5177b5a9595ad4 Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Mon, 15 Apr 2024 13:27:08 -0700 Subject: [PATCH 3/8] _PyObject_TryStoreInstanceAttribute -> _PyObject_StoreInstanceAttribute Fix issue where critical section isn't released Make _PyObject_TryGetInstanceAttribute return a bool --- Include/internal/pycore_object.h | 6 +- .../internal/pycore_pyatomic_ft_wrappers.h | 3 + Objects/dictobject.c | 65 +++++++++---------- Objects/object.c | 6 +- Objects/typeobject.c | 2 +- 5 files changed, 41 insertions(+), 41 deletions(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 8ae83879562e9a..cdbdefa26cc200 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -660,10 +660,10 @@ extern PyObject* _PyType_GetDocFromInternalDoc(const char *, const char *); extern PyObject* _PyType_GetTextSignatureFromInternalDoc(const char *, const char *, int); void _PyObject_InitInlineValues(PyObject *obj, PyTypeObject *tp); -extern int _PyObject_TryStoreInstanceAttribute(PyObject *obj, +extern int _PyObject_StoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value); -extern int _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, - PyObject **attr); +extern bool _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, + PyObject **attr); #ifdef Py_GIL_DISABLED # define MANAGED_DICT_OFFSET (((Py_ssize_t)sizeof(PyObject *))*-1) diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index 3adb9ae99f581e..c0c6d4d547ddde 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -36,6 +36,8 @@ extern "C" { _Py_atomic_load_ptr_relaxed(&value) #define FT_ATOMIC_LOAD_UINT8_RELAXED(value) \ _Py_atomic_load_uint8_relaxed(&value) +#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) \ + _Py_atomic_store_uint8_relaxed(&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) \ @@ -57,6 +59,7 @@ extern "C" { #define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) value #define FT_ATOMIC_LOAD_PTR_RELAXED(value) value #define FT_ATOMIC_LOAD_UINT8_RELAXED(value) value +#define FT_ATOMIC_STORE_UINT8_RELAXED(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_UINTPTR_RELEASE(value, new_value) value = new_value diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 91e8f86551206a..afedde44c68119 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -6629,7 +6629,7 @@ materialize_managed_dict_lock_held(PyObject *obj) OBJECT_STAT_INC(dict_materialized_on_request); PyDictObject *dict = make_dict_from_instance_attributes(interp, keys, values); FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict, - (PyDictObject *)dict); + (PyDictObject *)dict); return dict; } @@ -6713,8 +6713,6 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, if (ix == DKIX_EMPTY) { int res; if (dict == NULL) { - // Make the dict but don't publish it in the object - // so that no one else will see it. dict = materialize_managed_dict_lock_held(obj); if (dict == NULL) { return -1; @@ -6795,7 +6793,7 @@ store_instance_attr_dict(PyObject *obj, PyDictObject *dict, PyObject *name, PyOb } int -_PyObject_TryStoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value) +_PyObject_StoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value) { PyDictValues *values = _PyObject_InlineValues(obj); if (!FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { @@ -6803,7 +6801,6 @@ _PyObject_TryStoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *val } #ifdef Py_GIL_DISABLED - int res; // We have a valid inline values, at least for now... There are two potential // races with having the values become invalid. One is the dictionary // being detached from the object. The other is if someone is inserting @@ -6816,24 +6813,20 @@ _PyObject_TryStoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *val // prevent resizing. PyDictObject *dict = _PyObject_GetManagedDict(obj); if (dict == NULL) { + int res; Py_BEGIN_CRITICAL_SECTION(obj); dict = _PyObject_GetManagedDict(obj); if (dict == NULL) { res = store_instance_attr_lock_held(obj, values, name, value); } - else { - // We lost a race with the materialization of the dict, we'll - // try the insert with it... - goto with_dict; - } Py_END_CRITICAL_SECTION(); + + if (dict == NULL) { + return res; + } } - else { -with_dict: - res = store_instance_attr_dict(obj, dict, name, value); - } - return res; + return store_instance_attr_dict(obj, dict, name, value); #else return store_instance_attr_lock_held(obj, values, name, value); #endif @@ -6873,13 +6866,13 @@ _PyObject_ManagedDictValidityCheck(PyObject *obj) // Attempts to get an instance attribute from the inline values. Returns 0 if // the lookup from the inline values was successful or 1 if the inline values // are no longer valid. No error is set in either case. -int +bool _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr) { assert(PyUnicode_CheckExact(name)); PyDictValues *values = _PyObject_InlineValues(obj); if (!FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { - return 1; + return false; } PyDictKeysObject *keys = CACHED_KEYS(Py_TYPE(obj)); @@ -6887,14 +6880,14 @@ _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr Py_ssize_t ix = _PyDictKeys_StringLookup(keys, name); if (ix == DKIX_EMPTY) { *attr = NULL; - return 0; + return true; } #ifdef Py_GIL_DISABLED PyObject *value = _Py_atomic_load_ptr_relaxed(&values->values[ix]); if (value == NULL || _Py_TryIncrefCompare(&values->values[ix], value)) { *attr = value; - return 0; + return true; } PyDictObject *dict = _PyObject_GetManagedDict(obj); @@ -6916,33 +6909,34 @@ _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr Py_END_CRITICAL_SECTION(); if (success) { - return 0; + return true; } } // We have a dictionary, we'll need to lock it to prevent // the values from being resized. assert(dict != NULL); - int res; + + bool success; Py_BEGIN_CRITICAL_SECTION(dict); if (dict->ma_values == values && FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { value = _Py_atomic_load_ptr_relaxed(&values->values[ix]); *attr = Py_XNewRef(value); - res = 0; + success = true; } else { // Caller needs to lookup from the dictionary - res = 1; + success = false; } Py_END_CRITICAL_SECTION(); - return res; + return success; #else PyObject *value = values->values[ix]; *attr = Py_XNewRef(value); - return 0; + return true; #endif } @@ -7003,14 +6997,9 @@ PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg) static bool set_dict_inline_values(PyObject *obj, PyObject *new_dict) { - PyDictValues *values = _PyObject_InlineValues(obj); + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(obj); - if (values->valid) { - for (Py_ssize_t i = 0; i < values->capacity; i++) { - Py_CLEAR(values->values[i]); - } - values->valid = 0; - } + PyDictValues *values = _PyObject_InlineValues(obj); #ifdef Py_GIL_DISABLED PyDictObject *dict = _PyObject_ManagedDictPointer(obj)->dict; @@ -7022,6 +7011,14 @@ set_dict_inline_values(PyObject *obj, PyObject *new_dict) Py_XINCREF(new_dict); _PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)new_dict; + + if (values->valid) { + FT_ATOMIC_STORE_UINT8_RELAXED(values->valid, 0); + for (Py_ssize_t i = 0; i < values->capacity; i++) { + Py_CLEAR(values->values[i]); + } + } + return true; } @@ -7032,7 +7029,7 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict) assert(_PyObject_InlineValuesConsistencyCheck(obj)); PyTypeObject *tp = Py_TYPE(obj); if (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) { - PyDictObject *dict = _PyObject_ManagedDictPointer(obj)->dict; + PyDictObject *dict = _PyObject_GetManagedDict(obj); if (dict) { #ifdef Py_GIL_DISABLED clear_dict: @@ -7120,7 +7117,7 @@ PyObject_GenericGetDict(PyObject *obj, void *context) dict = _PyObject_GetManagedDict(obj); if (dict == NULL && (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && - _PyObject_InlineValues(obj)->valid) { + FT_ATOMIC_LOAD_UINT8_RELAXED(_PyObject_InlineValues(obj)->valid)) { dict = _PyObject_MaterializeManagedDict(obj); } else if (dict == NULL) { diff --git a/Objects/object.c b/Objects/object.c index 9329cd5ad07139..91bb0114cbfc32 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1483,7 +1483,7 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method) } PyObject *dict, *attr; if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && - !_PyObject_TryGetInstanceAttribute(obj, name, &attr)) { + _PyObject_TryGetInstanceAttribute(obj, name, &attr)) { if (attr != NULL) { *method = attr; Py_XDECREF(descr); @@ -1587,7 +1587,7 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name, if (dict == NULL) { if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES)) { if (PyUnicode_CheckExact(name) && - !_PyObject_TryGetInstanceAttribute(obj, name, &res)) { + _PyObject_TryGetInstanceAttribute(obj, name, &res)) { if (res != NULL) { goto done; } @@ -1698,7 +1698,7 @@ _PyObject_GenericSetAttrWithDict(PyObject *obj, PyObject *name, PyObject **dictptr; if ((tp->tp_flags & Py_TPFLAGS_INLINE_VALUES)) { - res = _PyObject_TryStoreInstanceAttribute(obj, name, value); + res = _PyObject_StoreInstanceAttribute(obj, name, value); goto error_check; } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index fc82b6ae6dfc4d..3690ba10dd6626 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -6199,7 +6199,7 @@ object_set_class(PyObject *self, PyObject *value, void *closure) Py_BEGIN_CRITICAL_SECTION2(self, dict); - if (dict == NULL || _PyDict_DetachFromObject(dict, self)) { + if (dict == NULL || _PyDict_DetachFromObject(dict, self) < 0) { error = true; } From 11d450cc80dc77025459b26b9b916c19fbd1e6ec Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Tue, 16 Apr 2024 16:46:29 -0700 Subject: [PATCH 4/8] Cleanup _PyObject_SetManagedDict, use cst for inline valid flag, fix issue w/ deleted dict --- .../internal/pycore_pyatomic_ft_wrappers.h | 14 ++- Lib/test/test_class.py | 9 ++ Objects/dictobject.c | 114 ++++++++++-------- Objects/typeobject.c | 12 +- 4 files changed, 93 insertions(+), 56 deletions(-) diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index c0c6d4d547ddde..bbfc462a733d0e 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -21,6 +21,7 @@ extern "C" { #ifdef Py_GIL_DISABLED #define FT_ATOMIC_LOAD_PTR(value) _Py_atomic_load_ptr(&value) +#define FT_ATOMIC_STORE_PTR(value, new_value) _Py_atomic_store_ptr(&value, new_value) #define FT_ATOMIC_LOAD_SSIZE(value) _Py_atomic_load_ssize(&value) #define FT_ATOMIC_LOAD_SSIZE_ACQUIRE(value) \ _Py_atomic_load_ssize_acquire(&value) @@ -34,10 +35,10 @@ extern "C" { _Py_atomic_load_uintptr_acquire(&value) #define FT_ATOMIC_LOAD_PTR_RELAXED(value) \ _Py_atomic_load_ptr_relaxed(&value) -#define FT_ATOMIC_LOAD_UINT8_RELAXED(value) \ - _Py_atomic_load_uint8_relaxed(&value) -#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) \ - _Py_atomic_store_uint8_relaxed(&value, new_value) +#define FT_ATOMIC_LOAD_UINT8(value) \ + _Py_atomic_load_uint8(&value) +#define FT_ATOMIC_STORE_UINT8(value, new_value) \ + _Py_atomic_store_uint8(&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) \ @@ -51,6 +52,7 @@ extern "C" { #else #define FT_ATOMIC_LOAD_PTR(value) value +#define FT_ATOMIC_STORE_PTR(value, new_value) value = new_value #define FT_ATOMIC_LOAD_SSIZE(value) value #define FT_ATOMIC_LOAD_SSIZE_ACQUIRE(value) value #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) value @@ -58,8 +60,8 @@ extern "C" { #define FT_ATOMIC_LOAD_PTR_ACQUIRE(value) value #define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) value #define FT_ATOMIC_LOAD_PTR_RELAXED(value) value -#define FT_ATOMIC_LOAD_UINT8_RELAXED(value) value -#define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) value = new_value +#define FT_ATOMIC_LOAD_UINT8(value) value +#define FT_ATOMIC_STORE_UINT8(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_UINTPTR_RELEASE(value, new_value) value = new_value diff --git a/Lib/test/test_class.py b/Lib/test/test_class.py index a9cfd8df691845..5885db84b66b01 100644 --- a/Lib/test/test_class.py +++ b/Lib/test/test_class.py @@ -873,6 +873,15 @@ def __init__(self): obj.foo = None # Aborted here self.assertEqual(obj.__dict__, {"foo":None}) + def test_store_attr_deleted_dict(self): + class Foo: + pass + + f = Foo() + del f.__dict__ + f.a = 3 + self.assertEqual(f.a, 3) + if __name__ == '__main__': unittest.main() diff --git a/Objects/dictobject.c b/Objects/dictobject.c index afedde44c68119..7c9d865da975e2 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -6796,8 +6796,15 @@ int _PyObject_StoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value) { PyDictValues *values = _PyObject_InlineValues(obj); - if (!FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { - return store_instance_attr_dict(obj, _PyObject_GetManagedDict(obj), name, value); + if (!FT_ATOMIC_LOAD_UINT8(values->valid)) { + PyDictObject *dict = _PyObject_GetManagedDict(obj); + if (dict == NULL) { + dict = (PyDictObject *)PyObject_GenericGetDict(obj, NULL); + if (dict == NULL) { + return -1; + } + } + return store_instance_attr_dict(obj, dict, name, value); } #ifdef Py_GIL_DISABLED @@ -6871,7 +6878,7 @@ _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr { assert(PyUnicode_CheckExact(name)); PyDictValues *values = _PyObject_InlineValues(obj); - if (!FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { + if (!FT_ATOMIC_LOAD_UINT8(values->valid)) { return false; } @@ -6920,8 +6927,7 @@ _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr bool success; Py_BEGIN_CRITICAL_SECTION(dict); - if (dict->ma_values == values && - FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { + if (dict->ma_values == values && FT_ATOMIC_LOAD_UINT8(values->valid)) { value = _Py_atomic_load_ptr_relaxed(&values->values[ix]); *attr = Py_XNewRef(value); success = true; @@ -6950,7 +6956,7 @@ _PyObject_IsInstanceDictEmpty(PyObject *obj) PyDictObject *dict; if (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) { PyDictValues *values = _PyObject_InlineValues(obj); - if (FT_ATOMIC_LOAD_UINT8_RELAXED(values->valid)) { + if (FT_ATOMIC_LOAD_UINT8(values->valid)) { PyDictKeysObject *keys = CACHED_KEYS(tp); for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) { if (FT_ATOMIC_LOAD_PTR_RELAXED(values->values[i]) != NULL) { @@ -6994,32 +7000,22 @@ PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg) return 0; } -static bool -set_dict_inline_values(PyObject *obj, PyObject *new_dict) +static void +set_dict_inline_values(PyObject *obj, PyDictObject *new_dict) { _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(obj); PyDictValues *values = _PyObject_InlineValues(obj); -#ifdef Py_GIL_DISABLED - PyDictObject *dict = _PyObject_ManagedDictPointer(obj)->dict; - if (dict != NULL) { - // We lost a race with materialization of the dict - return false; - } -#endif - Py_XINCREF(new_dict); - _PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)new_dict; + FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict, new_dict); if (values->valid) { - FT_ATOMIC_STORE_UINT8_RELAXED(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]); } } - - return true; } void @@ -7030,47 +7026,67 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict) PyTypeObject *tp = Py_TYPE(obj); if (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) { PyDictObject *dict = _PyObject_GetManagedDict(obj); - if (dict) { + if (dict == NULL) { #ifdef Py_GIL_DISABLED -clear_dict: -#endif - Py_BEGIN_CRITICAL_SECTION2(dict, obj); - - _PyDict_DetachFromObject(dict, obj); + Py_BEGIN_CRITICAL_SECTION(obj); - Py_XINCREF(new_dict); - _PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)new_dict; + dict = _PyObject_ManagedDictPointer(obj)->dict; + if (dict == NULL) { + set_dict_inline_values(obj, (PyDictObject *)new_dict); + } - Py_END_CRITICAL_SECTION2(); + Py_END_CRITICAL_SECTION(); - Py_DECREF(dict); + if (dict == NULL) { + return; + } +#else + set_dict_inline_values(obj, (PyDictObject *)new_dict); + return; +#endif } - else { - bool success; - Py_BEGIN_CRITICAL_SECTION(obj); - success = set_dict_inline_values(obj, new_dict); + Py_BEGIN_CRITICAL_SECTION2(dict, obj); - Py_END_CRITICAL_SECTION(); + // If the dict in the object has been replaced between when we + // got the dict and unlocked the objects then it's + // definitely no longer inline and there's no need to detach + // it, we can just replace it. + PyDictObject *cur_dict = _PyObject_ManagedDictPointer(obj)->dict; + assert(cur_dict == dict || + (cur_dict->ma_values != _PyObject_InlineValues(obj) && + !_PyObject_InlineValues(obj)->valid)); - (void)success; // suppress warning when GIL isn't disabled + Py_XINCREF(new_dict); + FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict, + (PyDictObject *)Py_XNewRef(new_dict)); -#ifdef Py_GIL_DISABLED - if (!success) { - dict = _PyObject_ManagedDictPointer(obj)->dict; - assert(dict != NULL); - goto clear_dict; - } -#endif + // If we got a replacement dict after locking the object and the dict + // then the old dict had to already have been detached. + assert(cur_dict == dict || + dict->ma_values != _PyObject_InlineValues(obj)); + + if (cur_dict == dict) { + _PyDict_DetachFromObject(dict, obj); } + + Py_END_CRITICAL_SECTION2(); + + Py_DECREF(dict); } else { + PyDictObject *dict; + Py_BEGIN_CRITICAL_SECTION(obj); - Py_XSETREF(_PyObject_ManagedDictPointer(obj)->dict, - (PyDictObject *)Py_XNewRef(new_dict)); + dict = _PyObject_ManagedDictPointer(obj)->dict; + + FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict, + (PyDictObject *)Py_XNewRef(new_dict)); Py_END_CRITICAL_SECTION(); + + Py_XDECREF(dict); } assert(_PyObject_InlineValuesConsistencyCheck(obj)); } @@ -7085,9 +7101,8 @@ int _PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj) { _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(mp); + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(obj); - assert(_PyObject_ManagedDictPointer(obj)->dict == mp); - assert(_PyObject_InlineValuesConsistencyCheck(obj)); if (mp->ma_values == NULL || mp->ma_values != _PyObject_InlineValues(obj)) { return 0; } @@ -7096,12 +7111,13 @@ _PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj) assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES); mp->ma_values = copy_values(mp->ma_values); - _PyObject_InlineValues(obj)->valid = 0; if (mp->ma_values == NULL) { return -1; } + FT_ATOMIC_STORE_UINT8(_PyObject_InlineValues(obj)->valid, 0); + assert(_PyObject_InlineValuesConsistencyCheck(obj)); ASSERT_CONSISTENT(mp); return 0; @@ -7117,7 +7133,7 @@ PyObject_GenericGetDict(PyObject *obj, void *context) dict = _PyObject_GetManagedDict(obj); if (dict == NULL && (tp->tp_flags & Py_TPFLAGS_INLINE_VALUES) && - FT_ATOMIC_LOAD_UINT8_RELAXED(_PyObject_InlineValues(obj)->valid)) { + FT_ATOMIC_LOAD_UINT8(_PyObject_InlineValues(obj)->valid)) { dict = _PyObject_MaterializeManagedDict(obj); } else if (dict == NULL) { diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 3690ba10dd6626..79589b04c6e4a6 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -6195,11 +6195,21 @@ object_set_class(PyObject *self, PyObject *value, void *closure) * so we must materialize the dictionary first. */ if (oldto->tp_flags & Py_TPFLAGS_INLINE_VALUES) { PyDictObject *dict = _PyObject_MaterializeManagedDict(self); + if (dict == NULL) { + return -1; + } + bool error = false; Py_BEGIN_CRITICAL_SECTION2(self, dict); - if (dict == NULL || _PyDict_DetachFromObject(dict, self) < 0) { + // If we raced after materialization and replaced the dict + // then the materialized dict should no longer have the + // inline values in which case detach is a nop. + assert(_PyObject_GetManagedDict(self) == dict || + dict->ma_values != _PyObject_InlineValues(self)); + + if (_PyDict_DetachFromObject(dict, self) < 0) { error = true; } From 29968e05e9ab70ec58c0ff6e5d5ab2d04c9c532f Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Wed, 17 Apr 2024 16:48:02 -0700 Subject: [PATCH 5/8] Refactor del/set into set_or_del_lock_held, and avoid locking on newly materialized dict Fix duplicate incref Fix comment Remove redundant if check on detach --- Include/internal/pycore_object.h | 2 +- Objects/dictobject.c | 77 +++++++++++++++----------------- 2 files changed, 37 insertions(+), 42 deletions(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index cdbdefa26cc200..88b052f4544b15 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -661,7 +661,7 @@ extern PyObject* _PyType_GetTextSignatureFromInternalDoc(const char *, const cha void _PyObject_InitInlineValues(PyObject *obj, PyTypeObject *tp); extern int _PyObject_StoreInstanceAttribute(PyObject *obj, - PyObject *name, PyObject *value); + PyObject *name, PyObject *value); extern bool _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 7c9d865da975e2..237b5b25bcb896 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -6659,6 +6659,22 @@ _PyObject_MaterializeManagedDict(PyObject *obj) return dict; } +static int +set_or_del_lock_held(PyDictObject *dict, PyObject *name, PyObject *value) +{ + if (value == NULL) { + Py_hash_t hash; + if (!PyUnicode_CheckExact(name) || (hash = unicode_get_hash(name)) == -1) { + hash = PyObject_Hash(name); + if (hash == -1) + return -1; + } + return delitem_knownhash_lock_held((PyObject *)dict, name, hash); + } else { + return setitem_lock_held(dict, name, value); + } +} + // Called with either the object's lock or the dict's lock held // depending on whether or not a dict has been materialized for // the object. @@ -6713,33 +6729,22 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, if (ix == DKIX_EMPTY) { int res; if (dict == NULL) { - dict = materialize_managed_dict_lock_held(obj); - if (dict == NULL) { + // Make the dict but don't publish it in the object + // so that no one else will see it. + dict = make_dict_from_instance_attributes(PyInterpreterState_Get(), keys, values); + if (dict == NULL || + set_or_del_lock_held(dict, name, value) < 0) { return -1; } - if (value == NULL) { - res = PyDict_DelItem((PyObject *)dict, name); - } else { - res = PyDict_SetItem((PyObject *)dict, name, value); - } - - return res; + FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict, + (PyDictObject *)dict); + return 0; } _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(dict); - if (value == NULL) { - Py_hash_t hash; - if (!PyUnicode_CheckExact(name) || (hash = unicode_get_hash(name)) == -1) { - hash = PyObject_Hash(name); - if (hash == -1) - return -1; - } - res = delitem_knownhash_lock_held((PyObject *)dict, name, hash); - } else { - res = setitem_lock_held(dict, name, value); - } + res = set_or_del_lock_held (dict, name, value); return res; } @@ -6782,11 +6787,7 @@ store_instance_attr_dict(PyObject *obj, PyDictObject *dict, PyObject *name, PyOb res = store_instance_attr_lock_held(obj, values, name, value); } else { - if (value == NULL) { - res = PyDict_DelItem((PyObject *)dict, name); - } else { - res = PyDict_SetItem((PyObject *)dict, name, value); - } + res = set_or_del_lock_held(dict, name, value); } Py_END_CRITICAL_SECTION(); return res; @@ -6870,9 +6871,8 @@ _PyObject_ManagedDictValidityCheck(PyObject *obj) } #endif -// Attempts to get an instance attribute from the inline values. Returns 0 if -// the lookup from the inline values was successful or 1 if the inline values -// are no longer valid. No error is set in either case. +// Attempts to get an instance attribute from the inline values. Returns true +// if successful, or false if the caller needs to lookup in the dictionary. bool _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr) { @@ -7048,27 +7048,22 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict) Py_BEGIN_CRITICAL_SECTION2(dict, obj); - // If the dict in the object has been replaced between when we - // got the dict and unlocked the objects then it's - // definitely no longer inline and there's no need to detach - // it, we can just replace it. +#ifdef Py_DEBUG + // If the dict in the object has been replaced between when we got + // the dict and unlocked the objects then it's definitely no longer + // inline and there's no need to detach it, we can just replace it. + // The call to _PyDict_DetachFromObject will be a nop. PyDictObject *cur_dict = _PyObject_ManagedDictPointer(obj)->dict; assert(cur_dict == dict || (cur_dict->ma_values != _PyObject_InlineValues(obj) && + dict->ma_values != _PyObject_InlineValues(obj) && !_PyObject_InlineValues(obj)->valid)); +#endif - Py_XINCREF(new_dict); FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict, (PyDictObject *)Py_XNewRef(new_dict)); - // If we got a replacement dict after locking the object and the dict - // then the old dict had to already have been detached. - assert(cur_dict == dict || - dict->ma_values != _PyObject_InlineValues(obj)); - - if (cur_dict == dict) { - _PyDict_DetachFromObject(dict, obj); - } + _PyDict_DetachFromObject(dict, obj); Py_END_CRITICAL_SECTION2(); From 0023395cc467e318625555428dabe745f201ef3b Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Fri, 19 Apr 2024 09:48:01 -0700 Subject: [PATCH 6/8] Handle dict replacement case better --- Objects/dictobject.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 237b5b25bcb896..4b06e458a6513a 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -7048,17 +7048,9 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict) Py_BEGIN_CRITICAL_SECTION2(dict, obj); -#ifdef Py_DEBUG - // If the dict in the object has been replaced between when we got - // the dict and unlocked the objects then it's definitely no longer - // inline and there's no need to detach it, we can just replace it. - // The call to _PyDict_DetachFromObject will be a nop. - PyDictObject *cur_dict = _PyObject_ManagedDictPointer(obj)->dict; - assert(cur_dict == dict || - (cur_dict->ma_values != _PyObject_InlineValues(obj) && - dict->ma_values != _PyObject_InlineValues(obj) && - !_PyObject_InlineValues(obj)->valid)); -#endif + // We've locked dict, but the actual dict could have changed + // since we locked it. + dict = _PyObject_ManagedDictPointer(obj)->dict; FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict, (PyDictObject *)Py_XNewRef(new_dict)); @@ -7067,7 +7059,7 @@ _PyObject_SetManagedDict(PyObject *obj, PyObject *new_dict) Py_END_CRITICAL_SECTION2(); - Py_DECREF(dict); + Py_XDECREF(dict); } else { PyDictObject *dict; @@ -7095,21 +7087,25 @@ PyObject_ClearManagedDict(PyObject *obj) int _PyDict_DetachFromObject(PyDictObject *mp, PyObject *obj) { - _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(mp); _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(obj); - if (mp->ma_values == NULL || mp->ma_values != _PyObject_InlineValues(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. + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(mp); assert(mp->ma_values->embedded == 1); assert(mp->ma_values->valid == 1); assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_INLINE_VALUES); - mp->ma_values = copy_values(mp->ma_values); + PyDictValues *values = copy_values(mp->ma_values); - if (mp->ma_values == NULL) { + if (values == NULL) { return -1; } + mp->ma_values = values; FT_ATOMIC_STORE_UINT8(_PyObject_InlineValues(obj)->valid, 0); From 8433d25fa63f396dc40aa5cb7c0d235d31155b8e Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Fri, 19 Apr 2024 11:21:11 -0700 Subject: [PATCH 7/8] Decref materialized dict on fail to set --- Objects/dictobject.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 4b06e458a6513a..b30b8d514fa492 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -6734,6 +6734,7 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values, dict = make_dict_from_instance_attributes(PyInterpreterState_Get(), keys, values); if (dict == NULL || set_or_del_lock_held(dict, name, value) < 0) { + Py_XDECREF(dict); return -1; } From ac503e8ae657f127fd06abe8e17f8c96815f8fce Mon Sep 17 00:00:00 2001 From: Dino Viehland Date: Fri, 19 Apr 2024 14:33:13 -0700 Subject: [PATCH 8/8] Fix missing decref when storing and dict is detached but has been deleted --- Objects/dictobject.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index b30b8d514fa492..2644516bc30770 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -6805,6 +6805,9 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyObject *name, PyObject *value) if (dict == NULL) { return -1; } + int res = store_instance_attr_dict(obj, dict, name, value); + Py_DECREF(dict); + return res; } return store_instance_attr_dict(obj, dict, name, value); }