Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-89013: Improve the performance of methodcaller (lazy version)#107201
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
gh-89013: Improve the performance of methodcaller (lazy version) #107201
Uh oh!
There was an error while loading. Please reload this page.
Conversation
eendebakpt commented Jul 24, 2023 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
corona10 commented Jul 25, 2023
How is methodcaller well used? Often converting to the vectorcall requires maintenance cost, so you need to justify increasing the code complexity. |
eendebakpt commented Jul 27, 2023
@corona10 The PR indeeds adds some additional code (the
But comparing performance and maintenance costs is a bit like comparing apples and oranges, so it it up to you (and other core devs) to make a decision here. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Modules/_operator.c Outdated
| Py_CLEAR(mc->xargs); | ||
| Py_CLEAR(mc->kwds); | ||
| return0; | ||
| _methodcaller_clear_vectorcall(mc); |
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.
Why did you separate the function?
Please, just inline the function in here if there is no specific reason.
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.
To make it more symmetric with the _methodcaller_initialize_vectorcall. I will inline here. Let me know if the _methodcaller_initialize_vectorcall should also be inlined.
Misc/NEWS.d/next/Library/2021-08-16-17-52-26.bpo-44850.r8jx5u.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
corona10 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.
I left some minor comments, I will take a look at this PR more.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented Jul 29, 2023
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
…endebakpt/cpython into fastmethodcaller_lazy_vectorcall
bedevere-bot commented Aug 1, 2023
corona10 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.
LGTM
Let's see the result of refleak tests.
corona10 commented Aug 1, 2023
Leak tests are unrelated to this PR. |
vstinner commented Aug 16, 2023
Nice optimization. |
This is a variation on #106960 where the allocation of the structures required for the vectorcall is delayed untill the first invocation of the
methodcaller. The advantage is that that formethodcallerobjects that are created, but never used, the creation time and amount of memory used is the same as current main.Benchmark results:
Benchmark script