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

[mosra]

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!

[mosra]

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?

[rwgk]

I need to look at this more carefully and think about it.
First impression:

• we're building a surprising bypass here for the sake of performance.
• it employs a side effect: the wrapped function manipulating the global Python error state.
  We're definitely setting ourself up for quicksand experiences: people thinking they are on firm ground, but there is a bug elsewhere that makes the Python error state invalid, but it only manifests itself through this bypass with an off-topic error that could take hours of debugging to understand. (I've been in similar situations many times.)
  Is the benefit of this optimization so big that we're ok accepting that danger ("at scale" it is pretty much certain that people will run into it)?

[rwgk]

Maybe I should add: if performance really matters, why do you have this code path in your loop?
Of course it's nice if it's fast.
But there is an opportunity cost to optimizations like this PR: things get more complicated and fragile.
Now we're in the realm of philosophy:

• Keep pybind11 lean and straightforward so that it is as good as it can be at wrapping with ease.
• Tell users: if you have a performance critical loop, put your data into a C++ array (that you can easily wrap) and have your performance critical loop in C++, taking Python completely out of that loop, literally.

So there are clear "performance really doesn't matter" and "performance really does matter" regimes where everybody is in agreement and happy, but a gray area in the middle, where it's not quite important enough for users to bite the bullet and code up the C++ array they'd need.
What's better globally over a long period of time for all users in aggregate?

• Having N bypasses like this in pybind11 to get more of that gray area?
• A leaner and less fragile pybind11?

[rwgk]

Tagging #2760 and @jblespiau who is maybe more on the N bypasses side of the spectrum than I am.

[Skylion007]

@rwgk This would also allow better error messages from type casters.

[rwgk]

Drive-by response:

This would also allow better error messages from type casters.

That would change my cost-benefit equation (previously I was only seeing "performance" as the benefit).

A PR that generates better error messages, based on this PR, may be convincing.

[jblespiau]

Tagging #2760 and @jblespiau who is maybe more on the N bypasses side of the spectrum than I am.

The bypass I was suggesting in the past was mostly about not performing a dynamic look-up every time we do a C++ to Python or Python to C++ conversion. I don't really consider this a "bypass", but more of a cache (if we know the C++ Type "A" maps to a given Python type, no need to do the look-up in the hash-map every single time to retrieve the same object again and again. It's even more obvious when you convert an std::vector<A>, you will do as many look-ups as there are items in the vector. I was suggesting to add the keyword "static" on the look-up line, and you get a huge speed-up. The only possible issue is if people unregister and re-register new types on the same symbol, but I don't even think it's possible (you cannot unregister a symbol as far as I know).
Another difference with the current CL is that I was suggesting to improve the normal path, not an exceptional one.

Specifically for the current PR, I would probably need to think more, but I am thinking that:

• Exceptions are known to be slow in Python. Exceptions should be exceptional, and not be the standard way to return something.
• Most of the time, after an exception, we crash.
• Given they are expected to be exceptional, why do we want to optimize it in the first place?

So I am wondering (a) why try to optimize something that should not happen in normal circumstances, (b) and also why, according to the benchmark, it has any impact on the non-raising path : if no exception is raised, it shouldn't change the runtime, right?

[Skylion007]

@rwgk I really like this handling though because it adds an extra escape hatch in cases where the user may forget to throw an error. Also, while the PyErr_SetString is exceptional, and really shouldn't be used except for advanced usages, the throwing an error when returning null to Python makes sense and could catch errors.

[rwgk]

extra escape hatch

Such things cause more surprises (and a lot of lost human time (to contrast that with machine optimization)) than one would think.

I don't feel comfortable with this change, but if it gets enough support from others I'll be fine with it.

[Skylion007]

Could also simplify issues like: #3336

[Skylion007]

Also would fix: #3600 . Currently, we have no way of raising anything other than a Type Exception in a caster. Even being able to specify the string of the type exception would help alot @rwgk