Skip to content
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

Allow a Python exception to be raised without throwing for improved performance #1853

Open
wants to merge 1 commit into
base: master
from

Conversation

@mosra
Copy link
Contributor

commented Jul 21, 2019

While probably not a big deal in general code, this patch might result in significant speedups in critical code paths, such as raising an IndexError in __getitem__() implementations to stop iterating -- for example iterating over / conversion of small arrays or math vector classes to lists (or to np.array).

Measuring with Magnum's Python bindings and the timeit module, the numbers before this were as follows (Release build, GCC 9, Py3.7, benchmark code for reference):

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 a 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, as numpy seems to be hitting the boundary condition three times when converting from iterables). So this change is mainly aimed at list conversions and general iteration where perf matters.


This is my first real PR to this project and while I tried to follow code style and added a test case, I most probably missed something important (even though I don't think this particular change should break anyone's use case) :) Let me know if you need more information or more testing / benchmarks. Happy to address your feedback. Thank you!

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.
@mosra mosra force-pushed the mosra:raise-exception-without-throwing branch from 9ee28a0 to 60ebd78 Jul 21, 2019
@mosra

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

This PR makes two tests fail, both expecting the Unable to convert function return value to a Python type! error, but now getting the original TypeError('Unregistered type : std::vector<float, std::allocator<float> >') or TypeError('Unregistered type : test_submodule_class_(module&)::NotRegistered') set by code executed earlier.

I'd argue this is fine (the exception still points directly to the problem) and the expected exceptions in those two tests should be updated. Furthermore this might mean that the append_note_if_missing_header_is_suspected lambda could be moved to the place where the original Unregistered type error gets set and then the Unable to convert function return value code path is not needed at all? Or is there another case where this could be hit without some pre-existing error being set?

superbobry added a commit to superbobry/pybind11 that referenced this pull request Aug 28, 2019
Prior to this commit throwing error_already_set was expensive due to the
eager construction of the error string (which required traversing the
Python stack). See pybind#1853 for more context and an alternative take on the
issue.
superbobry added a commit to superbobry/pybind11 that referenced this pull request Aug 28, 2019
Prior to this commit throwing error_already_set was expensive due to the
eager construction of the error string (which required traversing the
Python stack). See pybind#1853 for more context and an alternative take on the
issue.
superbobry added a commit to superbobry/pybind11 that referenced this pull request Aug 28, 2019
Prior to this commit throwing error_already_set was expensive due to the
eager construction of the error string (which required traversing the
Python stack). See pybind#1853 for more context and an alternative take on the
issue.

Note that error_already_set no longer inherits from std::runtime_error
because the latter has no default constructor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.