typeobject: thread safety

 - Make method_cache thread-local
 - Atomics in assign_version_tag
 - Use relaxed atomics for tp_flags

Author: colesbury

Include/internal/pycore_interp.h

@@ -70,6 +70,8 @@ typedef struct PyThreadStateImpl {
     struct brc_state brc;
 
     struct qsbr *qsbr;
+
+    struct type_cache type_cache;
 } PyThreadStateImpl;
 
 

Include/internal/pycore_object.h

@@ -84,7 +84,7 @@ extern int _PyTraceMalloc_NewReference(PyObject *op);
 // Fast inlined version of PyType_HasFeature()
 static inline int
 _PyType_HasFeature(PyTypeObject *type, unsigned long feature) {
-    return ((type->tp_flags & feature) != 0);
+    return PyType_HasFeature(type, feature);
 }
 
 extern void _PyType_InitCache(PyInterpreterState *interp);

Include/internal/pycore_runtime.h

@@ -165,6 +165,7 @@ typedef struct pyruntimestate {
     struct _Py_dict_runtime_state dict_state;
     struct _py_func_runtime_state func_state;
 
+    _PyMutex mutex;
     struct {
         /* Used to set PyTypeObject.tp_version_tag */
         // bpo-42745: next_version_tag remains shared by all interpreters

Include/internal/pycore_typeobject.h

@@ -63,7 +63,9 @@ _PyStaticType_GET_WEAKREFS_LISTPTR(static_builtin_state *state)
 }
 
 struct types_state {
+#ifndef Py_NOGIL
     struct type_cache type_cache;
+#endif
     size_t num_builtins_initialized;
     static_builtin_state builtins[_Py_MAX_STATIC_BUILTIN_TYPES];
 };

Include/object.h

@@ -1083,6 +1083,8 @@ PyType_HasFeature(PyTypeObject *type, unsigned long feature)
 #ifdef Py_LIMITED_API
     // PyTypeObject is opaque in the limited C API
     flags = PyType_GetFlags(type);
+#elif defined(_Py_THREAD_SANITIZER)
+    flags = _Py_atomic_load_ulong_relaxed(&type->tp_flags);
 #else
     flags = type->tp_flags;
 #endif

Objects/typeobject.c

@@ -37,11 +37,17 @@ class object "PyObject *" "&PyBaseObject_Type"
         (((unsigned int)(version) ^ (unsigned int)(name_hash))          \
          & ((1 << MCACHE_SIZE_EXP) - 1))
 
-#define MCACHE_HASH_METHOD(type, name)                                  \
-    MCACHE_HASH((type)->tp_version_tag, ((Py_ssize_t)(name)) >> 3)
+static inline unsigned int
+MCACHE_HASH_METHOD(PyTypeObject *type, PyObject *name)
+{
+    unsigned int version = _Py_atomic_load_uint32_relaxed(&type->tp_version_tag);
+    return MCACHE_HASH(version, ((Py_ssize_t)(name)) >> 3);
+}
+
 #define MCACHE_CACHEABLE_NAME(name)                             \
         PyUnicode_CheckExact(name) &&                           \
         PyUnicode_IS_READY(name) &&                             \
+        PyUnicode_CHECK_INTERNED(name) &&                       \
         (PyUnicode_GET_LENGTH(name) <= MCACHE_MAX_ATTR_SIZE)
 
 #define next_version_tag (_PyRuntime.types.next_version_tag)
@@ -289,66 +295,42 @@ _PyType_GetTextSignatureFromInternalDoc(const char *name, const char *internal_d
 static struct type_cache*
 get_type_cache(void)
 {
-    PyInterpreterState *interp = _PyInterpreterState_GET();
-    return &interp->types.type_cache;
-}
-
-
-static void
-type_cache_clear(struct type_cache *cache, PyObject *value)
-{
-    for (Py_ssize_t i = 0; i < (1 << MCACHE_SIZE_EXP); i++) {
-        struct type_cache_entry *entry = &cache->hashtable[i];
-        entry->version = 0;
-        Py_XSETREF(entry->name, _Py_XNewRef(value));
-        entry->value = NULL;
-    }
+    PyThreadState *tstate = _PyThreadState_GET();
+#ifdef Py_NOGIL
+    return &((PyThreadStateImpl *)tstate)->type_cache;
+#else
+    return &tstate->interp->types.type_cache;
+#endif
 }
 
 
 void
 _PyType_InitCache(PyInterpreterState *interp)
 {
-    struct type_cache *cache = &interp->types.type_cache;
-    for (Py_ssize_t i = 0; i < (1 << MCACHE_SIZE_EXP); i++) {
-        struct type_cache_entry *entry = &cache->hashtable[i];
-        assert(entry->name == NULL);
-
-        entry->version = 0;
-        // Set to None so _PyType_Lookup() can use Py_SETREF(),
-        // rather than using slower Py_XSETREF().
-        entry->name = Py_None;
-        entry->value = NULL;
-    }
 }
 
 
 static unsigned int
-_PyType_ClearCache(PyInterpreterState *interp)
+_PyType_ClearCache(PyThreadStateImpl *tstate)
 {
-    struct type_cache *cache = &interp->types.type_cache;
-    // Set to None, rather than NULL, so _PyType_Lookup() can
-    // use Py_SETREF() rather than using slower Py_XSETREF().
-    type_cache_clear(cache, Py_None);
-
+    memset(&tstate->type_cache, 0, sizeof(tstate->type_cache));
     return next_version_tag - 1;
 }
 
 
 unsigned int
 PyType_ClearCache(void)
 {
-    PyInterpreterState *interp = _PyInterpreterState_GET();
-    return _PyType_ClearCache(interp);
+    // TODO: clear all threads type caches or merge type caches
+    PyThreadState *tstate = _PyThreadState_GET();
+    return _PyType_ClearCache((PyThreadStateImpl *)tstate);
 }
 
 
 void
 _PyTypes_Fini(PyInterpreterState *interp)
 {
-    struct type_cache *cache = &interp->types.type_cache;
-    type_cache_clear(cache, NULL);
-
+    _PyType_ClearCache(&interp->_initial_thread);
     assert(interp->types.num_builtins_initialized == 0);
     // All the static builtin types should have been finalized already.
     for (size_t i = 0; i < _Py_MAX_STATIC_BUILTIN_TYPES; i++) {
@@ -436,8 +418,8 @@ PyType_Unwatch(int watcher_id, PyObject* obj)
     return 0;
 }
 
-void
-PyType_Modified(PyTypeObject *type)
+static void
+_PyType_ModifiedEx(PyTypeObject *type)
 {
     /* Invalidate any cached data for the specified type and all
        subclasses.  This function is called after the base
@@ -469,7 +451,7 @@ PyType_Modified(PyTypeObject *type)
             if (subclass == NULL) {
                 continue;
             }
-            PyType_Modified(subclass);
+            _PyType_ModifiedEx(subclass);
             Py_DECREF(subclass);
         }
     }
@@ -496,6 +478,14 @@ PyType_Modified(PyTypeObject *type)
     type->tp_version_tag = 0; /* 0 is not a valid version tag */
 }
 
+void
+PyType_Modified(PyTypeObject *type)
+{
+    _PyMutex_lock(&_PyRuntime.mutex);
+    _PyType_ModifiedEx(type);
+    _PyMutex_unlock(&_PyRuntime.mutex);
+}
+
 static void
 type_mro_modified(PyTypeObject *type, PyObject *bases) {
     /*
@@ -549,6 +539,23 @@ type_mro_modified(PyTypeObject *type, PyObject *bases) {
     type->tp_version_tag = 0; /* 0 is not a valid version tag */
 }
 
+static unsigned int
+advance_version_tag(void)
+{
+retry: ;
+    unsigned int version_tag = _Py_atomic_load_uint_relaxed(&next_version_tag);
+    if (version_tag == 0) {
+        /* We have run out of version numbers */
+        return 0;
+    }
+    if (!_Py_atomic_compare_exchange_uint(&next_version_tag,
+                                          version_tag,
+                                          version_tag + 1)) {
+        goto retry;
+    }
+    return version_tag;
+}
+
 static int
 assign_version_tag(PyTypeObject *type)
 {
@@ -564,11 +571,13 @@ assign_version_tag(PyTypeObject *type)
         return 0;
     }
 
-    if (next_version_tag == 0) {
+    unsigned int version_tag = advance_version_tag();
+    if (version_tag == 0) {
         /* We have run out of version numbers */
         return 0;
     }
-    type->tp_version_tag = next_version_tag++;
+
+    _Py_atomic_store_uint32_relaxed(&type->tp_version_tag, version_tag);
     assert (type->tp_version_tag != 0);
 
     PyObject *bases = type->tp_bases;
@@ -578,7 +587,8 @@ assign_version_tag(PyTypeObject *type)
         if (!assign_version_tag(_PyType_CAST(b)))
             return 0;
     }
-    type->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG;
+    _Py_atomic_store_ulong_relaxed(
+        &type->tp_flags, type->tp_flags | Py_TPFLAGS_VALID_VERSION_TAG);
     return 1;
 }
 
@@ -4198,9 +4208,10 @@ _PyType_Lookup(PyTypeObject *type, PyObject *name)
         entry->version = type->tp_version_tag;
         entry->value = res;  /* borrowed */
         assert(_PyASCIIObject_CAST(name)->hash != -1);
-        OBJECT_STAT_INC_COND(type_cache_collisions, entry->name != Py_None && entry->name != name);
+        OBJECT_STAT_INC_COND(type_cache_collisions, entry->name != NULL && entry->name != name);
         assert(_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG));
-        Py_SETREF(entry->name, Py_NewRef(name));
+        assert(_PyObject_IS_IMMORTAL(name));
+        entry->name = name;
     }
     return res;
 }
@@ -4374,7 +4385,9 @@ type_setattro(PyTypeObject *type, PyObject *name, PyObject *value)
         PyType_Modified(type);
 
         if (is_dunder_name(name)) {
+            _PyMutex_lock(&_PyRuntime.mutex);
             res = update_slot(type, name);
+            _PyMutex_unlock(&_PyRuntime.mutex);
         }
         assert(_PyType_CheckConsistency(type));
     }
@@ -6992,6 +7005,15 @@ _PyStaticType_InitBuiltin(PyTypeObject *self)
     return res;
 }
 
+static PyObject *
+init_once(void *dst, PyObject *obj)
+{
+    if (!_Py_atomic_compare_exchange_ptr(dst, NULL, obj)) {
+        Py_DECREF(obj);
+        obj = _Py_atomic_load_ptr(dst);
+    }
+    return obj;
+}
 
 static PyObject *
 init_subclasses(PyTypeObject *self)
@@ -7002,11 +7024,9 @@ init_subclasses(PyTypeObject *self)
     }
     if (self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
         static_builtin_state *state = _PyStaticType_GetState(self);
-        state->tp_subclasses = subclasses;
-        return subclasses;
+        return init_once(&state->tp_subclasses, subclasses);
     }
-    self->tp_subclasses = (void *)subclasses;
-    return subclasses;
+    return init_once(&self->tp_subclasses, subclasses);
 }
 
 static void
@@ -9074,13 +9094,16 @@ update_all_slots(PyTypeObject* type)
 {
     pytype_slotdef *p;
 
+    _PyMutex_lock(&_PyRuntime.mutex);
+
     /* Clear the VALID_VERSION flag of 'type' and all its subclasses. */
-    PyType_Modified(type);
+    _PyType_ModifiedEx(type);
 
     for (p = slotdefs; p->name; p++) {
         /* update_slot returns int but can't actually fail */
         update_slot(type, p->name_strobj);
     }
+    _PyMutex_unlock(&_PyRuntime.mutex);
 }