Skip to content

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

Merged
methane merged 1 commit intopython:masterfrom
jdemeyer:method_vectorcall
Jul 3, 2019
Merged

bpo-37233: optimize case totalargs == 0 in method_vectorcall#14550
methane merged 1 commit intopython:masterfrom
jdemeyer:method_vectorcall

Conversation

@jdemeyer
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

miss-islington commented Jul 3, 2019

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
Copy Markdown
Contributor

miss-islington commented Jul 3, 2019

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
Copy Markdown
Contributor Author

jdemeyer commented Jul 3, 2019

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

@methane
Copy link
Copy Markdown
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
Copy Markdown
Contributor

miss-islington commented Jul 3, 2019

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
Copy Markdown

bedevere-bot commented Jul 3, 2019

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