From 637f02d52ea7aae74160f08eb9fdaf1c9c026720 Mon Sep 17 00:00:00 2001 From: Dustin Spicuzza Date: Sat, 4 Nov 2023 13:31:56 -0400 Subject: [PATCH] Add gilsafe_object to simplify storing python objects in containers --- robotpy_build/include/gilsafe_object.h | 137 ++++++++++++++++++ tests/cpp/pyproject.toml.tmpl | 1 + tests/cpp/rpytest/ft/__init__.py | 2 + .../rpytest/ft/include/gilsafe_container.h | 27 ++++ tests/test_ft_misc.py | 9 ++ 5 files changed, 176 insertions(+) create mode 100644 robotpy_build/include/gilsafe_object.h create mode 100644 tests/cpp/rpytest/ft/include/gilsafe_container.h diff --git a/robotpy_build/include/gilsafe_object.h b/robotpy_build/include/gilsafe_object.h new file mode 100644 index 0000000..e096e46 --- /dev/null +++ b/robotpy_build/include/gilsafe_object.h @@ -0,0 +1,137 @@ + +#pragma once + +#include + +namespace py = pybind11; + +// Py_IsFinalizing is public API in 3.13 +#if PY_VERSION_HEX < 0x03130000 +#define Py_IsFinalizing _Py_IsFinalizing +#endif + +namespace rpy { + +/* + This object holds a python object, and can be stored in C++ containers that + aren't pybind11 aware. + + It is very inefficient -- it will acquire and release the GIL each time + a move/copy operation occurs! You should only use this object type as a + last resort. + + Assigning, moves, copies, and destruction acquire the GIL; only converting + this back into a python object requires holding the GIL. +*/ +class gilsafe_object final { + py::object o; + +public: + + // + // These operations require the caller to hold the GIL + // + + // copy conversion + operator py::object() const & { + return o; + } + + // move conversion + operator py::object() const && { + return std::move(o); + } + + // + // These operations do not require the caller to hold the GIL + // + + gilsafe_object() = default; + + ~gilsafe_object() { + if (o) { + // If the interpreter is alive, acquire the GIL, otherwise just leak + // the object to avoid a crash + if (!Py_IsFinalizing()) { + py::gil_scoped_acquire lock; + o.dec_ref(); + } + + o.release(); + } + } + + // Copy constructor; always increases the reference count + gilsafe_object(const gilsafe_object &other) { + py::gil_scoped_acquire lock; + o = other.o; + } + + // Copy constructor; always increases the reference count + gilsafe_object(const py::object &other) { + py::gil_scoped_acquire lock; + o = other; + } + + gilsafe_object(const py::handle &other) { + py::gil_scoped_acquire lock; + o = py::reinterpret_borrow(other); + } + + // Move constructor; steals object from ``other`` and preserves its reference count + gilsafe_object(gilsafe_object &&other) noexcept : o(std::move(other.o)) {} + + // Move constructor; steals object from ``other`` and preserves its reference count + gilsafe_object(py::object &&other) noexcept : o(std::move(other)) {} + + // copy assignment + gilsafe_object &operator=(const gilsafe_object& other) { + if (!o.is(other.o)) { + py::gil_scoped_acquire lock; + o = other.o; + } + return *this; + } + + // move assignment + gilsafe_object &operator=(gilsafe_object&& other) noexcept { + if (this != &other) { + py::gil_scoped_acquire lock; + o = std::move(other.o); + } + return *this; + } + + explicit operator bool() const { + return (bool)o; + } +}; + +} // namespace rpy + + + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +template <> +struct type_caster { + bool load(handle src, bool convert) { + value = src; + return true; + } + + static handle cast(const handle &src, return_value_policy /* policy */, handle /* parent */) { + return src.inc_ref(); + } + + PYBIND11_TYPE_CASTER(rpy::gilsafe_object, const_name("object")); +}; + +template <> +struct handle_type_name { + static constexpr auto name = const_name("object"); +}; + +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/tests/cpp/pyproject.toml.tmpl b/tests/cpp/pyproject.toml.tmpl index 70f6bb8..ee517e1 100644 --- a/tests/cpp/pyproject.toml.tmpl +++ b/tests/cpp/pyproject.toml.tmpl @@ -53,6 +53,7 @@ generate = [ { enums = "enums.h" }, { factory = "factory.h" }, { fields = "fields.h" }, + { gilsafe_container = "gilsafe_container.h" }, { keepalive = "keepalive.h" }, { ignore = "ignore.h" }, { ignored_by_default = "ignored_by_default.h" }, diff --git a/tests/cpp/rpytest/ft/__init__.py b/tests/cpp/rpytest/ft/__init__.py index 043e8c7..3057fc0 100644 --- a/tests/cpp/rpytest/ft/__init__.py +++ b/tests/cpp/rpytest/ft/__init__.py @@ -20,6 +20,7 @@ GCEnum, GEnum, GEnumMath, + GilsafeContainer, HasFactory, HasOperator, HasOperatorNoDefault, @@ -135,6 +136,7 @@ "GCEnum", "GEnum", "GEnumMath", + "GilsafeContainer", "HasFactory", "HasOperator", "HasOperatorNoDefault", diff --git a/tests/cpp/rpytest/ft/include/gilsafe_container.h b/tests/cpp/rpytest/ft/include/gilsafe_container.h new file mode 100644 index 0000000..ee06c85 --- /dev/null +++ b/tests/cpp/rpytest/ft/include/gilsafe_container.h @@ -0,0 +1,27 @@ + +#pragma once + +#include + +class GilsafeContainer { + rpy::gilsafe_object m_o; +public: + void assign(rpy::gilsafe_object o) { + m_o = o; + } + + static void check() { + auto c = std::make_unique(); + + py::gil_scoped_acquire a; + + py::object v = py::none(); + + { + py::gil_scoped_release r; + c->assign(v); + c.reset(); + } + } + +}; diff --git a/tests/test_ft_misc.py b/tests/test_ft_misc.py index 572f712..77193ef 100644 --- a/tests/test_ft_misc.py +++ b/tests/test_ft_misc.py @@ -130,6 +130,15 @@ def test_factory(): assert o.m_x == 5 +# +# gilsafe_container.h +# + + +def test_gilsafe_container(): + ft.GilsafeContainer.check() + + # # inline_code.h #