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.

Sign up for GitHub

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

pybind11::handle inc_ref() & dec_ref() PyGILState_Check() **excluding** nullptr #4246

Merged
merged 3 commits into from
Dec 9, 2022
Merged

pybind11::handle inc_ref() & dec_ref() PyGILState_Check() **excluding** nullptr #4246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8699639
pybind11/pytypes.h `inc_ref()`, `dec_ref()` `PyGILState_Check()` **ex…
Oct 17, 2022
6d202aa
Disable `PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF` for PyPy under Windows.
Oct 17, 2022
3115440
Add reference to PR #4268 (PyPy Windows)
Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,15 @@
# define PYBIND11_HAS_U8STRING
#endif

// See description of PR #4246:
#if !defined(NDEBUG) && !defined(PY_ASSERT_GIL_HELD_INCREF_DECREF) \
&& !(defined(PYPY_VERSION) \
&& defined(_MSC_VER)) /* PyPy Windows: pytest hangs indefinitely at the end of the \
process (see PR #4268) */ \
&& !defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF)
# define PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF
#endif

// #define PYBIND11_STR_LEGACY_PERMISSIVE
// If DEFINED, pybind11::str can hold PyUnicodeObject or PyBytesObject
// (probably surprising and never documented, but this was the
Expand Down
10 changes: 10 additions & 0 deletions include/pybind11/pytypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,11 @@ class handle : public detail::object_api<handle> {
const handle &inc_ref() const & {
#ifdef PYBIND11_HANDLE_REF_DEBUG
inc_ref_counter(1);
#endif
#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF)
if (m_ptr != nullptr && !PyGILState_Check()) {
throw std::runtime_error("pybind11::handle::inc_ref() PyGILState_Check() failure.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason no pybind11_fail? Because of the corresponding !PyErrOccurred() assertion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
But even if that was taken out in pybind11_fail(), I think a "raw" C++ exception is best here, we want the process to terminate as directly as possible. I was thinking of using std::terminate() directly, but then it is troublesome to also produce the message, so I decided simpler is better. In our environment at least, std::runtime_error() works out very nicely (useful traceback with message).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, functions that can raise exceptions do come with some subtle costs in C++. The compiler needs to keep track of the many ways in which might have to destroy objects if an exception arises at various different points of the program and generate code for each one. I don't think it's a performance issue, but this will likely make the debug binaries bigger (compared to pybind11_fail) given that such a central operation is affected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrow focus:

Currently pybind11_fail() includes this:

assert(!PyErr_Occurred());

That needs the GIL, but here (handle) we just determined that we don't have it.

Bigger picture:

There is a general tradeoff: a faster or leaner binary today vs a future with more features.

Boundary condition: human time is ~constant.

So ultimately we always trade one for the other.

If you hire person A who likes to optimize, you may get something that runs in 1.0 time units with 1.0 resources, with 10 features.

If you hire person B, who's more like me, you get something that runs in 1.1 time units with 1.1 resources, with 20 features.

What's "better"?

It depends.

}
#endif
Py_XINCREF(m_ptr);
return *this;
Expand All @@ -257,6 +262,11 @@ class handle : public detail::object_api<handle> {
this function automatically. Returns a reference to itself.
\endrst */
const handle &dec_ref() const & {
#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF)
if (m_ptr != nullptr && !PyGILState_Check()) {
throw std::runtime_error("pybind11::handle::dec_ref() PyGILState_Check() failure.");
}
#endif
Py_XDECREF(m_ptr);
return *this;
}
Expand Down