Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-116868: Avoid locking in PyType_IsSubtype #116829

Merged
merged 1 commit into from
Mar 15, 2024
Merged

gh-116868: Avoid locking in PyType_IsSubtype #116829

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 48 additions & 32 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,12 +373,23 @@ lookup_tp_mro(PyTypeObject *self)
PyObject *
_PyType_GetMRO(PyTypeObject *self)
{
PyObject *mro;
#ifdef Py_GIL_DISABLED
PyObject *mro = _Py_atomic_load_ptr_relaxed(&self->tp_mro);
if (mro == NULL) {
return NULL;
}
if (_Py_TryIncref(&self->tp_mro, mro)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there still reference count contention? In other words, will we want to start using some sort of deferred reference count here in the future?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably using "deferred reference count" too broadly here. I'm thinking of something like:

  1. PyType_IsSubtype uses a borrowed reference to self->tp_mro
  2. Modifications to the mro use QSBR to delay decref'ing the old MRO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we're no longer ending up in the parking lot which was the large source of the slowdown, I imagine there is still some amount of contention but it's not enough to obviously show up in gdb.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it under perf it does look like ~8% is spent in _Py_DecRefShared so this probably is still significant and using QSBR probably makes a lot of sense here especially given how little this will ever change.

return mro;
}

BEGIN_TYPE_LOCK();
mro = lookup_tp_mro(self);
Py_INCREF(mro);
Py_XINCREF(mro);
END_TYPE_LOCK()
return mro;
#else
return Py_XNewRef(lookup_tp_mro(self));
#endif
}

static inline void
Expand Down Expand Up @@ -911,7 +922,7 @@ PyType_Modified(PyTypeObject *type)
}

static int
is_subtype_unlocked(PyTypeObject *a, PyTypeObject *b);
is_subtype_with_mro(PyObject *a_mro, PyTypeObject *a, PyTypeObject *b);

static void
type_mro_modified(PyTypeObject *type, PyObject *bases) {
Expand Down Expand Up @@ -957,7 +968,7 @@ type_mro_modified(PyTypeObject *type, PyObject *bases) {
PyObject *b = PyTuple_GET_ITEM(bases, i);
PyTypeObject *cls = _PyType_CAST(b);

if (!is_subtype_unlocked(type, cls)) {
if (!is_subtype_with_mro(lookup_tp_mro(type), type, cls)) {
goto clear;
}
}
Expand Down Expand Up @@ -1442,7 +1453,7 @@ type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, void *context)
}
PyTypeObject *base = (PyTypeObject*)ob;

if (is_subtype_unlocked(base, type) ||
if (is_subtype_with_mro(lookup_tp_mro(base), base, type) ||
/* In case of reentering here again through a custom mro()
the above check is not enough since it relies on
base->tp_mro which would gonna be updated inside
Expand Down Expand Up @@ -2303,37 +2314,41 @@ type_is_subtype_base_chain(PyTypeObject *a, PyTypeObject *b)
}

static int
is_subtype_unlocked(PyTypeObject *a, PyTypeObject *b)
is_subtype_with_mro(PyObject *a_mro, PyTypeObject *a, PyTypeObject *b)
{
PyObject *mro;

ASSERT_TYPE_LOCK_HELD();
mro = lookup_tp_mro(a);
if (mro != NULL) {
int res;
if (a_mro != NULL) {
/* Deal with multiple inheritance without recursion
by walking the MRO tuple */
Py_ssize_t i, n;
assert(PyTuple_Check(mro));
n = PyTuple_GET_SIZE(mro);
assert(PyTuple_Check(a_mro));
n = PyTuple_GET_SIZE(a_mro);
res = 0;
for (i = 0; i < n; i++) {
if (PyTuple_GET_ITEM(mro, i) == (PyObject *)b)
return 1;
if (PyTuple_GET_ITEM(a_mro, i) == (PyObject *)b) {
res = 1;
break;
}
}
return 0;
}
else
else {
/* a is not completely initialized yet; follow tp_base */
return type_is_subtype_base_chain(a, b);
res = type_is_subtype_base_chain(a, b);
}
return res;
}

int
PyType_IsSubtype(PyTypeObject *a, PyTypeObject *b)
{
int res;
BEGIN_TYPE_LOCK();
res = is_subtype_unlocked(a, b);
END_TYPE_LOCK()
#ifdef Py_GIL_DISABLED
PyObject *mro = _PyType_GetMRO(a);
int res = is_subtype_with_mro(mro, a, b);
Py_XDECREF(mro);
return res;
#else
return is_subtype_with_mro(lookup_tp_mro(a), a, b);
#endif
}

/* Routines to do a method lookup in the type without looking in the
Expand Down Expand Up @@ -2826,7 +2841,7 @@ mro_check(PyTypeObject *type, PyObject *mro)
}
PyTypeObject *base = (PyTypeObject*)obj;

if (!is_subtype_unlocked(solid, solid_base(base))) {
if (!is_subtype_with_mro(lookup_tp_mro(solid), solid, solid_base(base))) {
PyErr_Format(
PyExc_TypeError,
"mro() returned base with unsuitable layout ('%.500s')",
Expand Down Expand Up @@ -7082,28 +7097,29 @@ inherit_special(PyTypeObject *type, PyTypeObject *base)
#undef COPYVAL

/* Setup fast subclass flags */
if (is_subtype_unlocked(base, (PyTypeObject*)PyExc_BaseException)) {
PyObject *mro = lookup_tp_mro(base);
if (is_subtype_with_mro(mro, base, (PyTypeObject*)PyExc_BaseException)) {
type->tp_flags |= Py_TPFLAGS_BASE_EXC_SUBCLASS;
}
else if (is_subtype_unlocked(base, &PyType_Type)) {
else if (is_subtype_with_mro(mro, base, &PyType_Type)) {
type->tp_flags |= Py_TPFLAGS_TYPE_SUBCLASS;
}
else if (is_subtype_unlocked(base, &PyLong_Type)) {
else if (is_subtype_with_mro(mro, base, &PyLong_Type)) {
type->tp_flags |= Py_TPFLAGS_LONG_SUBCLASS;
}
else if (is_subtype_unlocked(base, &PyBytes_Type)) {
else if (is_subtype_with_mro(mro, base, &PyBytes_Type)) {
type->tp_flags |= Py_TPFLAGS_BYTES_SUBCLASS;
}
else if (is_subtype_unlocked(base, &PyUnicode_Type)) {
else if (is_subtype_with_mro(mro, base, &PyUnicode_Type)) {
type->tp_flags |= Py_TPFLAGS_UNICODE_SUBCLASS;
}
else if (is_subtype_unlocked(base, &PyTuple_Type)) {
else if (is_subtype_with_mro(mro, base, &PyTuple_Type)) {
type->tp_flags |= Py_TPFLAGS_TUPLE_SUBCLASS;
}
else if (is_subtype_unlocked(base, &PyList_Type)) {
else if (is_subtype_with_mro(mro, base, &PyList_Type)) {
type->tp_flags |= Py_TPFLAGS_LIST_SUBCLASS;
}
else if (is_subtype_unlocked(base, &PyDict_Type)) {
else if (is_subtype_with_mro(mro, base, &PyDict_Type)) {
type->tp_flags |= Py_TPFLAGS_DICT_SUBCLASS;
}

Expand Down Expand Up @@ -10204,7 +10220,7 @@ update_one_slot(PyTypeObject *type, pytype_slotdef *p)
d = (PyWrapperDescrObject *)descr;
if ((specific == NULL || specific == d->d_wrapped) &&
d->d_base->wrapper == p->wrapper &&
is_subtype_unlocked(type, PyDescr_TYPE(d)))
is_subtype_with_mro(lookup_tp_mro(type), type, PyDescr_TYPE(d)))
{
specific = d->d_wrapped;
}
Expand Down
Loading