Add release_gil_before_calling_cpp_dtor annotation for class_
#30088
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rainwoodman
reviewed
Jan 18, 2024
include/pybind11/pybind11.h
Outdated
| } else { | ||
| detail::call_operator_delete( | ||
| v_h.value_ptr<type>(), v_h.type->type_size, v_h.type->type_align); | ||
| PyThreadState *py_ts = release_gil_before_calling_cpp_dtor ? PyEval_SaveThread() : nullptr; |
There was a problem hiding this comment.
What about py::gil_scoped_acquire?
There was a problem hiding this comment.
Thanks for the nudge! This got me into some useful back and forth experimenting. The end result I settled on is functionally equivalent to what you saw before, but the code organization is better I think (avoids the bool release_gil_before_calling_cpp_dtor). I added comments to explain why I'm not using gil_scoped_release, and that the catch (...) isn't expected to be reachable.
rainwoodman
reviewed
Jan 18, 2024
include/pybind11/pybind11.h
Outdated
| // std::terminate(). | ||
| error_scope scope; | ||
| // Deallocates an instance; via holder, if constructed; otherwise via operator delete. | ||
| // We could be deallocating because we are cleaning up after a Python exception. |
There was a problem hiding this comment.
We no longer clear the error scope in this function. Consider move the comment.
There was a problem hiding this comment.
I rewrote the comment, so that this is first:
// NOTE: The Python error indicator needs to cleared BEFORE this function is called.
rainwoodman
reviewed
Jan 18, 2024
| static void dealloc_release_gil_before_calling_cpp_dtor(detail::value_and_holder &v_h) { | ||
| error_scope scope; | ||
| // Intentionally not using `gil_scoped_release` because the non-simple | ||
| // version unconditionally calls `get_internals()`. |
rainwoodman
approved these changes
Jan 18, 2024
|
NOT waiting for GitHub Actions to finish: 1. The only change to before is a rewrite of a source code comment. 2. The only prior CI failure is a frequent flake. |
rwgk
added a commit
to google/clif
that referenced
this pull request
Jan 22, 2024
For easy direct access: * google/pybind11clif#30034 * google/pybind11clif#30087 * google/pybind11clif#30088 Note regarding the change in std_containers_copy_move_test.py: When the test was added (cl/570839777), the undocumented expectation was that it is hyper-sensitive, but that it will be adjusted or made more permissive (similar to e.g. google3/third_party/clif/testing/python/return_value_policy_test.py;l=25;rcl=534200687) as needed. #MIGRATION_3P_PYBIND11__DEFAULT ``` - 24a3b1c3729401ca661efacfbbd13a83117e1bae Add `case return_value_policy::_clif_automatic` in type_c... by Ralf W. Grosse-Kunstleve <rwgk@google.com> - 015834b13a99d233c15406c36a3a44d27ebfc846 Change `array_caster` in pybind11/stl.h to support value ... by Ralf W. Grosse-Kunstleve <rwgk@google.com> - ce00785ef877f0bb1dba10115f00366111583b3c Add `release_gil_before_calling_cpp_dtor` annotation for ... by Ralf W. Grosse-Kunstleve <rwgk@google.com> ``` PiperOrigin-RevId: 599883612
rwgk
added a commit
to google/clif
that referenced
this pull request
Jan 22, 2024
This one-line production code change builds on google/pybind11clif#30088 (imported via cl/599883612). PyCLIF-pybind11 TGP failures fixed: b/287289622#comment51 (21 Passing, 20 unique) GitHub testing: #87 PiperOrigin-RevId: 600509355
rwgk
added a commit
to rwgk/pybind11
that referenced
this pull request
Feb 15, 2025
…if#30092 (minor fixes). Note for completeness: These are identical to the current versions on the pybind11clif main branch (@ commit 4841661df5daf26ecdedaace54e64d0782e63f64): * test_class_release_gil_before_calling_cpp_dtor.cpp * test_class_release_gil_before_calling_cpp_dtor.py
rwgk
added a commit
to pybind/pybind11
that referenced
this pull request
Feb 16, 2025
) * Backport of google/pybind11clif#30088 (main PR) and google/pybind11clif#30092 (minor fixes). Note for completeness: These are identical to the current versions on the pybind11clif main branch (@ commit 4841661df5daf26ecdedaace54e64d0782e63f64): * test_class_release_gil_before_calling_cpp_dtor.cpp * test_class_release_gil_before_calling_cpp_dtor.py * Fix potential data race in test_class_release_gil_before_calling_cpp_dtor.cpp The original intent was to let the singleton leak, but making that tread-safe is slightly more involved than this solution. It's totally fine in this case if the RegistryType destructor runs on process teardown. See also: #5522 (comment) --------- Co-authored-by: Ralf W. Grosse-Kunstleve <rwgk@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The primary motivation for this PR is to support PyCLIF-pybind11: this PR fixes about 20 failures when testing PyCLIF-pybind11 Google-globally.
However, more generally, it also solves pybind/pybind11#1446.
Background:
PyCLIF-C-API releases the GIL when calling the destructor of a wrapped C++ object (this cannot be disabled by the user).
pybind11 does not.
Relevant code:
PyCLIF-C-API: clif/python/gen.py
pybind11: pybind11/pybind11.h
An attempt was made to remove the GIL release code from the PyCLIF-C-API gen.py code linked above, but Google-global testing made it clear that this is infeasible. There are too many use cases that critically rely on the current PyCLIF behavior. Reviewing the use cases made it obvious that pushing the GIL-release code into user code is impractical.
In contrast, adding the GIL-release feature as an option into pybind11 turned out to be relatively straightforward:
The core change (in pybind11.h
class_::dealloc()) is to add ReleaseGIL-try-catch-ReaquireGIL-rethrow logic around the existing code calling the C++ destructor of wrapped objects.All the rest is more-or-less boilerplate code to add the
py:: release_gil_before_calling_cpp_dtorannotation, then use it to enable the new feature if that annotation is provided by the user.PyCLIF-pybind11 can then simply use the annotation to replicate the established behavior of PyCLIF-C-API.
Suggested changelog entry: