Always lock object on writes to managed values

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

Include/internal/pycore_object.h

@@ -9,11 +9,12 @@ extern "C" {
 #endif
 
 #include <stdbool.h>
-#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 *);

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

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);

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);

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);
         }