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

vectorcall: fix bindings & use for call0 and call_method0 #1287

Merged
merged 3 commits into from
Nov 26, 2020

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Nov 19, 2020

This PR fixes the broken vectorcall ffi bindings, and proves that the new ones work by using them in .call0 and .call_method0 implementations (where possible).

This yields quite a tidy ~20% performance speedup on those functions, because it no longer creates an empty PyTuple and PyDict each time:

before:

test bench_call_0        ... bench:      50,712 ns/iter (+/- 5,555)
test bench_call_method_0 ... bench:     164,581 ns/iter (+/- 10,141)

after:

test bench_call_0        ... bench:      40,996 ns/iter (+/- 4,490)
test bench_call_method_0 ... bench:     138,826 ns/iter (+/- 18,465)

I'd love to follow this PR up with some ways to use vectorcall for functions with arguments too. Will leave that for #684 instead of pushing this further right now.

Closes #1283

@davidhewitt
Copy link
Member Author

Windows failing to link on abi3 - presumably it's us doing something wrong; I'm asking about it at https://bugs.python.org/issue42415

@davidhewitt
Copy link
Member Author

Rather than wait for that bug to be resolved (which I guess might not happen until Python 3.9.1) I've put a TODO in the code and just enabling the optimization for the unlimited API for now.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks. This is a pretty good step towards vectorizing function calls. Comments are mainly around FFI definitions.

src/ffi/cpython/abstract_.rs Outdated Show resolved Hide resolved

#[cfg(all(Py_3_8, not(PyPy)))]
#[inline(always)]
pub unsafe fn _PyObject_FastCallTstate(
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm looking at the same file but the version in master (so 3.10-dev). I figured it was best for these inline functions to be as up-to-date as possible.

Comment on lines +202 to +208
// skipped _PyObject_VectorcallMethodId
// skipped _PyObject_CallMethodIdNoArgs
// skipped _PyObject_CallMethodIdOneArg

// skipped _PyObject_HasLen
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why are they skipped?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't consider them relevant to my PR; I could add them but I'd also quite like to keep as few unlimited API definitions as possible (as they're more likely for us to be work to maintain).

See also #1289 which talks a bit about what I'm thinking.

@@ -0,0 +1,3 @@
pub mod abstract_;
Copy link
Member

Choose a reason for hiding this comment

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

👍 for creating this new module. Maybe we can reduce cfg using this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think for sure (again see #1289).

src/instance.rs Outdated Show resolved Hide resolved
@davidhewitt davidhewitt force-pushed the vectorcall branch 2 times, most recently from 6d5c5cf to 4d14ac8 Compare November 22, 2020 18:36
@kngwyu
Copy link
Member

kngwyu commented Nov 26, 2020

Thanks!

@kngwyu kngwyu merged commit 47a731b into PyO3:master Nov 26, 2020
@davidhewitt davidhewitt deleted the vectorcall branch December 24, 2021 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Vectorcall Bindings
2 participants