Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-131586: Avoid refcount contention in some "special" calls#131588
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
In the free threaded build, the `_PyObject_LookupSpecial()` call can lead toreference count contention on the returned function object becuase itdoesn'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" microbenchmarkin `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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice approach, we don't need module state anymore for this case.
cc@vstinner
Uh oh!
There was an error while loading.Please reload this page.
bedevere-bot commentedMar 24, 2025
🤖 New build scheduled with the buildbot fleet by@colesbury for commit93c8143 🤖 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This comment needs to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The overall design LGTM and the implementation looks correct. But I don't understand well StackRef, so I prefer to abstain from approving this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good. One quibble about the naming of a return type.
OOI, do you have any numbers of the impact of the PR, either stats or benchmarks?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| externunsignedint | |
| externuint32_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've moved the comment to the header file. The return value isunsigned int becausetp_version_tag (in PyTypeObject) is anunsigned int and all the other_PyType_Lookup variants store the version tag as anunsigned 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%) |
67fbfb4 intopython:mainUh oh!
There was an error while loading.Please reload this page.
Nice enhancement. |
…ython#131588)In the free threaded build, the `_PyObject_LookupSpecial()` call can lead toreference count contention on the returned function object becuase itdoesn'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" microbenchmarkin `ftscalingbench.py`. However, the are still some uses of`_PyObject_LookupSpecial()` that need to be addressed in future PRs.
…ython#131588)In the free threaded build, the `_PyObject_LookupSpecial()` call can lead toreference count contention on the returned function object becuase itdoesn'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" microbenchmarkin `ftscalingbench.py`. However, the are still some uses of`_PyObject_LookupSpecial()` that need to be addressed in future PRs.
Uh oh!
There was an error while loading.Please reload this page.
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