Add comment, inline _PyDict_SplitKeysInvalidated, remove print, fix race in specialize_instance_load_attr

Author: DinoV

Include/internal/pycore_dict.h

@@ -91,7 +91,6 @@ typedef struct {
 
 extern PyDictKeysObject *_PyDict_NewKeysForClass(PyHeapTypeObject *);
 extern void _PyDict_RemoveKeysForClass(PyHeapTypeObject *);
-extern void _PyDict_SplitKeysInvalidated(PyDictKeysObject* keys);
 extern PyObject *_PyDict_FromKeys(PyObject *, PyObject *, PyObject *);
 
 /* Implementations of the `|` and `|=` operators for dict, used by the

Lib/test/test_free_threading/test_type.py

@@ -236,10 +236,6 @@ def writer(tid):
         for t in writers:
             t.join()
 
-        print(
-            "rounds=%d writers=%d stoppers=%d -> stale reads = %d"
-            % (NROUNDS, NWRITERS, NSTOPPERS, len(bugs))
-        )
         self.assertFalse(bugs)
 
     def run_one(self, writer_func, reader_func):

Objects/dictobject.c

@@ -1952,6 +1952,10 @@ insert_split_key(PyDictKeysObject *keys, PyObject *key, Py_hash_t hash)
         return ix;
     }
 
+    // We need to acquire the type lock before the keys mutex. Another lock
+    // is never acquired below the keys mutex but a keys mutex can be acquired
+    // elsewhere while we hold the types lock. To avoid deadlocks we must always
+    // acquire the type lock first.
     Py_BEGIN_CRITICAL_SECTION_MUTEX(&_PyInterpreterState_GET()->types.mutex);
 #endif
 
@@ -1960,7 +1964,12 @@ insert_split_key(PyDictKeysObject *keys, PyObject *key, Py_hash_t hash)
     if (ix == DKIX_EMPTY && keys->dk_usable > 0) {
         // Insert into new slot
         FT_ATOMIC_STORE_UINT32_RELAXED(keys->dk_version, 0);
-        _PyDict_SplitKeysInvalidated(keys);
+        struct _instancekeysobject *shared_keys = _PyDictKeys_AsSharedKeys(keys);
+        PyTypeObject *type = FT_ATOMIC_LOAD_PTR_ACQUIRE(shared_keys->dsk_owning_type);
+        if (type) {
+            // we acquired the type lock above
+            _PyType_Modified_Unlocked(type);
+        }
         Py_ssize_t hashpos = find_empty_slot(keys, hash);
         ix = keys->dk_nentries;
         dictkeys_set_index(keys, hashpos, ix);
@@ -7293,16 +7302,6 @@ _PyDict_RemoveKeysForClass(PyHeapTypeObject *cls)
     _PyDictKeys_DecRef(cls->ht_cached_keys);
 }
 
-void
-_PyDict_SplitKeysInvalidated(PyDictKeysObject* keys)
-{
-    struct _instancekeysobject *shared_keys = _PyDictKeys_AsSharedKeys(keys);
-    PyTypeObject *type = FT_ATOMIC_LOAD_PTR_ACQUIRE(shared_keys->dsk_owning_type);
-    if (type) {
-        _PyType_Modified_Unlocked(type);
-    }
-}
-
 void
 _PyObject_InitInlineValues(PyObject *obj, PyTypeObject *tp)
 {

Python/specialize.c

@@ -980,12 +980,15 @@ static int
 specialize_instance_load_attr(PyObject* owner, _Py_CODEUNIT* instr, PyObject* name)
 {
     // 0 is not a valid version
-    uint32_t shared_keys_version = 0;
-    bool shadow = instance_has_key(owner, name, &shared_keys_version);
     PyObject *descr = NULL;
     unsigned int tp_version = 0;
     PyTypeObject *type = Py_TYPE(owner);
+    // Read the type version before the keys version, we could have a concurrent
+    // modification of the split keys in which case we update the keys version and
+    // then the type version, this ensures we will still deopt if that happens.
     DescriptorClassification kind = analyze_descriptor_load(type, name, &descr, &tp_version);
+    uint32_t shared_keys_version = 0;
+    bool shadow = instance_has_key(owner, name, &shared_keys_version);
     int result = do_specialize_instance_load_attr(owner, instr, name, shadow, shared_keys_version, kind, descr, tp_version);
     Py_XDECREF(descr);
     return result;