Skip to content

Commit cd52172

Browse files
authored
gh-145685: Improve scaling of type attribute lookups (gh-145774)
Avoid locking in the PyType_Lookup cache-miss path if the type's tp_version_tag is already valid.
1 parent 7a65900 commit cd52172

File tree

3 files changed

+35
-24
lines changed

3 files changed

+35
-24
lines changed

Include/internal/pycore_pyatomic_ft_wrappers.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,8 @@ extern "C" {
9595
_Py_atomic_store_int_relaxed(&value, new_value)
9696
#define FT_ATOMIC_LOAD_INT_RELAXED(value) \
9797
_Py_atomic_load_int_relaxed(&value)
98+
#define FT_ATOMIC_LOAD_UINT(value) \
99+
_Py_atomic_load_uint(&value)
98100
#define FT_ATOMIC_STORE_UINT_RELAXED(value, new_value) \
99101
_Py_atomic_store_uint_relaxed(&value, new_value)
100102
#define FT_ATOMIC_LOAD_UINT_RELAXED(value) \
@@ -167,6 +169,7 @@ extern "C" {
167169
#define FT_ATOMIC_STORE_INT(value, new_value) value = new_value
168170
#define FT_ATOMIC_LOAD_INT_RELAXED(value) value
169171
#define FT_ATOMIC_STORE_INT_RELAXED(value, new_value) value = new_value
172+
#define FT_ATOMIC_LOAD_UINT(value) value
170173
#define FT_ATOMIC_LOAD_UINT_RELAXED(value) value
171174
#define FT_ATOMIC_STORE_UINT_RELAXED(value, new_value) value = new_value
172175
#define FT_ATOMIC_LOAD_LONG_RELAXED(value) value
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Improve scaling of type attribute lookups in the :term:`free-threaded build` by
2+
avoiding contention on the internal type lock.

Objects/typeobject.c

Lines changed: 30 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,8 @@ class object "PyObject *" "&PyBaseObject_Type"
5252
MCACHE_HASH(FT_ATOMIC_LOAD_UINT_RELAXED((type)->tp_version_tag), \
5353
((Py_ssize_t)(name)) >> 3)
5454
#define MCACHE_CACHEABLE_NAME(name) \
55-
PyUnicode_CheckExact(name) && \
56-
(PyUnicode_GET_LENGTH(name) <= MCACHE_MAX_ATTR_SIZE)
55+
(PyUnicode_CheckExact(name) && \
56+
(PyUnicode_GET_LENGTH(name) <= MCACHE_MAX_ATTR_SIZE))
5757

5858
#define NEXT_VERSION_TAG(interp) \
5959
(interp)->types.next_version_tag
@@ -6134,6 +6134,14 @@ _PyType_LookupRefAndVersion(PyTypeObject *type, PyObject *name, unsigned int *ve
61346134
return PyStackRef_AsPyObjectSteal(out);
61356135
}
61366136

6137+
static int
6138+
should_assign_version_tag(PyTypeObject *type, PyObject *name, unsigned int version_tag)
6139+
{
6140+
return (version_tag == 0
6141+
&& FT_ATOMIC_LOAD_UINT16_RELAXED(type->tp_versions_used) < MAX_VERSIONS_PER_CLASS
6142+
&& MCACHE_CACHEABLE_NAME(name));
6143+
}
6144+
61376145
unsigned int
61386146
_PyType_LookupStackRefAndVersion(PyTypeObject *type, PyObject *name, _PyStackRef *out)
61396147
{
@@ -6182,41 +6190,39 @@ _PyType_LookupStackRefAndVersion(PyTypeObject *type, PyObject *name, _PyStackRef
61826190
/* We may end up clearing live exceptions below, so make sure it's ours. */
61836191
assert(!PyErr_Occurred());
61846192

6185-
// We need to atomically do the lookup and capture the version before
6186-
// anyone else can modify our mro or mutate the type.
6187-
61886193
int res;
61896194
PyInterpreterState *interp = _PyInterpreterState_GET();
6190-
int has_version = 0;
6191-
unsigned int assigned_version = 0;
61926195

6193-
BEGIN_TYPE_LOCK();
6194-
// We must assign the version before doing the lookup. If
6195-
// find_name_in_mro() blocks and releases the critical section
6196-
// then the type version can change.
6197-
if (MCACHE_CACHEABLE_NAME(name)) {
6198-
has_version = assign_version_tag(interp, type);
6199-
assigned_version = type->tp_version_tag;
6200-
}
6201-
res = find_name_in_mro(type, name, out);
6202-
END_TYPE_LOCK();
6196+
unsigned int version_tag = FT_ATOMIC_LOAD_UINT(type->tp_version_tag);
6197+
if (should_assign_version_tag(type, name, version_tag)) {
6198+
BEGIN_TYPE_LOCK();
6199+
assign_version_tag(interp, type);
6200+
version_tag = type->tp_version_tag;
6201+
res = find_name_in_mro(type, name, out);
6202+
END_TYPE_LOCK();
6203+
}
6204+
else {
6205+
res = find_name_in_mro(type, name, out);
6206+
}
62036207

62046208
/* Only put NULL results into cache if there was no error. */
62056209
if (res < 0) {
62066210
*out = PyStackRef_NULL;
62076211
return 0;
62086212
}
62096213

6210-
if (has_version) {
6211-
PyObject *res_obj = PyStackRef_AsPyObjectBorrow(*out);
6214+
if (version_tag == 0 || !MCACHE_CACHEABLE_NAME(name)) {
6215+
return 0;
6216+
}
6217+
6218+
PyObject *res_obj = PyStackRef_AsPyObjectBorrow(*out);
62126219
#if Py_GIL_DISABLED
6213-
update_cache_gil_disabled(entry, name, assigned_version, res_obj);
6220+
update_cache_gil_disabled(entry, name, version_tag, res_obj);
62146221
#else
6215-
PyObject *old_value = update_cache(entry, name, assigned_version, res_obj);
6216-
Py_DECREF(old_value);
6222+
PyObject *old_value = update_cache(entry, name, version_tag, res_obj);
6223+
Py_DECREF(old_value);
62176224
#endif
6218-
}
6219-
return has_version ? assigned_version : 0;
6225+
return version_tag;
62206226
}
62216227

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

0 commit comments

Comments
 (0)