Add a scope guard call policy #740

dean0x7d · 2017-03-16T15:19:35Z

This is a proposed solution to #625 + a bit of generalization to allow any scope guard. The basic idea is that the following:

m.def("foo", foo, py::call_guard<T>());

is equivalent to:

m.def("foo", [](Args&&... args) {
    T guard{};
    return foo(std::forward<Args>(args)...);
});

To release the GIL, one would pass py::call_guard<py::gil_scoped_release>() (could also be aliased to something shorter). In general, any default-constructible RAII class could be used.

Implementation

It compiles to nothing when the guard isn't used so there's no binary size or runtime overhead. It mainly uses the type system and integrates with the existing wrapper created for all functions.

An alternative would be to create a cpp_function-like wrapper as proposed in #625 and there are definitely pros and cons to both approaches. The main reason for submitting this PR is that the implementation uses minimal code compared to actual function wrapping and argument forwarding.

jagerman · 2017-03-16T16:03:45Z

Nice approach, I like it!

wjakob · 2017-03-16T16:09:11Z

Cool, I like it as well!

dean0x7d · 2017-03-25T14:25:05Z

OK, the documentation is in place. Feel free to review.

The only thing I couldn't find out is how to check the GIL state with PyPy (it doesn't seem to be supported via the C API) so that test is skipped there, but the generic guard test covers the same functionality.

wjakob · 2017-03-25T14:33:37Z

This looks great, the documentation is very clear.

One potential area for improvement: I think it's only a matter of time until people will try to mix multiple call guards for a single bound function (just like one can specify multiple keep_alive policies). I'm wondering is there is a more general implementation of the extract_call_guard_t interface that would allow this to work?

wjakob · 2017-03-25T14:34:53Z

(This could be something like a filter over Extras that assembles a std::tuple with all of the call guards)

dean0x7d · 2017-03-25T15:06:25Z

That's a good point about multiple guards. I forgot to document how that would work with the current implementation.

Unfortunately, I don't think that a std::tuple or any parameter pack based implementation is viable. I had considered it originally, but the issue is that the construction and destruction order of tuple elements is not specified. Same goes for argument initialization order in a function call (if multiple guards were expanded where there is currently one). This could yield some surprises if users assume that call_guard<T1>, call_guard<T2> will follow that order but it's actually random depending on the platform (with pretty nasty results, e.g. if T2 releases the GIL, but T1 uses the CPython API).

A solution with well-defined order would be for users to create a simple composite guard:

struct CompositeGuard {
    T1 guard1;
    T2 guard2;
};

So perhaps just add that to the documentation + a static_assert to ensure only a simple call_guard is allowed.

dean0x7d · 2017-03-25T15:37:13Z

Thinking about it a bit more, (ab)using std::initializer_list would work with well-defined guard order. But I think the implementation is on the ugly side:

Return call_impl(Func &&f, index_sequence<Is...>, std::initializer_list<int>);

call_impl<Return>(std::forward<Func>(f), indices{}, {((void) Guard{}, 0)...});

jagerman · 2017-03-25T17:31:16Z

I have some trouble seeing why that would guarantee the guards aren't destroyed until the call is finished; wouldn't it be valid for a compiler to destroy them after evaluating the initializer list?

jagerman · 2017-03-25T19:29:26Z

Something like:

template <typename Guard, typename...> struct call_guardian { Guard guard; };
template <typename Guard1, typename Guard2, typename... Guards>
struct call_guardian<Guard1, Guard2, Guards...> {
    Guard1 guard;
    call_guardian<Guard2, Guards...> next;
};

ought to be enough to guarantee FILO guard construction/destruction order.

jagerman · 2017-03-25T20:01:56Z

include/pybind11/common.h

+/// Check if T is an instantiation of the template `Class`. For example:
+/// `is_<std::shared_ptr, T>` is true if `T == std::shared_ptr<U>` where U can be anything.
+template <template<typename...> class Class, typename T> struct is_ : std::false_type { };
+template <template<typename...> class C, typename... Us> struct is_<C, C<Us...>> : std::true_type { };


Perhaps rename this to is_specialization? is_ isn't immediately obvious.

True, renamed. I kind of liked the naming as a prefix of some kind, e.g. is_call_guard = is_<call_guard, T>, but a proper name is more useful.

dean0x7d · 2017-03-25T23:20:04Z

I have some trouble seeing why that would guarantee the guards aren't destroyed until the call is finished; wouldn't it be valid for a compiler to destroy them after evaluating the initializer list?

Temporary objects are cleaned up at the end of the full expression in which they appear -- essentially at ;. This is what makes nested and chained calls work in C++ (reference lifetime extension doesn't propagate through function returns).

That being said, the init list is still ugly and I don't like. Your call_guardian idea is much better!

The only question I have now: call_guard<T1, T2> or call_guard<T1>, call_guard<T2>? The former is trivial to implement and it would make a terser syntax. The latter is more versatile since additional guards can be added at a later stage, but it does require passing Extra... through a filter metafunction. I assume the more versatile one is more desirable?

wjakob · 2017-03-28T22:29:32Z

Both are fine with me. It sounds like the first variant is easier to implement and faster to compile, so I'd lean towards that one.

jagerman · 2017-03-28T22:36:03Z

I thought I commented on that, but apparently not: I like the first variant more as well, unless there's some compelling alternative use you have in mind for extra parameters.

dean0x7d · 2017-03-29T13:10:25Z

OK, I've implemented the call_guard<T1, T2, ...> version and replaced first_of_t with exactly_one_t (implementation in terms of the existing constexpr functions + pack_element which may be useful for other things as well).

jagerman · 2017-04-01T20:34:59Z

I can definitely use pack_element in #762; right now it uses std::tuple_element<N, std::tuple<...>>, but since I don't actually need the tuple, that seemed rather heavy.

jagerman

Looks good to me, aside from a couple minor comment updates.

jagerman · 2017-04-01T20:47:34Z

include/pybind11/pybind11.h

@@ -143,8 +143,11 @@ class cpp_function : public function {
            /* Override policy for rvalues -- usually to enforce rvp::move on an rvalue */
            const auto policy = detail::return_value_policy_override<Return>::policy(call.func.policy);

+            /* Function scope guard -- defaults to the compile-to-nothing `void_type` */


~~I think `void_type` should be `call_guard<>`~~

Scratch that, I see now: it's going to be call_guard<>::type, which is the void_type.

Yeah, the 0 and 1 argument versions just return the type directly since those are the most common cases. The additional wrappers start with 2 arguments and above.

jagerman · 2017-04-01T20:48:19Z

tests/test_call_policies.cpp

@@ -0,0 +1,92 @@
+/*
+    tests/test_keep_alive.cpp -- keep_alive modifier (pybind11's version
+    of Boost.Python's with_custodian_and_ward / with_custodian_and_ward_postcall)


The comment filename and description are obsolete now. Perhaps:

tests/test_call_policies.cpp -- keep_alive (pybind11's version of Boost.Python's with_custodian_and_ward / with_custodian_and_ward_postcall) and call_guard examples and tests.

Fixed, thanks.

```c++ m.def("foo", foo, py::call_guard<T>()); ``` is equivalent to: ```c++ m.def("foo", [](args...) { T scope_guard; return foo(args...); // forwarded arguments }); ```

dean0x7d · 2017-04-02T21:00:51Z

Fixed the comment and squashed the commits. If there's nothing else, I'll merge this after the CI tests are done.

wjakob · 2017-04-02T21:10:09Z

This looks great, please go ahead!

dean0x7d added this to the v2.2 milestone Mar 16, 2017

dean0x7d force-pushed the call_guard branch from 7303340 to 29623ba Compare March 25, 2017 14:18

dean0x7d changed the title ~~WIP: Add a scope guard call policy~~ Add a scope guard call policy Mar 25, 2017

jagerman reviewed Mar 25, 2017

View reviewed changes

jagerman reviewed Apr 1, 2017

View reviewed changes

dean0x7d added 2 commits April 2, 2017 22:48

Add a scope guard call policy

ca5d914

```c++ m.def("foo", foo, py::call_guard<T>()); ``` is equivalent to: ```c++ m.def("foo", [](args...) { T scope_guard; return foo(args...); // forwarded arguments }); ```

Replace first_of_t with exactly_one_t

ec20807

dean0x7d force-pushed the call_guard branch from 1b4f1b5 to ec20807 Compare April 2, 2017 20:50

dean0x7d merged commit 82ece94 into pybind:master Apr 2, 2017

dean0x7d mentioned this pull request Apr 2, 2017

Provide overload similar to return_value_policy to unlock the GIL #625

Closed

dean0x7d deleted the call_guard branch April 5, 2017 08:26

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#740

Closed

Add a scope guard call policy #740

Add a scope guard call policy #740

Uh oh!

Conversation

dean0x7d commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation

Uh oh!

jagerman commented Mar 16, 2017

Uh oh!

wjakob commented Mar 16, 2017

Uh oh!

dean0x7d commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjakob commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjakob commented Mar 25, 2017

Uh oh!

dean0x7d commented Mar 25, 2017

Uh oh!

dean0x7d commented Mar 25, 2017

Uh oh!

jagerman commented Mar 25, 2017

Uh oh!

jagerman commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagerman Mar 25, 2017

Choose a reason for hiding this comment

Uh oh!

dean0x7d Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

dean0x7d commented Mar 25, 2017

Uh oh!

wjakob commented Mar 28, 2017

Uh oh!

jagerman commented Mar 28, 2017

Uh oh!

dean0x7d commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagerman commented Apr 1, 2017

Uh oh!

jagerman left a comment

Choose a reason for hiding this comment

Uh oh!

jagerman Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jagerman Apr 1, 2017

Choose a reason for hiding this comment

Uh oh!

dean0x7d Apr 2, 2017

Choose a reason for hiding this comment

Uh oh!

jagerman Apr 1, 2017

Choose a reason for hiding this comment

Uh oh!

dean0x7d Apr 2, 2017

Choose a reason for hiding this comment

Uh oh!

dean0x7d commented Apr 2, 2017

Uh oh!

wjakob commented Apr 2, 2017

Uh oh!

Uh oh!

dean0x7d commented Mar 16, 2017 •

edited

Loading

dean0x7d commented Mar 25, 2017 •

edited

Loading

wjakob commented Mar 25, 2017 •

edited

Loading

jagerman commented Mar 25, 2017 •

edited

Loading

dean0x7d commented Mar 29, 2017 •

edited

Loading

jagerman Apr 1, 2017 •

edited

Loading