gh-131586: Avoid refcount contention in some "special" calls #131588
Conversation
In the free threaded build, the `_PyObject_LookupSpecial()` call can lead to reference count contention on the returned function object becuase it doesn't use stackrefs. Refactor some of the callers to use `_PyObject_MaybeCallSpecialNoArgs`, which uses stackrefs internally. This fixes the scaling bottleneck in the "lookup_special" microbenchmark in `ftscalingbench.py`. However, the are still some uses of `_PyObject_LookupSpecial()` that need to be addressed in future PRs.
| if (method != NULL) { | ||
| PyObject *result = _PyObject_CallNoArgs(method); | ||
| Py_DECREF(method); | ||
| PyObject *result = _PyObject_MaybeCallSpecialNoArgs(number, &_Py_ID(__ceil__)); |
There was a problem hiding this comment.
Nice approach, we don't need module state anymore for this case.
cc @vstinner
colesbury
requested review from
Fidget-Spinner,
ericsnowcurrently and
markshannon
as code owners
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 93c8143 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131588%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
Objects/typeobject.c
Outdated
| /* Internal API to look for a name through the MRO. | ||
| This returns a strong reference, and doesn't set an exception! | ||
| If nonzero, version is set to the value of type->tp_version at the time of | ||
| the lookup. | ||
| */ |
There was a problem hiding this comment.
This comment needs to be updated.
Include/internal/pycore_object.h
Outdated
| @@ -890,6 +890,8 @@ extern bool _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, | |||
| PyObject **attr); | |||
| extern PyObject *_PyType_LookupRefAndVersion(PyTypeObject *, PyObject *, | |||
| unsigned int *); | |||
| extern unsigned int | |||
There was a problem hiding this comment.
| extern unsigned int | |
| extern uint32_t |
int returns imply a very limited range of values, usually only 0/-1. Please use C99 int types when returning actual data, rather just an error code.
Should the comment explaining what it does go here, rather than on the implementation?
It seems to me that "what" comments go in headers, and "how" comments in the C code.
There was a problem hiding this comment.
I've moved the comment to the header file. The return value is unsigned int because tp_version_tag (in PyTypeObject) is an unsigned int and all the other _PyType_Lookup variants store the version tag as an unsigned int.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Oops... those are the numbers for a different PR (#130064). Performance looks unchanged with this PR (<0.1%) |
|
Nice enhancement. |
In the free threaded build, the
_PyObject_LookupSpecial()call can lead to reference count contention on the returned function object becuase it doesn't use stackrefs. Refactor some of the callers to use_PyObject_MaybeCallSpecialNoArgs, which uses stackrefs internally.This fixes the scaling bottleneck in the "lookup_special" microbenchmark in
ftscalingbench.py. However, the are still some uses of_PyObject_LookupSpecial()that need to be addressed in future PRs._PyType_LookupRefand related functions #131586