-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Use vectorcall for all-positional-argument calls #5896
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
a669603
ddcc5d2
9120b9c
d5a88c3
7e66762
f6aaf68
7529fbe
6c1b2f7
278f081
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2171,29 +2171,73 @@ class argument_loader { | |
|
|
||
| /// Helper class which collects only positional arguments for a Python function call. | ||
| /// A fancier version below can collect any argument, but this one is optimal for simple calls. | ||
| template <return_value_policy policy> | ||
| template <size_t N, return_value_policy policy> | ||
| class simple_collector { | ||
| // Disable warnings about useless comparisons when N == 0. | ||
| PYBIND11_WARNING_PUSH | ||
| PYBIND11_WARNING_DISABLE_GCC("-Wtype-limits") | ||
| PYBIND11_WARNING_DISABLE_INTEL(186) | ||
|
||
| public: | ||
| template <typename... Ts> | ||
| explicit simple_collector(Ts &&...values) | ||
| : m_args(pybind11::make_tuple<policy>(std::forward<Ts>(values)...)) {} | ||
| explicit simple_collector(Ts &&...values) { | ||
| static_assert(sizeof...(Ts) == N, ""); | ||
| size_t i = 0; | ||
| using expander = int[]; | ||
| (void) expander{ | ||
| 0, | ||
| (m_args[i++] = detail::make_caster<Ts>::cast(std::forward<Ts>(values), policy, nullptr) | ||
| .inc_ref() | ||
| .ptr(), | ||
| 0)...}; | ||
| for (i = 0; i < N; ++i) { | ||
| if (!m_args[i]) { | ||
| #if !defined(PYBIND11_DETAILED_ERROR_MESSAGES) | ||
| throw cast_error_unable_to_convert_call_arg(std::to_string(i)); | ||
| #else | ||
| std::array<std::string, N> argtypes{{type_id<Ts>()...}}; | ||
| throw cast_error_unable_to_convert_call_arg(std::to_string(i), argtypes[i]); | ||
| #endif | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const tuple &args() const & { return m_args; } | ||
| dict kwargs() const { return {}; } | ||
| ~simple_collector() { | ||
| for (size_t i = 0; i < N; ++i) { | ||
| handle(m_args[i]).dec_ref(); | ||
| } | ||
| } | ||
|
|
||
| tuple args() && { return std::move(m_args); } | ||
| simple_collector(const simple_collector &) = delete; | ||
| simple_collector(simple_collector &&) noexcept = default; | ||
| simple_collector &operator=(const simple_collector &) = delete; | ||
| simple_collector &operator=(simple_collector &&) noexcept = default; | ||
|
|
||
| tuple args() const { | ||
| tuple result(N); | ||
| for (size_t i = 0; i < N; ++i) { | ||
| PyTuple_SET_ITEM(result.ptr(), i, handle(m_args[i]).inc_ref().ptr()); | ||
| } | ||
| return result; | ||
| } | ||
| dict kwargs() const { return {}; } | ||
|
|
||
| /// Call a Python function and pass the collected arguments | ||
| object call(PyObject *ptr) const { | ||
| PyObject *result = PyObject_CallObject(ptr, m_args.ptr()); | ||
| #if PY_VERSION_HEX >= 0x03090000 | ||
| PyObject *result = PyObject_Vectorcall(ptr, m_args.data(), N, nullptr); | ||
| #else | ||
| // Use the old name for 3.8. | ||
| PyObject *result = _PyObject_Vectorcall(ptr, m_args.data(), N, nullptr); | ||
| #endif | ||
| if (!result) { | ||
| throw error_already_set(); | ||
| } | ||
| return reinterpret_steal<object>(result); | ||
| } | ||
|
|
||
| private: | ||
| tuple m_args; | ||
| std::array<PyObject *, N> m_args; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we get away from the N template instantiations by having a maximum N (capacity) and runtime |
||
| PYBIND11_WARNING_POP | ||
| }; | ||
|
|
||
| /// Helper class which collects positional, keyword, * and ** arguments for a Python function call | ||
|
|
@@ -2328,8 +2372,8 @@ constexpr bool args_are_all_positional() { | |
| template <return_value_policy policy, | ||
| typename... Args, | ||
| typename = enable_if_t<args_are_all_positional<Args...>()>> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Connected to the capacity idea above: I think we don't have to be stingy with the capacity, e.g. 128 (pointers) would seem totally fine, so in practice this would hardly ever go to the fallback. |
||
| simple_collector<policy> collect_arguments(Args &&...args) { | ||
| return simple_collector<policy>(std::forward<Args>(args)...); | ||
| simple_collector<sizeof...(Args), policy> collect_arguments(Args &&...args) { | ||
| return simple_collector<sizeof...(Args), policy>(std::forward<Args>(args)...); | ||
| } | ||
|
|
||
| /// Collect all arguments, including keywords and unpacking (only instantiated when needed) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC there are two downsides with this approach:
The obvious question: Is it really worth it, for real-world situations?