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.

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

Conversation

DinoV
Copy link
Contributor

@DinoV DinoV commented Mar 14, 2024

This showed up as a heavy point of contention while testing PR #116775. Currently we just simply lock for PyType_IsSubType, when profiling is enabled on all threads PyCFunction_Check hits this pretty heavily and leads to a nearly 50% slow down vs. a lock free version.

This uses _Py_TryIncref to try and get a reference to the MRO and falls back to the lock only if that cannot succeed.

@DinoV DinoV changed the title Avoid locking in PyType_IsSubtype gh-116868: Avoid locking in PyType_IsSubtype Mar 15, 2024
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.

@DinoV DinoV marked this pull request as ready for review March 15, 2024 16:13
@DinoV DinoV requested a review from markshannon as a code owner March 15, 2024 16:13
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

LGTM.

I suspect we'll want to avoid reference counting the mro in PyType_IsSubtype in the future, but we can address that later.

@DinoV DinoV merged commit 280de36 into python:main Mar 15, 2024
34 of 35 checks passed
@itamaro
Copy link
Contributor

itamaro commented Mar 16, 2024

I suspect we'll want to avoid reference counting the mro in PyType_IsSubtype in the future, but we can address that later.

Is there an open issue to track the "addressing it later" bit?

@colesbury
Copy link
Contributor

Is there an open issue to track the "addressing it later" bit?

It's in "Deferred tasks " in #108219

vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@DinoV DinoV deleted the nogil_issubtype branch May 31, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants