Improvement: Less GIL locking

[zwimer]

Binder currently seems to grab the GIL where not necessary. Here is an example:

struct PyCallBack_Annotation_Base : public Annotation::Base {
    using Annotation::Base::Base;

    bool eliminatable() const override {
        pybind11::gil_scoped_acquire gil;
        pybind11::function overload = pybind11::get_overload(static_cast<const Annotation::Base *>(this), "eliminatable");
        if (overload) {
            auto o = overload.operator()<pybind11::return_value_policy::reference>();
            if (pybind11::detail::cast_is_temporary_value_reference<bool>::value) {
                static pybind11::detail::override_caster_t<bool> caster;
                return pybind11::detail::cast_ref<bool>(std::move(o), caster);
            }
            else return pybind11::detail::cast_safe<bool>(std::move(o));
        }
        return Base::eliminatable();
    }
};

Notice that that return Base::eliminatable(); is influenced by pybind11::gil_scoped_acquire gil;; but if Annotation::Base().eliminatable is itself threadsafe this function should not need to be protected by the GIL. In that case, this can be modified in one of 2 ways:

1. If binder does not need the GIL locked to do the logic before eliminatable() the GIL need not be acquired.
2. If binder does require the GIL locked for those intermediate lines, it can be released before the return statement such as:

struct PyCallBack_Annotation_Base : public Annotation::Base {
    using Annotation::Base::Base;

    bool eliminatable() const override {
        { // Create a new scope
            pybind11::gil_scoped_acquire gil;
            pybind11::function overload = pybind11::get_overload(static_cast<const Annotation::Base *>(this), "eliminatable");
            if (overload) {
                auto o = overload.operator()<pybind11::return_value_policy::reference>();
                if (pybind11::detail::cast_is_temporary_value_reference<bool>::value) {
                    static pybind11::detail::override_caster_t<bool> caster;
                    return pybind11::detail::cast_ref<bool>(std::move(o), caster);
                }
                else return pybind11::detail::cast_safe<bool>(std::move(o));
            }
        } // Release GIL
        return Base::eliminatable();
    }
};

[lyskov]

but if Annotation::Base().eliminatable is itself threadsafe this function should not need to be protected by the GIL

-- @zwimer could you please elaborate on this? Particularly why should we assume that Annotation::Base().eliminatable is threadsafe? The way i see it: it is indeed a C++ native call, but this does not mean it could be call some Python code inside (for example thorough a callback). Thanks,

[zwimer]

Oh! My mistake, I misread the documentation in pybind11 to say the GIL is not held by default: https://pybind11.readthedocs.io/en/stable/advanced/misc.html?highlight=GIL#global-interpreter-lock-gil

In that case (perhaps this is better for a different issue?), would it be reasonable to feature request a 'do not hold the GIL' option in binder? Some libraries are designed to be thread-safe and do not need the GIL / actively do not want the GIL held. This could be a global option or a per-class / per-function / per-namespace option.

[lyskov]

Interesting idea @zwimer ! I think this should be possible to do on the class level or/and namespace level. Let me think about how is it best to handle this.