Skip to content

Provide overload similar to return_value_policy to unlock the GIL #625

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

Closed
virtuald opened this issue Jan 30, 2017 · 11 comments
Closed

Provide overload similar to return_value_policy to unlock the GIL #625

virtuald opened this issue Jan 30, 2017 · 11 comments

Comments

@virtuald
Copy link
Contributor

It would be ideal to not have to create a lambda each time one wants to unlock the gil:

.def("putFrame", [](CvSource &__inst, cv::Mat &image) {
        py::gil_scoped_release release;
        __inst.PutFrame(image);
      })

Instead, it seems like an ideal API would be something like:

.def("putFrame", &CvSource::PutFrame, py::release_gil)
jagerman added a commit to jagerman/pybind11 that referenced this issue Jan 30, 2017
This cleans up the previous commit slightly by further reducing the
function call arguments to a single struct (containing the
function_record, arguments vector, and parent).

Although this doesn't currently change anything, it does allow for
future functionality to have a place for precalls to store temporary
objects that need to be destroyed after a function call (whether or not
the call succeeds).

As a concrete example, with this change pybind#625 could be easily implemented
(I think) by adding a std::unique_ptr<gil_scoped_release> member to the
`function_call` struct with a precall that actually constructs it.
Without this, the precall can't do that: the postcall won't be invoked
if the call throws an exception.

This doesn't seems to affect the .so size noticeably (either way).
@jagerman
Copy link
Member

One of the big issues with this is that it would have to be in the core function call dispatch code, but adding it would increase the object size of all function calls. I just pushed a change to #611 that might aid with this, however, by allowing this to happen through the precall mechanism without impacting other functions. See the commit message for how I think it could easily work without impacting functions that don't use it if you want to take a stab at implementing it.

@virtuald
Copy link
Contributor Author

It does seem that the precall/postcall mechanism would be ideal for implementing such a thing. Assuming that having an extra unique_ptr in the function object isn't a huge deal, that could work. I might look at this at the end of the week.

jagerman added a commit to jagerman/pybind11 that referenced this issue Jan 30, 2017
This cleans up the previous commit slightly by further reducing the
function call arguments to a single struct (containing the
function_record, arguments vector, and parent).

Although this doesn't currently change anything, it does allow for
future functionality to have a place for precalls to store temporary
objects that need to be destroyed after a function call (whether or not
the call succeeds).

As a concrete example, with this change pybind#625 could be easily implemented
(I think) by adding a std::unique_ptr<gil_scoped_release> member to the
`function_call` struct with a precall that actually constructs it.
Without this, the precall can't do that: the postcall won't be invoked
if the call throws an exception.

This doesn't seems to affect the .so size noticeably (either way).
jagerman added a commit to jagerman/pybind11 that referenced this issue Jan 30, 2017
This cleans up the previous commit slightly by further reducing the
function call arguments to a single struct (containing the
function_record, arguments vector, and parent).

Although this doesn't currently change anything, it does allow for
future functionality to have a place for precalls to store temporary
objects that need to be destroyed after a function call (whether or not
the call succeeds).

As a concrete example, with this change pybind#625 could be easily implemented
(I think) by adding a std::unique_ptr<gil_scoped_release> member to the
`function_call` struct with a precall that actually constructs it.
Without this, the precall can't do that: the postcall won't be invoked
if the call throws an exception.

This doesn't seems to affect the .so size noticeably (either way).
wjakob pushed a commit that referenced this issue Jan 31, 2017
This cleans up the previous commit slightly by further reducing the
function call arguments to a single struct (containing the
function_record, arguments vector, and parent).

Although this doesn't currently change anything, it does allow for
future functionality to have a place for precalls to store temporary
objects that need to be destroyed after a function call (whether or not
the call succeeds).

As a concrete example, with this change #625 could be easily implemented
(I think) by adding a std::unique_ptr<gil_scoped_release> member to the
`function_call` struct with a precall that actually constructs it.
Without this, the precall can't do that: the postcall won't be invoked
if the call throws an exception.

This doesn't seems to affect the .so size noticeably (either way).
@virtuald
Copy link
Contributor Author

virtuald commented Feb 8, 2017

I looked a little bit at your commit, and I don't believe that mechanism would be quite what one would need. It would release the gil too early, as it would presumably be needed for any arg casting that might happen?. Same thing with the return value.

@jagerman
Copy link
Member

jagerman commented Feb 8, 2017

Hmm, I think you're right: the precall would be fine for initializing it, as long as it was the last precall, but it needs to expire between the function call and the casting of the return value.

(There's also another problem added since that commit: with #643, the function_call might not expire right away: there are overload cases where it is saved to try again a second time).

Another idea that might work: a wrapper generator so that you could write something like:

m.def("f", gil_released(&f))

gil_release() would return a lambda with the same signature as f, and simply take out a gil_scoped_release then forward all arguments to the real f. Basically what you're doing manually, but with some clever templates to do it all for you.

@virtuald
Copy link
Contributor Author

virtuald commented Feb 8, 2017

Alright, so I wrote a wrapper function that seems to do the job (inspired by http://stackoverflow.com/questions/30679445/python-like-c-decorators and the cpp_function implementation) for my use cases:

namespace pybind11 {

    // normal functions
    template <typename Return, typename... Args>
    std::function<Return(Args...)> release_gil(Return (*f)(Args ...) PYBIND11_NOEXCEPT_SPECIFIER) {
        return [f](Args... args) -> Return {
            gil_scoped_release r;
            return f(args...);
        };
    }
    
    // member functions (non-const)
    template <typename Return, typename Class, typename... Args>
    std::function<Return(Class*, Args...)> release_gil(Return (Class::*f)(Args ...) PYBIND11_NOEXCEPT_SPECIFIER) {
        return [f](Class *c, Args... args) -> Return {
            gil_scoped_release r;
            return (c->*f)(args...);
        };
    }
    
    // member functions (const)
    template <typename Return, typename Class, typename... Args>
    std::function<Return(Class*, Args...)> release_gil(Return (Class::*f)(Args ...) const PYBIND11_NOEXCEPT_SPECIFIER) {
        return [f](const Class *c, Args... args) -> Return {
            gil_scoped_release r;
            return (c->*f)(args...);
        };
    }
    
} // namespace pybind11

@virtuald
Copy link
Contributor Author

virtuald commented Feb 8, 2017

.. ha, I hadn't seen your comment yet, but that's exactly what I was thinking too. :)

@jagerman
Copy link
Member

jagerman commented Feb 8, 2017

I think this would be worth submitting as a PR: it's a nice feature that doesn't impose any cost if not using it.

Some comments:

  • you need to add the PYBIND11_NOEXCEPT_TPL_ARG macro to the end of the template arguments, as done in cpp_function. Doing so adds another template parameter when compiled in C++17 mode, which is what the PYBIND11_NOEXCEPT_SPECIFIER contains. (In C++11/14 these macros are both empty).

  • a logical place for this would be as a static function in the gil_scoped_release class rather than as a free function. wrap seems a suitable name, so that you would call: m.def("f", py::gil_scoped_release::wrap(&myfunc)) to bind it.

  • std::function has some overhead and storage costs which is why it isn't currently used in pybind11. It ought to be possible to avoid it by basically doing what the 4 constructors in cpp_function do. (They might also be covering some cases, like the const/non-const method detection that you might want to copy).

  • I have a feeling that you would need a special case for Return = void because C++ doesn't allow return void.

@virtuald
Copy link
Contributor Author

virtuald commented Feb 8, 2017

I agree that it would be great to be builtin to pybind... if nobody else does it, I will try to get to it in March, as I've got some tight deadlines that I'm trying to meet.

@dean0x7d
Copy link
Member

dean0x7d commented Feb 8, 2017

I have a feeling that you would need a special case for Return = void because C++ doesn't allow return void.

C++ allows it:

void foo() {
    return void();
}

void bar() {
    return foo();
}

Makes generic code nice and consistent.

@jagerman
Copy link
Member

BTW: all the PYBIND11_NOEXCEPT_ stuff is gone now; it was experimental for C++17, but didn't work properly. (And the replacement is much cleaner).

@dean0x7d
Copy link
Member

dean0x7d commented Apr 2, 2017

Resolved as part of #740. The GIL can be released using:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants