Skip to content

Commit db6d52e

Browse files
committed
GH-132380: Avoid locking in type lookup.
If the type lookup cache (mcache) is missed, do not acquire TYPE_LOCK unless we need to assign a type version. We actually don't need to lock in order to do the MRO attribute lookup.
1 parent 1ffe913 commit db6d52e

2 files changed

Lines changed: 37 additions & 14 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
When doing a type attribute lookup, do not acquire ``TYPE_LOCK`` unless we need
2+
to assign a new type version. While doing the MRO lookup, we don't need to
3+
hold the lock. This reduces lock contention, expecially when looking up
4+
attributes that are not in the type lookup cache.

Objects/typeobject.c

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5880,6 +5880,26 @@ PyObject_GetItemData(PyObject *obj)
58805880
return (char *)obj + Py_TYPE(obj)->tp_basicsize;
58815881
}
58825882

5883+
// Try to assign a new type version tag, return it if successful. Return 0
5884+
// if no version was assigned.
5885+
static unsigned int
5886+
type_assign_version(PyTypeObject *type)
5887+
{
5888+
unsigned int version = FT_ATOMIC_LOAD_UINT32_RELAXED((type)->tp_version_tag);
5889+
if (version == 0) {
5890+
BEGIN_TYPE_LOCK();
5891+
PyInterpreterState *interp = _PyInterpreterState_GET();
5892+
if (assign_version_tag(interp, type)) {
5893+
version = type->tp_version_tag;
5894+
}
5895+
else {
5896+
version = 0;
5897+
}
5898+
END_TYPE_LOCK();
5899+
}
5900+
return version;
5901+
}
5902+
58835903
/* Internal API to look for a name through the MRO, bypassing the method cache.
58845904
This returns a borrowed reference, and might set an exception.
58855905
'error' is set to: -1: error with exception; 1: error without exception; 0: ok */
@@ -5892,15 +5912,17 @@ find_name_in_mro(PyTypeObject *type, PyObject *name, int *error)
58925912
return NULL;
58935913
}
58945914

5895-
/* Look in tp_dict of types in MRO */
5896-
PyObject *mro = lookup_tp_mro(type);
5915+
/* Look in tp_dict of types in MRO. Keep a strong reference to mro
5916+
because type->tp_mro can be replaced during dict lookup, e.g. when
5917+
comparing to non-string keys. */
5918+
PyObject *mro = _PyType_GetMRO(type);
58975919
if (mro == NULL) {
58985920
if (!is_readying(type)) {
58995921
if (PyType_Ready(type) < 0) {
59005922
*error = -1;
59015923
return NULL;
59025924
}
5903-
mro = lookup_tp_mro(type);
5925+
mro = _PyType_GetMRO(type);
59045926
}
59055927
if (mro == NULL) {
59065928
*error = 1;
@@ -5909,9 +5931,6 @@ find_name_in_mro(PyTypeObject *type, PyObject *name, int *error)
59095931
}
59105932

59115933
PyObject *res = NULL;
5912-
/* Keep a strong reference to mro because type->tp_mro can be replaced
5913-
during dict lookup, e.g. when comparing to non-string keys. */
5914-
Py_INCREF(mro);
59155934
Py_ssize_t n = PyTuple_GET_SIZE(mro);
59165935
for (Py_ssize_t i = 0; i < n; i++) {
59175936
PyObject *base = PyTuple_GET_ITEM(mro, i);
@@ -6084,19 +6103,19 @@ _PyType_LookupStackRefAndVersion(PyTypeObject *type, PyObject *name, _PyStackRef
60846103

60856104
PyObject *res;
60866105
int error;
6087-
PyInterpreterState *interp = _PyInterpreterState_GET();
6088-
int has_version = 0;
60896106
unsigned int assigned_version = 0;
6090-
BEGIN_TYPE_LOCK();
60916107
// We must assign the version before doing the lookup. If
60926108
// find_name_in_mro() blocks and releases the critical section
60936109
// then the type version can change.
60946110
if (MCACHE_CACHEABLE_NAME(name)) {
6095-
has_version = assign_version_tag(interp, type);
6096-
assigned_version = type->tp_version_tag;
6111+
assigned_version = type_assign_version(type);
60976112
}
6113+
// Note that calling find_name_in_mro() might cause the type version to
6114+
// change. For example, if a __hash__ or __eq__ method mutates the types.
6115+
// In that case, 'assigned_version' will be stale after this call. Since
6116+
// that's expected to be rare and does not cause a correctness issue, we
6117+
// don't check for this case.
60986118
res = find_name_in_mro(type, name, &error);
6099-
END_TYPE_LOCK();
61006119

61016120
/* Only put NULL results into cache if there was no error. */
61026121
if (error) {
@@ -6115,7 +6134,7 @@ _PyType_LookupStackRefAndVersion(PyTypeObject *type, PyObject *name, _PyStackRef
61156134
return 0;
61166135
}
61176136

6118-
if (has_version) {
6137+
if (assigned_version != 0) {
61196138
#if Py_GIL_DISABLED
61206139
update_cache_gil_disabled(entry, name, assigned_version, res);
61216140
#else
@@ -6124,7 +6143,7 @@ _PyType_LookupStackRefAndVersion(PyTypeObject *type, PyObject *name, _PyStackRef
61246143
#endif
61256144
}
61266145
*out = res ? PyStackRef_FromPyObjectSteal(res) : PyStackRef_NULL;
6127-
return has_version ? assigned_version : 0;
6146+
return assigned_version;
61286147
}
61296148

61306149
/* Internal API to look for a name through the MRO.

0 commit comments

Comments
 (0)