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.

Sign up for GitHub

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

Jump to bottom

bpo-37233: optimize case totalargs == 0 in method_vectorcall #14550

Merged
merged 1 commit into from
Jul 3, 2019
Merged

bpo-37233: optimize case totalargs == 0 in method_vectorcall #14550

merged 1 commit into from
Jul 3, 2019

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented Jul 2, 2019 •
edited by bedevere-bot
Loading

Because of bpo-37138, we need to check the case totalargs == 0 anyway. So it doesn't hurt to move that check earlier and add a fast path for the common case of calling a method without arguments.

CC @methane

https://bugs.python.org/issue37233

@gpshead gpshead requested a review from methane July 2, 2019 22:09
@methane
Copy link
Member

methane commented Jul 3, 2019

I'm not sure this is really optimization.
Isn't (nargsf & PY_VECTORCALL_ARGUMENTS_OFFSET) true on major case?

We need if (totalargs) { at C level. But it doesn't mean this check is needed in assembly level.
Compilers may optimize out this check if it is safe.

Would you show microbenchmark result for this?

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jul 3, 2019

Compilers may optimize out this check if it is safe.

I checked and they don't optimize away this check. So my original reasoning is still correct: if this check has to be done anyway, we should put it as early as possible.

@methane methane merged commit 53c2143 into python:master Jul 3, 2019
@miss-islington
Copy link
Contributor

Thanks @jdemeyer for the PR, and @methane for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @jdemeyer and @methane, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 53c214344038341ce86fcf7efa12dc33be9d5b45 3.8

@jdemeyer jdemeyer deleted the method_vectorcall branch July 3, 2019 12:01
@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jul 3, 2019

If you want to backport this, you should also backport #13974

@methane
Copy link
Member

methane commented Jul 3, 2019

OK, I want to keep 3.8 and 3.9 same to ease future backporting.
#13974 seems safe enough to backport too.

@miss-islington
Copy link
Contributor

Thanks @jdemeyer for the PR, and @methane for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 3, 2019
@jdemeyer @miss-islington
bpo-37233: optimize method_vectorcall in case of totalargs == 0 (pyth…
03cf5f2 
…onGH-14550)

(cherry picked from commit 53c2143)

Co-authored-by: Jeroen Demeyer <J.Demeyer@UGent.be>
@bedevere-bot
Copy link

GH-14574 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Jul 3, 2019
@miss-islington @jdemeyer
bpo-37233: optimize method_vectorcall in case of totalargs == 0 (GH-1…
1099e34 
…4550)

(cherry picked from commit 53c2143)

Co-authored-by: Jeroen Demeyer <J.Demeyer@UGent.be>
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@jdemeyer @lisroach
bpo-37233: optimize method_vectorcall in case of totalargs == 0 (pyth…
88a4ecf 
…onGH-14550)
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@jdemeyer @DinoV
bpo-37233: optimize method_vectorcall in case of totalargs == 0 (pyth…
95f5d8e 
…onGH-14550)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@methane methane Awaiting requested review from methane

Assignees

@methane methane

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jdemeyer @methane @miss-islington @bedevere-bot @gpshead @the-knights-who-say-ni