gh-124153: Introduce PyType_GetBaseByToken function (PoC)#121079
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
This comment was marked as outdated.
This comment was marked as outdated.
ContributorAuthor
neonene commented Jul 3, 2024
The fromtimeitimporttimeitsetup=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)
|
neonene changed the title 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
Member
encukou left a comment
encukou left a comment 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.
Modules/_ctypes/ctypes.h Outdated
| PyErr_Format(PyExc_TypeError, "expected a ctypes type, got '%N'", type); | ||
| returnNULL; | ||
| } | ||
| exercise_get_base_by_token(PyCType_Type); |
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.
ContributorAuthor
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.
neonene changed the title 
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
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:
PyType_GetBaseByToken()PyType_GetToken()Discussion: https://discuss.python.org/t/55598
📚 Documentation preview 📚: https://cpython-previews--121079.org.readthedocs.build/
PyType_GetBaseByTokenfunction withPy_tp_tokenslot #124153