FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-89013: Improve the performance of methodcaller (lazy version) by eendebakpt · Pull Request #107201 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-89013: Improve the performance of methodcaller (lazy version) - #107201

Merged
corona10 merged 21 commits into
python:mainfrom
eendebakpt:fastmethodcaller_lazy_vectorcall
Aug 1, 2023
ConversationCommits (21)ChecksFiles changed
Merged

gh-89013: Improve the performance of methodcaller (lazy version)#107201
corona10 merged 21 commits into
python:mainfrom
eendebakpt:fastmethodcaller_lazy_vectorcall

Conversation

eendebakpt commented Jul 24, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

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)

bedevere-bot mentioned this pull request Jul 24, 2023
Closed
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 commented Jul 25, 2023

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 requested review from corona10 and vstinner July 29, 2023 02:58
corona10 reviewed Jul 29, 2023
View reviewed changes
Comment thread Modules/_operator.c
corona10 reviewed Jul 29, 2023
View reviewed changes
Comment thread Modules/_operator.c Outdated
corona10 reviewed Jul 29, 2023
View reviewed changes
Comment thread Modules/_operator.c Outdated
corona10 reviewed Jul 29, 2023
View reviewed changes
Comment thread Modules/_operator.c Outdated
corona10 reviewed Jul 29, 2023
View reviewed changes
Comment thread Modules/_operator.c Outdated
Py_CLEAR(mc->xargs);
Py_CLEAR(mc->kwds);
return 0;
_methodcaller_clear_vectorcall(mc);

corona10 Jul 29, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why did you separate the function?
Please, just inline the function in here if there is no specific reason.

eendebakpt Jul 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

corona10 reviewed Jul 29, 2023
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I left some minor comments, I will take a look at this PR more.

corona10 reviewed Jul 29, 2023
View reviewed changes
Comment thread Modules/_operator.c Outdated
corona10 reviewed Jul 29, 2023
View reviewed changes
Comment thread Modules/_operator.c Outdated
corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jul 29, 2023

bedevere-bot commented Jul 29, 2023

Copy link
Copy Markdown

🤖 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 removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jul 29, 2023
corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 1, 2023

bedevere-bot commented Aug 1, 2023

Copy link
Copy Markdown

🤖 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 removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 1, 2023
corona10 approved these changes Aug 1, 2023
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Let's see the result of refleak tests.

corona10 commented Aug 1, 2023

Copy link
Copy Markdown
Member

Leak tests are unrelated to this PR.

corona10 merged commit f7c9144 into python:main Aug 1, 2023
corona10 mentioned this pull request Aug 1, 2023
Closed

vstinner commented Aug 16, 2023

Copy link
Copy Markdown
Member

Nice optimization.

colesbury mentioned this pull request Nov 21, 2024
Closed
eendebakpt mentioned this pull request Dec 1, 2024
Closed
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

corona10 corona10 approved these changes
vstinner Awaiting requested review from vstinner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL