gh-116868: Avoid locking in PyType_IsSubtype #116829
Conversation
DinoV
changed the title
| if (mro == NULL) { | ||
| return NULL; | ||
| } | ||
| if (_Py_TryIncref(&self->tp_mro, mro)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm probably using "deferred reference count" too broadly here. I'm thinking of something like:
- PyType_IsSubtype uses a borrowed reference to
self->tp_mro - Modifications to the mro use QSBR to delay decref'ing the old MRO
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
colesbury
left a comment
colesbury
left a comment
There was a problem hiding this comment.
LGTM.
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? |
It's in "Deferred tasks " in #108219 |
Make PyType_IsSubType not acquire lock
Make PyType_IsSubType not acquire lock
Make PyType_IsSubType not acquire lock
3 participants
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 threadsPyCFunction_Checkhits this pretty heavily and leads to a nearly 50% slow down vs. a lock free version.This uses
_Py_TryIncrefto try and get a reference to the MRO and falls back to the lock only if that cannot succeed.PyType_IsSubtype#116868