-
-
Notifications
You must be signed in to change notification settings - Fork 34k
bpo-47067: Add vectorcall for gaobject #31996
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
FYI, we decided to add caching mechanism instead of applying vector call. cc @sweeneyde |
|
@sweeneyde |
effb099 to
9496dab
Compare
|
BTW, I replicated some speedup with pyperf commmands like Some results: dict[str, int](a=1, b=2) # 359 ns +- 6 ns -> 280 ns +- 3 ns: 1.29x faster
list[int](()) # 259 ns +- 3 ns -> 246 ns +- 5 ns: 1.05x faster
MappingProxyType[str, int](d) # 273 ns +- 13 ns -> 261 ns +- 4 ns: 1.05x faster
class A:
def __init__(self, a, b):
pass
G = GenericAlias(A, int)
G(1, 2) # 198 ns +- 6 ns -> 190 ns +- 4 ns: 1.04x faster |
sweeneyde
left a comment
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.
Some small code style / PEP 7 nits
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
So what does this speed up -- the |
The |
|
If we’re now changing Makefile this PR seems to have strayed from the topic? |
I agree, we should probably keep the vectorcall changes for now, then explore global strings in another PR. |
|
Just FYI the order of execution of deep freeze and global string generator matter so don't change it in this PR. |
0b7c47c to
5fd5165
Compare
|
I have made the requested changes; please review again :) |
|
Thanks for making the requested changes! @sweeneyde: please review the changes made to this pull request. |
|
Thanks for the changes, this is looking close. I am still not convinced that adding
|
|
In my mind,
For the purpose of simplicity and decoupling, it is appropriate to delete it :) |
sweeneyde
left a comment
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.
Very close! One last thing.
Misc/NEWS.d/next/Library/2022-03-20-17-15-56.bpo-47067.XXLnje.rst
Outdated
Show resolved
Hide resolved
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
sweeneyde
left a comment
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.
Thanks!
Misc/NEWS.d/next/Library/2022-03-20-17-15-56.bpo-47067.XXLnje.rst
Outdated
Show resolved
Hide resolved
e4e5065 to
2355648
Compare
|
The string change seems to have really improved the microbenchmarks. Before/after this PR: I also checked for refleaks just for fun, and Thanks for the contribution! |
|
Great results! In the end, how was the "make regen-all" issue resolved? |
I don't think it was solved, but ignored for the sake of this particular PR. I added a comment to https://bugs.python.org/issue46712 inquiring about it |


https://bugs.python.org/issue47067