From 60ebd78fe9c61cf1bd1d8117dca25660980a6577 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 21 Jul 2019 22:26:02 +0200 Subject: [PATCH 1/2] Allow a Python exception to be raised without throwing. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While probably not a big deal in general code, this might result in significant speedups in critical code paths, such as raising an IndexError in __getitem__() implementations to stop iterating -- for example conversion of small arrays or math vector classes to lists (or np.array, for example). Measuring with Magnum's Python bindings and the timeit module, the numbers were as follows (Release build, GCC 9, Py3.7): Vector3(1.0, 2.0, 3.0) 0.54144 µs list(Vector3(1.0, 2.0, 3.0)) 5.25274 µs Vector3()[0] # doesn't throw 0.97913 µs Vector3()[3] # throws 5.78696 µs raise IndexError() # throws 0.15339 µs After this patch and a change where Vector3's __getitem__ calls PyErr_SetString() and returns an empty py::object instead of throwing py::index_error, the numbers went down considerably: Vector3(1.0, 2.0, 3.0) 0.55200 µs list(Vector3(1.0, 2.0, 3.0)) 1.60032 µs Vector3()[0] # doesn't throw 0.68562 µs Vector3()[3] # throws 0.84481 µs raise IndexError() # throws 0.17830 µs To be extra sure about the impact, I also tried PyErr_SetString() together with throwing py::error_already_set as that's also less code being executed, but that led only to comparably minor improvement (~4 µs down from 5.8). Important thing to note is that for conversion to np.array and friends, implementing a buffer protocol is far more performant solution (under a microsecond compared to around 6 µs when relying on this patch and almost 20 µs when throwing py::index_error). So this change is mainly aimed at list conversions and general iteration. --- include/pybind11/pybind11.h | 18 +++++++++++++----- tests/test_exceptions.cpp | 8 ++++++++ tests/test_exceptions.py | 26 ++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index fbd971cfb1..755e8081b6 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -769,11 +769,19 @@ class cpp_function : public function { PyErr_SetString(PyExc_TypeError, msg.c_str()); return nullptr; } else if (!result) { - std::string msg = "Unable to convert function return value to a " - "Python type! The signature was\n\t"; - msg += it->signature; - append_note_if_missing_header_is_suspected(msg); - PyErr_SetString(PyExc_TypeError, msg.c_str()); + /* Allow the user to raise a Python exception directly using + PyErr_SetString() together with returning a null py::object, as + that may be significantly faster than throwing a C++ exception + in critical code paths. In that case we arrive here with + non-null PyErr_Occurred(), so keep that exception instead of + overwriting it with another. */ + if (!PyErr_Occurred()) { + std::string msg = "Unable to convert function return value to " + "a Python type! The signature was\n\t"; + msg += it->signature; + append_note_if_missing_header_is_suspected(msg); + PyErr_SetString(PyExc_TypeError, msg.c_str()); + } return nullptr; } else { if (overloads->is_constructor && !self_value_and_holder.holder_constructed()) { diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index d30139037f..f6d0376310 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -171,6 +171,14 @@ TEST_SUBMODULE(exceptions, m) { throw py::error_already_set(); }); + m.def("raise_without_throw", [](bool return_null, bool set_error) { + if (set_error) + PyErr_SetString(PyExc_FutureWarning, "this is a robbery!"); + if (return_null) + return py::object{}; + return py::cast(5); + }, py::arg("return_null"), py::arg("set_error")); + m.def("python_call_in_destructor", [](py::dict d) { try { PythonCallInDestructor set_dict_in_destructor(d); diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 6edff9fe4b..0b8a3c87f9 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -20,6 +20,32 @@ def test_error_already_set(msg): assert msg(excinfo.value) == "foo" +def test_raise_without_throw(msg): + # Not setting an error and returning a non-null object + assert m.raise_without_throw(return_null=False, set_error=False) == 5 + + # Setting an error and returning a null object is an allowed alternative to + # throwing a C++ exception + with pytest.raises(FutureWarning) as excinfo: + m.raise_without_throw(return_null=True, set_error=True) + assert msg(excinfo.value) == "this is a robbery!" + + # Setting an error and returning a non-null object is a Python system error + with pytest.raises(SystemError) as excinfo: + m.raise_without_throw(return_null=False, set_error=True) + assert "returned a result with an error set" in str(excinfo.value) + + # Returning a null object without error being set is not allowed either, as + # that's also the case when function return value can't be converted to a + # Python type + with pytest.raises(TypeError) as excinfo: + m.raise_without_throw(return_null=True, set_error=False) + assert msg(excinfo.value) == ( + "Unable to convert function return value to a Python type! The " + "signature was\n\t(return_null: bool, set_error: bool) -> object" + ) + + def test_cross_module_exceptions(): with pytest.raises(RuntimeError) as excinfo: cm.raise_runtime_error() From 91de6578f1d0253f5ae71a03f7134f7d401d287d Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Sun, 8 Aug 2021 16:28:18 -0400 Subject: [PATCH 2/2] Try to relax test for X-platform --- tests/test_exceptions.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 3e93aa5c17..1fb6cb502f 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -36,9 +36,9 @@ def test_raise_without_throw(msg): assert msg(excinfo.value) == "this is a robbery!" # Setting an error and returning a non-null object is a Python system error - with pytest.raises(SystemError) as excinfo: - m.raise_without_throw(return_null=False, set_error=True) - assert "returned a result with an error set" in str(excinfo.value) + if not env.PY2: + with pytest.raises(SystemError) as excinfo: + m.raise_without_throw(return_null=False, set_error=True) # Returning a null object without error being set is not allowed either, as # that's also the case when function return value can't be converted to a