-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: add pybind11/gil_safe_call_once.h (to fix deadlocks in pybind11/numpy.h) #4877
Merged
Merged
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
38317c3
LazyInitializeAtLeastOnceDestroyNever v1
e7b8c4f
Go back to using `union` as originally suggested by jbms@. The trick …
74ac0d9
Revert "Go back to using `union` as originally suggested by jbms@. Th…
109a165
Remove `#include <stdalign.h>`
88cec11
Suppress gcc 4.8.5 (CentOS 7) warning.
1ce2715
Replace comments:
e7be9c2
Adopt suggestion by @tkoeppe:
f07b28b
Add `PYBIND11_CONSTINIT`, but it does not work for the current use ca…
36be645
Revert "Add `PYBIND11_CONSTINIT`, but it does not work for the curren…
7bc16a6
Reapply "Add `PYBIND11_CONSTINIT`, but it does not work for the curre…
78f4e93
Add Default Member Initializer on `value_storage_` as suggested by @t…
6d9441d
Fix copy-paste-missed-a-change mishap in commit 88cec1152ab5576db19ba…
a864f21
Semi-paranoid placement new (based on https://github.com/pybind/pybin…
6689b06
Move PYBIND11_CONSTINIT to detail/common.h
d965f29
Move code to the right places, rename new class and some variables.
398a42c
Fix oversight: update tests/extra_python_package/test_files.py
ab2cf8e
Get the name right first.
6d5bdd8
Use `std::call_once`, `std::atomic`, following a pattern developed by…
4c5dd1b
Make the API more self-documenting (and possibly more easily reusable).
8633c5b
google-clang-tidy IWYU fixes
82f3efc
Rewrite comment as suggested by @tkoeppe
3ebd139
Update test_exceptions.cpp and exceptions.rst
c33712d
Fix oversight in previous commit: add `PYBIND11_CONSTINIT`
dcf2b92
Make `get_stored()` non-const for simplicity.
4557dce
Add comment regarding `KeyboardInterrupt` behavior, based heavily on …
704fe13
Add `assert(PyGILState_Check())` in `gil_scoped_release` ctor (simple…
b2f87a8
Fix oversight in previous commit (missing include cassert).
fad1017
Remove use of std::atomic, leaving comments with rationale, why it is…
8453302
Rewrite comment re `std:optional` based on deeper reflection (aka 2nd…
66bbc67
Additional comment with the conclusion of a discussion under PR #4877.
7cd9390
Small comment changes suggested by @tkoeppe.
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Remove use of std::atomic, leaving comments with rationale, why it is…
… not needed.
- Loading branch information
There are no files selected for viewing
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this a std::optional and avoid both the atomic flag and the storage here.
You don't need the atomic since call_once is an inherent mutex that asserts ordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the access inside the
call_once
that the atomic is for, but rather, the earlier access (the first check for the fast path).However, I think you have a point that because we assume that the GIL is held on function entry, the boolean doesn't need to be atomic: the GIL serializes access to it (both read and write). The code with the atomic variable would work even without any external synchronisation, but since we already have the GIL on entry, I agree that we should be able to use a non-atomic boolean.
(And then the boolean + placement new could be replaced by std::optional, indeed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And just to make this point clearly: the first, outer boolean check provides a fast path on which we don't interact with the GIL any further. We only do the release and reacquire dance on the (hopefully rare) slow path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Thomas! I'll definitely make the change to the simple
bool
then.Except that pybind11 still supports C++11, and
std::optional
was added only with C++17.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case leave it as bool + placement new :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, and also we do actually want the "constant-initializable, trivial destructible" behaviour of bool + placement new. Not only do we not want to destroy this object, but this also gives us cheaper static variables (no static guard, and no global destructructor list entry).