From 7a6ed9f446714e7a82451ca939b338a509d629e7 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Wed, 19 Jun 2024 11:02:16 +0100 Subject: [PATCH 1/6] Maintain type version invariants. --- Lib/test/test_type_cache.py | 14 ++++++----- Modules/_testinternalcapi.c | 1 - Objects/typeobject.c | 50 ++++++++++++++++++++++++++----------- 3 files changed, 44 insertions(+), 21 deletions(-) diff --git a/Lib/test/test_type_cache.py b/Lib/test/test_type_cache.py index edaf076707ad8b..5c7d45ab87a473 100644 --- a/Lib/test/test_type_cache.py +++ b/Lib/test/test_type_cache.py @@ -106,8 +106,10 @@ def _assign_valid_version_or_skip(self, type_): if type_get_version(type_) == 0: self.skipTest("Could not assign valid type version") - def _assign_and_check_version_0(self, user_type): + def _no_more_versions(self, user_type): type_modified(user_type) + for _ in range(1001): + type_assign_specific_version_unsafe(user_type, 1000_000_000) type_assign_specific_version_unsafe(user_type, 0) self.assertEqual(type_get_version(user_type), 0) @@ -136,7 +138,7 @@ def load_foo_1(type_): self._check_specialization(load_foo_1, A, "LOAD_ATTR", should_specialize=True) del load_foo_1 - self._assign_and_check_version_0(A) + self._no_more_versions(A) def load_foo_2(type_): return type_.foo @@ -187,7 +189,7 @@ def load_x_1(instance): self._check_specialization(load_x_1, G(), "LOAD_ATTR", should_specialize=True) del load_x_1 - self._assign_and_check_version_0(G) + self._no_more_versions(G) def load_x_2(instance): instance.x @@ -206,7 +208,7 @@ def store_bar_1(type_): self._check_specialization(store_bar_1, B(), "STORE_ATTR", should_specialize=True) del store_bar_1 - self._assign_and_check_version_0(B) + self._no_more_versions(B) def store_bar_2(type_): type_.bar = 10 @@ -226,7 +228,7 @@ def call_class_1(type_): self._check_specialization(call_class_1, F, "CALL", should_specialize=True) del call_class_1 - self._assign_and_check_version_0(F) + self._no_more_versions(F) def call_class_2(type_): type_() @@ -245,7 +247,7 @@ def to_bool_1(instance): self._check_specialization(to_bool_1, H(), "TO_BOOL", should_specialize=True) del to_bool_1 - self._assign_and_check_version_0(H) + self._no_more_versions(H) def to_bool_2(instance): not instance diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index 139a0509795de9..3dd40a66abc1a4 100644 --- a/Modules/_testinternalcapi.c +++ b/Modules/_testinternalcapi.c @@ -2014,7 +2014,6 @@ type_assign_specific_version_unsafe(PyObject *self, PyObject *args) } assert(!PyType_HasFeature(type, Py_TPFLAGS_IMMUTABLETYPE)); _PyType_SetVersion(type, version); - type->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG; Py_RETURN_NONE; } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 0dcf1d399d91be..4ec52004789ad0 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -936,6 +936,19 @@ PyType_ClearWatcher(int watcher_id) return 0; } +#ifndef NDEBUG +/* Returns true if tp_flags and tp_version_tag are consistent */ +static bool version_tag_consistency(PyTypeObject *type) +{ + if (_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { + return type->tp_version_tag != 0; + } + else { + return type->tp_version_tag == 0; + } +} +#endif + static int assign_version_tag(PyInterpreterState *interp, PyTypeObject *type); int @@ -952,6 +965,7 @@ PyType_Watch(int watcher_id, PyObject* obj) } // ensure we will get a callback on the next modification BEGIN_TYPE_LOCK() + assert(version_tag_consistency(type)); assign_version_tag(interp, type); type->tp_watched |= (1 << watcher_id); END_TYPE_LOCK() @@ -1070,7 +1084,6 @@ type_modified_unlocked(PyTypeObject *type) } } - type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; _PyType_SetVersion(type, 0); /* 0 is not a valid version tag */ if (PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { // This field *must* be invalidated if the type is modified (see the @@ -1082,6 +1095,7 @@ type_modified_unlocked(PyTypeObject *type) void PyType_Modified(PyTypeObject *type) { + assert(version_tag_consistency(type)); // Quick check without the lock held if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { return; @@ -1147,7 +1161,6 @@ type_mro_modified(PyTypeObject *type, PyObject *bases) { clear: assert(!(type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN)); - type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; _PyType_SetVersion(type, 0); /* 0 is not a valid version tag */ if (PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { // This field *must* be invalidated if the type is modified (see the @@ -1168,6 +1181,7 @@ This is similar to func_version_cache. void _PyType_SetVersion(PyTypeObject *tp, unsigned int version) { + assert(version_tag_consistency(tp)); #ifndef Py_GIL_DISABLED PyInterpreterState *interp = _PyInterpreterState_GET(); // lookup the old version and set to null @@ -1178,6 +1192,13 @@ _PyType_SetVersion(PyTypeObject *tp, unsigned int version) *slot = NULL; } #endif + if (version) { + tp->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG; + tp->tp_versions_used++; + } + else { + tp->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; + } FT_ATOMIC_STORE_UINT32_RELAXED(tp->tp_version_tag, version); #ifndef Py_GIL_DISABLED if (version != 0) { @@ -1219,6 +1240,7 @@ _PyType_GetVersionForCurrentState(PyTypeObject *tp) static int assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) { + assert(version_tag_consistency(type)); ASSERT_TYPE_LOCK_HELD(); /* Ensure that the tp_version_tag is valid and set @@ -1235,7 +1257,14 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) if (type->tp_versions_used >= MAX_VERSIONS_PER_CLASS) { return 0; } - type->tp_versions_used++; + + PyObject *bases = lookup_tp_bases(type); + Py_ssize_t n = PyTuple_GET_SIZE(bases); + for (Py_ssize_t i = 0; i < n; i++) { + PyObject *b = PyTuple_GET_ITEM(bases, i); + if (!assign_version_tag(interp, _PyType_CAST(b))) + return 0; + } if (type->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) { /* static types */ if (NEXT_GLOBAL_VERSION_TAG > _Py_MAX_GLOBAL_TYPE_VERSION_TAG) { @@ -1254,15 +1283,7 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) _PyType_SetVersion(type, NEXT_VERSION_TAG(interp)++); assert (type->tp_version_tag != 0); } - - PyObject *bases = lookup_tp_bases(type); - Py_ssize_t n = PyTuple_GET_SIZE(bases); - for (Py_ssize_t i = 0; i < n; i++) { - PyObject *b = PyTuple_GET_ITEM(bases, i); - if (!assign_version_tag(interp, _PyType_CAST(b))) - return 0; - } - type->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG; + assert(version_tag_consistency(type)); return 1; } @@ -3284,6 +3305,7 @@ static int mro_internal_unlocked(PyTypeObject *type, int initial, PyObject **p_old_mro) { ASSERT_TYPE_LOCK_HELD(); + assert(version_tag_consistency(type)); PyObject *new_mro, *old_mro; int reent; @@ -5432,6 +5454,7 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name) unsigned int h = MCACHE_HASH_METHOD(type, name); struct type_cache *cache = get_type_cache(); struct type_cache_entry *entry = &cache->hashtable[h]; + assert(version_tag_consistency(type)); #ifdef Py_GIL_DISABLED // synchronize-with other writing threads by doing an acquire load on the sequence while (1) { @@ -5898,7 +5921,6 @@ fini_static_type(PyInterpreterState *interp, PyTypeObject *type, if (final) { type->tp_flags &= ~Py_TPFLAGS_READY; - type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; _PyType_SetVersion(type, 0); } @@ -8516,11 +8538,11 @@ init_static_type(PyInterpreterState *interp, PyTypeObject *self, assert(NEXT_GLOBAL_VERSION_TAG <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG); _PyType_SetVersion(self, NEXT_GLOBAL_VERSION_TAG++); - self->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG; } else { assert(!initial); assert(self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN); + assert(self->tp_version_tag != 0); assert(self->tp_flags & Py_TPFLAGS_VALID_VERSION_TAG); } From ea9dd376e958bda80d1cfdfd1245dc9479b38fb0 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Wed, 19 Jun 2024 11:07:30 +0100 Subject: [PATCH 2/6] Add test for #119462 --- Lib/test/test_type_cache.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Lib/test/test_type_cache.py b/Lib/test/test_type_cache.py index 5c7d45ab87a473..89632a3abebfb5 100644 --- a/Lib/test/test_type_cache.py +++ b/Lib/test/test_type_cache.py @@ -93,6 +93,21 @@ class C: new_version = type_get_version(C) self.assertEqual(new_version, 0) + def test_119462(self): + + class Holder: + value = None + + @classmethod + def set_value(cls): + cls.value = object() + + class HolderSub(Holder): + pass + + for _ in range(1050): + Holder.set_value() + HolderSub.value @support.cpython_only @requires_specialization From 86b57e1782a7309740963a0fa7ba00f928919960 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Wed, 19 Jun 2024 11:11:03 +0100 Subject: [PATCH 3/6] Add news --- .../2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst new file mode 100644 index 00000000000000..499752a6e2dc75 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst @@ -0,0 +1,4 @@ +Make sure that invariants of type versioning are maintained: * Superclasses +always have their version number assigned before subclasses * The +Py_TPFLAGS_VALID_VERSION_TAG is always set if the version tag is non-zero * +The Py_TPFLAGS_VALID_VERSION_TAG is always unset if the version tag is zero From b977a978de5dc2ab4b43d6d7ee849d4b4674b841 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Wed, 19 Jun 2024 12:54:51 +0100 Subject: [PATCH 4/6] Sort out free-threading --- Objects/typeobject.c | 112 +++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 74 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 4ec52004789ad0..3ac67f427812fa 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -988,43 +988,38 @@ PyType_Unwatch(int watcher_id, PyObject* obj) return 0; } -#ifdef Py_GIL_DISABLED - static void -type_modification_starting_unlocked(PyTypeObject *type) +set_version_unlocked(PyTypeObject *tp, unsigned int version) { ASSERT_TYPE_LOCK_HELD(); - - /* Clear version tags on all types, but leave the valid - version tag intact. This prepares for a modification so that - any concurrent readers of the type cache will not see invalid - values. - */ - if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { - return; + assert(version_tag_consistency(tp)); +#ifndef Py_GIL_DISABLED + PyInterpreterState *interp = _PyInterpreterState_GET(); + // lookup the old version and set to null + if (tp->tp_version_tag != 0) { + PyTypeObject **slot = + interp->types.type_version_cache + + (tp->tp_version_tag % TYPE_VERSION_CACHE_SIZE); + *slot = NULL; } - - PyObject *subclasses = lookup_tp_subclasses(type); - if (subclasses != NULL) { - assert(PyDict_CheckExact(subclasses)); - - Py_ssize_t i = 0; - PyObject *ref; - while (PyDict_Next(subclasses, &i, NULL, &ref)) { - PyTypeObject *subclass = type_from_ref(ref); - if (subclass == NULL) { - continue; - } - type_modification_starting_unlocked(subclass); - Py_DECREF(subclass); - } +#endif + if (version) { + tp->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG; + tp->tp_versions_used++; + } + else { + tp->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; + } + tp->tp_version_tag = version; +#ifndef Py_GIL_DISABLED + if (version != 0) { + PyTypeObject **slot = + interp->types.type_version_cache + + (version % TYPE_VERSION_CACHE_SIZE); + *slot = tp; } - - /* 0 is not a valid version tag */ - _PyType_SetVersion(type, 0); -} - #endif +} static void type_modified_unlocked(PyTypeObject *type) @@ -1084,7 +1079,7 @@ type_modified_unlocked(PyTypeObject *type) } } - _PyType_SetVersion(type, 0); /* 0 is not a valid version tag */ + set_version_unlocked(type, 0); /* 0 is not a valid version tag */ if (PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { // This field *must* be invalidated if the type is modified (see the // comment on struct _specialization_cache): @@ -1095,13 +1090,13 @@ type_modified_unlocked(PyTypeObject *type) void PyType_Modified(PyTypeObject *type) { - assert(version_tag_consistency(type)); // Quick check without the lock held if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { return; } BEGIN_TYPE_LOCK() + assert(version_tag_consistency(type)); type_modified_unlocked(type); END_TYPE_LOCK() } @@ -1161,7 +1156,7 @@ type_mro_modified(PyTypeObject *type, PyObject *bases) { clear: assert(!(type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN)); - _PyType_SetVersion(type, 0); /* 0 is not a valid version tag */ + set_version_unlocked(type, 0); /* 0 is not a valid version tag */ if (PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { // This field *must* be invalidated if the type is modified (see the // comment on struct _specialization_cache): @@ -1181,33 +1176,10 @@ This is similar to func_version_cache. void _PyType_SetVersion(PyTypeObject *tp, unsigned int version) { - assert(version_tag_consistency(tp)); -#ifndef Py_GIL_DISABLED - PyInterpreterState *interp = _PyInterpreterState_GET(); - // lookup the old version and set to null - if (tp->tp_version_tag != 0) { - PyTypeObject **slot = - interp->types.type_version_cache - + (tp->tp_version_tag % TYPE_VERSION_CACHE_SIZE); - *slot = NULL; - } -#endif - if (version) { - tp->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG; - tp->tp_versions_used++; - } - else { - tp->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; - } - FT_ATOMIC_STORE_UINT32_RELAXED(tp->tp_version_tag, version); -#ifndef Py_GIL_DISABLED - if (version != 0) { - PyTypeObject **slot = - interp->types.type_version_cache - + (version % TYPE_VERSION_CACHE_SIZE); - *slot = tp; - } -#endif + + BEGIN_TYPE_LOCK() + set_version_unlocked(tp, version); + END_TYPE_LOCK() } PyTypeObject * @@ -1271,7 +1243,7 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) /* We have run out of version numbers */ return 0; } - _PyType_SetVersion(type, NEXT_GLOBAL_VERSION_TAG++); + set_version_unlocked(type, NEXT_GLOBAL_VERSION_TAG++); assert (type->tp_version_tag <= _Py_MAX_GLOBAL_TYPE_VERSION_TAG); } else { @@ -1280,7 +1252,7 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) /* We have run out of version numbers */ return 0; } - _PyType_SetVersion(type, NEXT_VERSION_TAG(interp)++); + set_version_unlocked(type, NEXT_VERSION_TAG(interp)++); assert (type->tp_version_tag != 0); } assert(version_tag_consistency(type)); @@ -5454,7 +5426,6 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name) unsigned int h = MCACHE_HASH_METHOD(type, name); struct type_cache *cache = get_type_cache(); struct type_cache_entry *entry = &cache->hashtable[h]; - assert(version_tag_consistency(type)); #ifdef Py_GIL_DISABLED // synchronize-with other writing threads by doing an acquire load on the sequence while (1) { @@ -5484,6 +5455,7 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name) } } #else + assert(version_tag_consistency(type)); if (entry->version == type->tp_version_tag && entry->name == name) { assert(_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)); @@ -5793,16 +5765,6 @@ type_setattro(PyObject *self, PyObject *name, PyObject *value) return -1; } -#ifdef Py_GIL_DISABLED - // In free-threaded builds readers can race with the lock-free portion - // of the type cache and the assignment into the dict. We clear all of the - // versions initially so no readers will succeed in the lock-free case. - // They'll then block on the type lock until the update below is done. - type_modification_starting_unlocked(type); -#endif - - res = _PyDict_SetItem_LockHeld((PyDictObject *)dict, name, value); - /* Clear the VALID_VERSION flag of 'type' and all its subclasses. This could possibly be unified with the update_subclasses() recursion in update_slot(), but carefully: @@ -5810,6 +5772,8 @@ type_setattro(PyObject *self, PyObject *name, PyObject *value) recursing into subclasses. */ type_modified_unlocked(type); + res = _PyDict_SetItem_LockHeld((PyDictObject *)dict, name, value); + if (res == 0) { if (is_dunder_name(name)) { res = update_slot(type, name); From 54407ab938cdd404bf85cae7bde82af685d1a44c Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Wed, 19 Jun 2024 15:40:21 +0100 Subject: [PATCH 5/6] Remove uses of Py_TPFLAGS_VALID_VERSION_TAG and use atomic writes for version tag. --- Doc/c-api/typeobj.rst | 4 +- Include/object.h | 2 +- ...-06-19-11-10-50.gh-issue-119462.DpcqSe.rst | 8 +-- Modules/pyexpat.c | 2 +- Objects/typeobject.c | 57 ++++++------------- 5 files changed, 25 insertions(+), 48 deletions(-) diff --git a/Doc/c-api/typeobj.rst b/Doc/c-api/typeobj.rst index a6a2c437ea4e16..c9ef076c78c66a 100644 --- a/Doc/c-api/typeobj.rst +++ b/Doc/c-api/typeobj.rst @@ -1328,8 +1328,8 @@ and :c:data:`PyType_Type` effectively act as defaults.) To indicate that a class has changed call :c:func:`PyType_Modified` .. warning:: - This flag is present in header files, but is an internal feature and should - not be used. It will be removed in a future version of CPython + This flag is present in header files, but is not be used. + It will be removed in a future version of CPython .. c:member:: const char* PyTypeObject.tp_doc diff --git a/Include/object.h b/Include/object.h index 48f97502ad19f1..98d8bb0b6aa771 100644 --- a/Include/object.h +++ b/Include/object.h @@ -556,7 +556,7 @@ given type object has a specified feature. /* Objects behave like an unbound method */ #define Py_TPFLAGS_METHOD_DESCRIPTOR (1UL << 17) -/* Object has up-to-date type attribute cache */ +/* Unused. Legacy flag */ #define Py_TPFLAGS_VALID_VERSION_TAG (1UL << 19) /* Type is abstract and cannot be instantiated */ diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst index 499752a6e2dc75..7a3b74b63b2e40 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2024-06-19-11-10-50.gh-issue-119462.DpcqSe.rst @@ -1,4 +1,4 @@ -Make sure that invariants of type versioning are maintained: * Superclasses -always have their version number assigned before subclasses * The -Py_TPFLAGS_VALID_VERSION_TAG is always set if the version tag is non-zero * -The Py_TPFLAGS_VALID_VERSION_TAG is always unset if the version tag is zero +Make sure that invariants of type versioning are maintained: +* Superclasses always have their version number assigned before subclasses +* The version tag is always zero if the tag is not valid. +* The version tag is always non-if the tag is valid. diff --git a/Modules/pyexpat.c b/Modules/pyexpat.c index 8495fe2dd4dd2b..9733bc34f7c80a 100644 --- a/Modules/pyexpat.c +++ b/Modules/pyexpat.c @@ -1651,7 +1651,7 @@ PyDoc_STRVAR(pyexpat_module_documentation, static int init_handler_descrs(pyexpat_state *state) { int i; - assert(!PyType_HasFeature(state->xml_parse_type, Py_TPFLAGS_VALID_VERSION_TAG)); + assert(state->xml_parse_type->tp_version_tag == 0); for (i = 0; handler_info[i].name != NULL; i++) { struct HandlerInfo *hi = &handler_info[i]; hi->getset.name = hi->name; diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 3ac67f427812fa..062189e24eb10c 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -936,19 +936,6 @@ PyType_ClearWatcher(int watcher_id) return 0; } -#ifndef NDEBUG -/* Returns true if tp_flags and tp_version_tag are consistent */ -static bool version_tag_consistency(PyTypeObject *type) -{ - if (_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { - return type->tp_version_tag != 0; - } - else { - return type->tp_version_tag == 0; - } -} -#endif - static int assign_version_tag(PyInterpreterState *interp, PyTypeObject *type); int @@ -965,7 +952,6 @@ PyType_Watch(int watcher_id, PyObject* obj) } // ensure we will get a callback on the next modification BEGIN_TYPE_LOCK() - assert(version_tag_consistency(type)); assign_version_tag(interp, type); type->tp_watched |= (1 << watcher_id); END_TYPE_LOCK() @@ -992,7 +978,6 @@ static void set_version_unlocked(PyTypeObject *tp, unsigned int version) { ASSERT_TYPE_LOCK_HELD(); - assert(version_tag_consistency(tp)); #ifndef Py_GIL_DISABLED PyInterpreterState *interp = _PyInterpreterState_GET(); // lookup the old version and set to null @@ -1002,15 +987,15 @@ set_version_unlocked(PyTypeObject *tp, unsigned int version) + (tp->tp_version_tag % TYPE_VERSION_CACHE_SIZE); *slot = NULL; } -#endif if (version) { - tp->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG; - tp->tp_versions_used++; + _Py_atomic_add_uint16(&tp->tp_versions_used, 1); } - else { - tp->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG; +#else + if (version) { + tp->tp_versions_used++; } - tp->tp_version_tag = version; +#endif + FT_ATOMIC_STORE_UINT32_RELAXED(tp->tp_version_tag, version); #ifndef Py_GIL_DISABLED if (version != 0) { PyTypeObject **slot = @@ -1030,16 +1015,16 @@ type_modified_unlocked(PyTypeObject *type) Invariants: - - before Py_TPFLAGS_VALID_VERSION_TAG can be set on a type, + - before tp_version_tag can be set on a type, it must first be set on all super types. - This function clears the Py_TPFLAGS_VALID_VERSION_TAG of a + This function clears the tp_version_tag of a type (so it must first clear it on all subclasses). The - tp_version_tag value is meaningless unless this flag is set. + tp_version_tag value is meaningless when equal to zero. We don't assign new version tags eagerly, but only as needed. */ - if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { + if (type->tp_version_tag == 0) { return; } @@ -1091,12 +1076,11 @@ void PyType_Modified(PyTypeObject *type) { // Quick check without the lock held - if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { + if (type->tp_version_tag == 0) { return; } BEGIN_TYPE_LOCK() - assert(version_tag_consistency(type)); type_modified_unlocked(type); END_TYPE_LOCK() } @@ -1212,15 +1196,13 @@ _PyType_GetVersionForCurrentState(PyTypeObject *tp) static int assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) { - assert(version_tag_consistency(type)); ASSERT_TYPE_LOCK_HELD(); - /* Ensure that the tp_version_tag is valid and set - Py_TPFLAGS_VALID_VERSION_TAG. To respect the invariant, this - must first be done on all super classes. Return 0 if this - cannot be done, 1 if Py_TPFLAGS_VALID_VERSION_TAG. + /* Ensure that the tp_version_tag is valid. + * To respect the invariant, this must first be done on all super classes. + * Return 0 if this cannot be done, 1 if tp_version_tag is set. */ - if (_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { + if (type->tp_version_tag != 0) { return 1; } if (!_PyType_HasFeature(type, Py_TPFLAGS_READY)) { @@ -1255,7 +1237,6 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) set_version_unlocked(type, NEXT_VERSION_TAG(interp)++); assert (type->tp_version_tag != 0); } - assert(version_tag_consistency(type)); return 1; } @@ -3277,7 +3258,6 @@ static int mro_internal_unlocked(PyTypeObject *type, int initial, PyObject **p_old_mro) { ASSERT_TYPE_LOCK_HELD(); - assert(version_tag_consistency(type)); PyObject *new_mro, *old_mro; int reent; @@ -3312,7 +3292,7 @@ mro_internal_unlocked(PyTypeObject *type, int initial, PyObject **p_old_mro) else { /* For static builtin types, this is only called during init before the method cache has been populated. */ - assert(_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)); + assert(type->tp_version_tag); } if (p_old_mro != NULL) @@ -5455,10 +5435,9 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name) } } #else - assert(version_tag_consistency(type)); if (entry->version == type->tp_version_tag && entry->name == name) { - assert(_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)); + assert(type->tp_version_tag); OBJECT_STAT_INC_COND(type_cache_hits, !is_dunder_name(name)); OBJECT_STAT_INC_COND(type_cache_dunder_hits, is_dunder_name(name)); Py_XINCREF(entry->value); @@ -5481,7 +5460,6 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name) if (MCACHE_CACHEABLE_NAME(name)) { has_version = assign_version_tag(interp, type); version = type->tp_version_tag; - assert(!has_version || _PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)); } END_TYPE_LOCK() @@ -8507,7 +8485,6 @@ init_static_type(PyInterpreterState *interp, PyTypeObject *self, assert(!initial); assert(self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN); assert(self->tp_version_tag != 0); - assert(self->tp_flags & Py_TPFLAGS_VALID_VERSION_TAG); } managed_static_type_state_init(interp, self, isbuiltin, initial); From 917beca3151116c7550d1278bfd3ef6522b87875 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Wed, 19 Jun 2024 16:44:02 +0100 Subject: [PATCH 6/6] Address review comments --- Objects/typeobject.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 062189e24eb10c..1cc6ca79298108 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -988,11 +988,11 @@ set_version_unlocked(PyTypeObject *tp, unsigned int version) *slot = NULL; } if (version) { - _Py_atomic_add_uint16(&tp->tp_versions_used, 1); + tp->tp_versions_used++; } #else if (version) { - tp->tp_versions_used++; + _Py_atomic_add_uint16(&tp->tp_versions_used, 1); } #endif FT_ATOMIC_STORE_UINT32_RELAXED(tp->tp_version_tag, version); @@ -1216,8 +1216,9 @@ assign_version_tag(PyInterpreterState *interp, PyTypeObject *type) Py_ssize_t n = PyTuple_GET_SIZE(bases); for (Py_ssize_t i = 0; i < n; i++) { PyObject *b = PyTuple_GET_ITEM(bases, i); - if (!assign_version_tag(interp, _PyType_CAST(b))) + if (!assign_version_tag(interp, _PyType_CAST(b))) { return 0; + } } if (type->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) { /* static types */