Skip to content
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

Merged
merged 21 commits into from
Aug 1, 2023

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Jul 24, 2023

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 for methodcaller objects that are created, but never used, the creation time and amount of memory used is the same as current main.

Benchmark results:

call: Mean +- std dev: [main] 136 ns +- 1 ns -> [pr_lazy] 58.3 ns +- 1.4 ns: 2.34x faster
creation: Mean +- std dev: [main] 98.7 ns +- 3.4 ns -> [pr_lazy] 95.7 ns +- 3.2 ns: 1.03x faster
creation+call: Mean +- std dev: [main] 255 ns +- 9 ns -> [pr_lazy] 186 ns +- 6 ns: 1.37x faster
call kwarg: Mean +- std dev: [main] 217 ns +- 3 ns -> [pr_lazy] 85.9 ns +- 2.4 ns: 2.53x faster
creation kwarg: Mean +- std dev: [main] 166 ns +- 4 ns -> [pr_lazy] 170 ns +- 5 ns: 1.02x slower
creation+call kwarg: Mean +- std dev: [main] 386 ns +- 13 ns -> [pr_lazy] 339 ns +- 4 ns: 1.14x faster

Geometric mean: 1.45x faster
Benchmark script
import pyperf

setup = """
from operator import methodcaller as mc
arr = []
call = mc('sort')
call_kwarg = mc('sort', reverse=True)
"""

runner = pyperf.Runner()
runner.timeit(name="call", stmt="call(arr)", setup=setup)
runner.timeit(name="creation", stmt="call = mc('sort')", setup=setup)
runner.timeit(name="creation+call", stmt="call = mc('sort'); call(arr)", setup=setup)
runner.timeit(name="call kwarg", stmt="call_kwarg(arr)", setup=setup)
runner.timeit(name="creation kwarg", stmt="call = mc('sort', reverse=True)", setup=setup)
runner.timeit(name="creation+call kwarg", stmt="call = mc('sort', reverse=True); call(arr)", setup=setup)

@eendebakpt eendebakpt changed the title Draft: gh-89013: Improve the performance of methodcaller (lazy version) gh-89013: Improve the performance of methodcaller (lazy version) Jul 24, 2023
@corona10
Copy link
Member

How is methodcaller well used? Often converting to the vectorcall requires maintenance cost, so you need to justify increasing the code complexity.

@eendebakpt
Copy link
Contributor Author

@corona10 The PR indeeds adds some additional code (the methodcaller_vectorcall is small, the initialization code a bit larger), but it is not a lot of code. If maintenance is a concern, I can try rewrite the code to keep the additions to a minimum. (in this PR I added some code because of an earlier concern of added memory usage)

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.

@corona10 corona10 requested review from vstinner and corona10 July 29, 2023 02:58
Py_CLEAR(mc->kwds);
return 0;
_methodcaller_clear_vectorcall(mc);
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@corona10 corona10 left a 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.

@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jul 29, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit d70a8ad 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jul 29, 2023
@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 1, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 812856b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 1, 2023
Copy link
Member

@corona10 corona10 left a 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
Copy link
Member

corona10 commented Aug 1, 2023

Leak tests are unrelated to this PR.

@vstinner
Copy link
Member

Nice optimization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants