diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index 5a496d59ab7af79..656ff2ebde69cd0 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -245,7 +245,7 @@ _PyDict_NotifyEvent(PyInterpreterState *interp, return DICT_NEXT_VERSION(interp) | (mp->ma_version_tag & DICT_WATCHER_AND_MODIFICATION_MASK); } -extern PyObject *_PyObject_MakeDictFromInstanceAttributes(PyObject *obj, PyDictValues *values); +extern PyObject *_PyObject_MaterializeManagedDict(PyObject *obj); extern bool _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv); extern PyObject *_PyDict_FromItems( PyObject *const *keys, Py_ssize_t keys_offset, @@ -264,6 +264,37 @@ _PyDictValues_AddToInsertionOrder(PyDictValues *values, Py_ssize_t ix) *size_ptr = size; } +#ifdef Py_GIL_DISABLED + +static inline bool +_PyDictOrValues_TryStoreValue(PyObject *obj, Py_ssize_t index, PyObject *value) +{ + bool success; + Py_BEGIN_CRITICAL_SECTION(obj); + + PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj); + if (!_PyDictOrValues_IsValues(*dorv_ptr)) { + success = false; + goto exit; + } + + PyDictValues *values = _PyDictOrValues_GetValues(*dorv_ptr); + PyObject *old_value = _Py_atomic_load_ptr_relaxed(&values->values[index]); + _Py_atomic_store_ptr_relaxed(&values->values[index], value); + if (old_value == NULL) { + _PyDictValues_AddToInsertionOrder(values, index); + } + else { + Py_DECREF(old_value); + } + success = true; +exit: + Py_END_CRITICAL_SECTION(); + return success; +} + +#endif + #ifdef __cplusplus } #endif diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 8f4773c707e1b7b..c439d3549d741f8 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -9,11 +9,12 @@ extern "C" { #endif #include -#include "pycore_gc.h" // _PyObject_GC_IS_TRACKED() +#include "pycore_gc.h" // _PyObject_GC_IS_TRACKED() #include "pycore_emscripten_trampoline.h" // _PyCFunction_TrampolineCall() -#include "pycore_interp.h" // PyInterpreterState.gc -#include "pycore_pystate.h" // _PyInterpreterState_GET() -#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION +#include "pycore_interp.h" // PyInterpreterState.gc +#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_STORE_PTR_RELAXED +#include "pycore_pystate.h" // _PyInterpreterState_GET() +#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION /* Check if an object is consistent. For example, ensure that the reference counter is greater than or equal to 1, and ensure that ob_type is not NULL. @@ -653,7 +654,7 @@ _PyObject_DictOrValuesPointer(PyObject *obj) static inline int _PyDictOrValues_IsValues(PyDictOrValues dorv) { - return ((uintptr_t)dorv.values) & 1; + return ((uintptr_t)FT_ATOMIC_LOAD_PTR_RELAXED(dorv.values)) & 1; } // Should only be used when we know the object isn't racing with other @@ -662,7 +663,7 @@ static inline PyDictValues * _PyDictOrValues_GetValues(PyDictOrValues dorv) { assert(_PyDictOrValues_IsValues(dorv)); - return (PyDictValues *)(dorv.values + 1); + return (PyDictValues *)(((char *)FT_ATOMIC_LOAD_PTR_RELAXED(dorv.values)) + 1); } static inline PyDictValues * @@ -698,31 +699,19 @@ static inline PyObject * _PyDictOrValues_GetDict(PyDictOrValues dorv) { assert(!_PyDictOrValues_IsValues(dorv)); -#ifdef Py_GIL_DISABLED - return _Py_atomic_load_ptr_relaxed(&dorv.dict); -#else - return dorv.dict; -#endif + return FT_ATOMIC_LOAD_PTR_RELAXED(dorv.dict); } static inline void _PyDictOrValues_SetValues(PyDictOrValues *ptr, PyDictValues *values) { -#ifdef Py_GIL_DISABLED - _Py_atomic_store_ptr_relaxed(&ptr->values, (char *)values - 1); -#else - ptr->values = ((char *)values) - 1; -#endif + FT_ATOMIC_STORE_PTR_RELAXED(ptr->values, (char *)values - 1); } static inline void _PyDictOrValues_SetDict(PyDictOrValues *ptr, PyObject *dict) { -#ifdef Py_GIL_DISABLED - _Py_atomic_store_ptr_relaxed(&ptr->dict, dict); -#else - ptr->dict = dict; -#endif + FT_ATOMIC_STORE_PTR_RELAXED(ptr->dict, dict); } extern PyObject ** _PyObject_ComputedDictPointer(PyObject *); diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index e441600d54e1aac..af92306d7d7c751 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_LOAD_PTR_RELAXED(value) \ + _Py_atomic_load_ptr_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) \ @@ -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_LOAD_PTR_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_SSIZE_RELAXED(value, new_value) value = new_value diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 7e1e53cbc078c8e..12a5d95fb9f6195 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -6415,24 +6415,48 @@ make_dict_from_instance_attributes(PyInterpreterState *interp, } PyObject * -_PyObject_MakeDictFromInstanceAttributes(PyObject *obj, PyDictValues *values) +_PyObject_MaterializeManagedDict(PyObject *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); -} + PyTypeObject *tp = Py_TYPE(obj); + PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj); + PyObject *dict; + + assert(_PyType_HasFeature(tp, Py_TPFLAGS_MANAGED_DICT)); + + Py_BEGIN_CRITICAL_SECTION(obj); -static bool -has_unique_reference(PyObject *op) -{ #ifdef Py_GIL_DISABLED - return (_Py_IsOwnedByCurrentThread(op) && - op->ob_ref_local == 1 && - _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared) == 0); + // Check again that we didn't race with some other thread materializing dict + if (!_PyDictOrValues_IsValues(*dorv_ptr)) { + dict = _PyDictOrValues_GetDict(*dorv_ptr); + goto got_dict; + } #else - return Py_REFCNT(op) == 1; + assert(_PyDictOrValues_IsValues(*dorv_ptr)); +#endif + + OBJECT_STAT_INC(dict_materialized_on_request); + dict = make_dict_from_instance_attributes( + interp, CACHED_KEYS(tp), _PyDictOrValues_GetValues(*dorv_ptr)); + + if (dict != NULL) { +#ifdef Py_GIL_DISABLED + if (_PyObject_GC_IS_SHARED(obj)) { + // The values were accessed from multiple threads and need to be + // freed via QSBR, now the dict needs to be shared as it owns the + // values. + _PyObject_GC_SET_SHARED(dict); + } #endif + _PyDictOrValues_SetDict(dorv_ptr, dict); + } + +#ifdef Py_GIL_DISABLED +got_dict: +#endif + Py_END_CRITICAL_SECTION(); + return dict; } // Return true if the dict was dematerialized, false otherwise. @@ -6456,7 +6480,7 @@ _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv) } assert(_PyType_HasFeature(Py_TYPE(obj), Py_TPFLAGS_HEAPTYPE)); if (dict->ma_keys != CACHED_KEYS(Py_TYPE(obj)) || - !has_unique_reference((PyObject *)dict)) + Py_REFCNT(dict) != 1) { return false; } @@ -6693,24 +6717,8 @@ PyObject_GenericGetDict(PyObject *obj, void *context) PyTypeObject *tp = Py_TYPE(obj); if (_PyType_HasFeature(tp, Py_TPFLAGS_MANAGED_DICT)) { PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj); - PyDictValues *values; - if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) { - OBJECT_STAT_INC(dict_materialized_on_request); - Py_BEGIN_CRITICAL_SECTION(obj); - dict = make_dict_from_instance_attributes( - interp, CACHED_KEYS(tp), values); - if (dict != NULL) { -#ifdef Py_GIL_DISABLED - if (_PyObject_GC_IS_SHARED(obj)) { - // The values were accessed from multiple threads and need to be - // freed via QSBR, now the dict needs to be shared as it owns the - // values. - _PyObject_GC_SET_SHARED(dict); - } -#endif - _PyDictOrValues_SetDict(dorv_ptr, dict); - } - Py_END_CRITICAL_SECTION(); + if (_PyDictOrValues_IsValues(*dorv_ptr)) { + dict = _PyObject_MaterializeManagedDict(obj); } else { dict = _PyDictOrValues_GetDict(*dorv_ptr); diff --git a/Objects/object.c b/Objects/object.c index b01afc4f1aa8361..9fff29251aae506 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -7,7 +7,7 @@ #include "pycore_ceval.h" // _Py_EnterRecursiveCallTstate() #include "pycore_context.h" // _PyContextTokenMissing_Type #include "pycore_descrobject.h" // _PyMethodWrapper_Type -#include "pycore_dict.h" // _PyObject_MakeDictFromInstanceAttributes() +#include "pycore_dict.h" // _PyObject_MaterializeManagedDict() #include "pycore_floatobject.h" // _PyFloat_DebugMallocStats() #include "pycore_initconfig.h" // _PyStatus_EXCEPTION() #include "pycore_hashtable.h" // _Py_hashtable_new() @@ -1398,34 +1398,9 @@ _PyObject_GetDictPtr(PyObject *obj) return _PyObject_ComputedDictPointer(obj); } PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj); - PyDictValues *values; - if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) { - PyObject *dict; - Py_BEGIN_CRITICAL_SECTION(obj); -#ifdef Py_GIL_DISABLED - if ((values = _PyDictOrValues_TryGetValues(obj)) == NULL) { - dict = _PyDictOrValues_GetDict(*dorv_ptr); - goto done; - } -#endif - - dict = _PyObject_MakeDictFromInstanceAttributes(obj, values); - if (dict == NULL) { - goto done; - } - -#ifdef Py_GIL_DISABLED - if (_PyObject_GC_IS_SHARED(obj)) { - // The values were accessed from multiple threads and need to be - // freed via QSBR, now the dict needs to be shared as it owns the - // values. - _PyObject_GC_SET_SHARED(dict); - } -#endif - dorv_ptr->dict = dict; + if (_PyDictOrValues_IsValues(*dorv_ptr)) { + PyObject *dict = _PyObject_MaterializeManagedDict(obj); -done: - Py_END_CRITICAL_SECTION(); if (dict == NULL) { PyErr_Clear(); return NULL; @@ -1618,12 +1593,11 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name, } } else { - dict = _PyObject_MakeDictFromInstanceAttributes(obj, values); + dict = _PyObject_MaterializeManagedDict(obj); if (dict == NULL) { res = NULL; goto done; } - dorv_ptr->dict = dict; } } else { @@ -1725,23 +1699,31 @@ _PyObject_GenericSetAttrWithDict(PyObject *obj, PyObject *name, PyObject **dictptr; if ((tp->tp_flags & Py_TPFLAGS_MANAGED_DICT)) { PyDictOrValues *dorv_ptr = _PyObject_DictOrValuesPointer(obj); - PyDictValues *values; - if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) { + bool stored_inline = false; + + Py_BEGIN_CRITICAL_SECTION(obj); + + if (_PyDictOrValues_IsValues(*dorv_ptr)) { res = _PyObject_StoreInstanceAttribute( - obj, values, name, value); - goto error_check; + obj, _PyDictOrValues_GetValues(*dorv_ptr), name, value); + stored_inline = true; + goto managed_dict_done; } dictptr = &dorv_ptr->dict; if (*dictptr == NULL) { if (_PyObject_InitInlineValues(obj, tp) < 0) { goto done; } - if ((values = _PyDictOrValues_TryGetValues(obj)) != NULL) { - res = _PyObject_StoreInstanceAttribute( - obj, values, name, value); - goto error_check; - } + res = _PyObject_StoreInstanceAttribute( + obj, _PyDictOrValues_GetValues(*dorv_ptr), name, value); + stored_inline = true; + goto managed_dict_done; } +managed_dict_done: + Py_END_CRITICAL_SECTION(); + + if (stored_inline) + goto error_check; } else { dictptr = _PyObject_ComputedDictPointer(obj); diff --git a/Python/bytecodes.c b/Python/bytecodes.c index eec4a541b25b4a6..7dfe9dc43e6822a 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2094,10 +2094,11 @@ dummy_func( op(_STORE_ATTR_INSTANCE_VALUE, (index/1, value, owner --)) { STAT_INC(STORE_ATTR, hit); +#if Py_GIL_DISABLED + bool success = _PyDictOrValues_TryStoreValue(owner, index, value); + DEOPT_IF(!success); +#else PyDictValues *values = _PyDictOrValues_TryGetValues(owner); -#ifdef Py_GIL_DISABLED - DEOPT_IF(values == NULL); -#endif PyObject *old_value = values->values[index]; values->values[index] = value; if (old_value == NULL) { @@ -2106,6 +2107,7 @@ dummy_func( else { Py_DECREF(old_value); } +#endif Py_DECREF(owner); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index d1825c26bb68771..19f6155443946e9 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2043,10 +2043,11 @@ value = stack_pointer[-2]; uint16_t index = (uint16_t)CURRENT_OPERAND(); STAT_INC(STORE_ATTR, hit); + #if Py_GIL_DISABLED + bool success = _PyDictOrValues_TryStoreValue(owner, index, value); + if (!success) goto deoptimize; + #else PyDictValues *values = _PyDictOrValues_TryGetValues(owner); - #ifdef Py_GIL_DISABLED - if (values == NULL) goto deoptimize; - #endif PyObject *old_value = values->values[index]; values->values[index] = value; if (old_value == NULL) { @@ -2055,6 +2056,7 @@ else { Py_DECREF(old_value); } + #endif Py_DECREF(owner); stack_pointer += -2; break; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index f35e2d938fbaf68..343e06a25373a9d 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5194,10 +5194,11 @@ { uint16_t index = read_u16(&this_instr[4].cache); STAT_INC(STORE_ATTR, hit); + #if Py_GIL_DISABLED + bool success = _PyDictOrValues_TryStoreValue(owner, index, value); + DEOPT_IF(!success, STORE_ATTR); + #else PyDictValues *values = _PyDictOrValues_TryGetValues(owner); - #ifdef Py_GIL_DISABLED - DEOPT_IF(values == NULL, STORE_ATTR); - #endif PyObject *old_value = values->values[index]; values->values[index] = value; if (old_value == NULL) { @@ -5206,6 +5207,7 @@ else { Py_DECREF(old_value); } + #endif Py_DECREF(owner); } stack_pointer += -2;