gh-124153: Introduce PyType_GetBaseByToken function (PoC) - #121079
Closed
neonene wants to merge 103 commits into
Closed
gh-124153: Introduce PyType_GetBaseByToken function (PoC)#121079neonene wants to merge 103 commits into
neonene wants to merge 103 commits into
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
This comment was marked as outdated.
Copy link
Copy Markdown
Contributor
Author
|
The tp_bases can be used in a fallback implementation. I checked the overhead, adding a repeat function temporarily (100972d): from timeit import timeit
setup = f"""if 1:
import _testcapi
A = _testcapi.create_type_with_token("_testcapi.A", 0)
tokenA = _testcapi.get_tp_token(A)
class B(A): pass
class C(B): pass
class D(C): pass
class E(D): pass
getbase = _testcapi.repeat_getbasebytoken
"""
c_repeat = 10 # py_repeat = timeit default (1000000)
mro = timeit(s1 := f'getbase(C, tokenA, {c_repeat}, True)', setup)
bases = timeit(s2 := f'getbase(C, tokenA, {c_repeat}, False)', setup)
print(s1, mro)
print(s2, bases, bases / mro)Win non-debug: (the higher, the slower)
|
Sorry, something went wrong.
This will keep the slowdown by up to 2% on the `telco` benchmark (PGO). Unlike the `PyDecContextObject`, extending the `PyDecObject` struct seems to affect only binary ops and seems to be a waste of memory.
Faster than the upstream by up to 2% on the `telco` benchmarks (PGO/non-PGO). Based on the GetBaseByToken() optimization by ac82d36.
Keeps the performance unchanged even if the private function is not inlined (i.e. not trained well on PGO).
PyType_GetBaseByToken() fails to inline the wrapped ptivate function, whose overhead appears to be not ignorable.
This cleanup can cause a slowdown by 10% on the `telco` benchmark for some reason.
This version sets the *result to NULL at the end to reduce the overhead of double memory acces when returning true. Under verification.
encukou
reviewed
Sep 16, 2024
encukou
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
The code looks good. Do you want to clear the draft bit, and file an issue?
If there are performance more tweaks to make, they can go in a follow-up PR.
Sorry, something went wrong.
Comment thread
Modules/_ctypes/ctypes.h
Outdated
| PyErr_Format(PyExc_TypeError, "expected a ctypes type, got '%N'", type); | ||
| return NULL; | ||
| } | ||
| exercise_get_base_by_token(PyCType_Type); |
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
Could you remove the exercise from this PR?
Hopefully the training will get better as PyType_GetBaseByToken is used more; if not, we can adjust it in a future PR.
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment.
I'll post a new PR. It may be better to have a dedicated branch when the result argument is NULL.
Sorry, something went wrong.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reference implementation of the following C-API functinons:
Discussion: https://discuss.python.org/t/55598
📚 Documentation preview 📚: https://cpython-previews--121079.org.readthedocs.build/