-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add per-interpreter storage for gil_safe_call_once_and_store
#5933
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
Open
XuehaiPan
wants to merge
65
commits into
pybind:master
Choose a base branch
from
XuehaiPan:subinterp-call-once-and-store
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,056
−240
Open
Changes from all commits
Commits
Show all changes
65 commits
Select commit
Hold shift + click to select a range
729654c
Add new argument to `gil_safe_call_once_and_store::call_once_and_stor…
XuehaiPan d2b7605
Add per-interpreter storage for `gil_safe_call_once_and_store`
XuehaiPan e741760
Make `~gil_safe_call_once_and_store` a no-op
XuehaiPan 5d1d678
Fix C++11 compatibility
XuehaiPan 0bac82d
Improve thread-safety and add default finalizer
XuehaiPan 2a4b118
Merge branch 'master' into subinterp-call-once-and-store
XuehaiPan be97110
Try fix thread-safety
XuehaiPan 3e77ce9
Try fix thread-safety
XuehaiPan d5b8813
Add a warning comment
XuehaiPan f6d0f88
Simplify `PYBIND11_INTERNALS_VERSION >= 12`
XuehaiPan 7d8339e
Try fix thread-safety
XuehaiPan 1920f43
Try fix thread-safety
XuehaiPan 900bed6
Merge branch 'master' into subinterp-call-once-and-store
XuehaiPan a6754ba
Revert get_pp()
XuehaiPan 1aed3ab
Update comments
XuehaiPan b61e902
Move call-once storage out of internals
XuehaiPan b72cd41
Revert internal version bump
XuehaiPan ac02a32
Cleanup outdated comments
XuehaiPan ddb6dd4
Move atomic_bool alias into pybind11::detail namespace
rwgk 3fb52df
Add explicit #include <unordered_map> for subinterpreter support
rwgk 32deca4
Remove extraneous semicolon after destructor definition
rwgk a4d4d73
Add comment explaining unused finalize parameter
rwgk 7cb30ce
Add comment explaining error_scope usage
rwgk 7d34139
Improve exception safety in get_or_create_call_once_storage_map()
rwgk 78e3945
Add timeout-minutes: 3 to cpptest workflow steps
rwgk 1014ee4
Add progress reporter for test_with_catch Catch2 runner
rwgk 21d0dc5
clang-format auto-fix (overlooked before)
rwgk e1b1b1b
Disable "Move Subinterpreter" test on free-threaded Python 3.14+
rwgk 89cae6d
style: pre-commit fixes
pre-commit-ci[bot] a090637
Add test for gil_safe_call_once_and_store per-interpreter isolation
rwgk cb5e7d7
Add STARTING/DONE timestamps to test_with_catch output
rwgk 0f8f32a
Disable stdout buffering in test_with_catch
rwgk a3abdee
EXPERIMENT: Re-enable hanging test to verify CI log buffering fix
rwgk d6f2a7f
Revert "Disable stdout buffering in test_with_catch"
rwgk 9b70460
Use USES_TERMINAL for cpptest to show output immediately
rwgk 8951004
Fix clang-tidy performance-avoid-endl warning
rwgk c4cbe73
Add SIGTERM handler to show when test is killed by timeout
rwgk f330a79
Fix typo: atleast -> at_least
rwgk 6c1ccb9
Fix GCC warn_unused_result error for write() in signal handler
rwgk 3c01ff3
Add USES_TERMINAL to other C++ test targets
rwgk 9e9843d
Revert "EXPERIMENT: Re-enable hanging test to verify CI log buffering…
rwgk e7c2648
Update comment to reference PR #5940 for Move Subinterpreter fix
rwgk 58c08ac
Add alias `interpid_t = std::int64_t`
XuehaiPan 305a293
Add isolation and gc test for `gil_safe_call_once_and_store`
XuehaiPan f6bba0f
Add thread local cache for gil_safe_call_once_and_store
XuehaiPan 66e4697
Revert "Add thread local cache for gil_safe_call_once_and_store"
XuehaiPan d0819cc
Revert changes according to code review
XuehaiPan 5ce00e5
Relocate multiple-interpreters tests
XuehaiPan 97b50fe
Add more tests for multiple interpreters
XuehaiPan 8819ec4
Remove copy constructor
XuehaiPan e84e9c1
Merge remote-tracking branch 'upstream/master' into subinterp-call-on…
XuehaiPan d9daef5
Apply suggestions from code review
XuehaiPan 9a3328b
Refactor to use per-storage capsule instead
XuehaiPan b68faf0
Merge remote-tracking branch 'upstream/master' into subinterp-call-on…
XuehaiPan bc20601
Update comments
XuehaiPan b39c049
Update singleton tests
XuehaiPan 9ef71ec
Use interpreter id type for `get_num_interpreters_seen()`
XuehaiPan 98370f2
Suppress unused variable warning
XuehaiPan 534235e
HACKING
XuehaiPan d038714
Revert "HACKING"
XuehaiPan 3a2c34a
Try fix concurrency
XuehaiPan 99a095d
Test even harder
XuehaiPan 7daecd7
Reorg code to avoid duplicates
XuehaiPan cd950dc
Fix unique_ptr::reset -> unique_ptr::release
XuehaiPan db22bb4
Merge remote-tracking branch 'upstream/master' into subinterp-call-on…
XuehaiPan 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
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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.
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.
If
internals_pp_managercan do it, why can't we?Should the map be a member of
local_internalsso we can benefit from the caching and finalization logic that already exists there? We can add things to local internals without bumping the internals version.Uh oh!
There was an error while loading. Please reload this page.
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 think there are still some bugs with the
internals_pp_manager.internalsandlocal_internalsare not reliable for now. There is still some work that needs to be done to ensure it works correctly under the multiple-interpreters case. So I added this warning comment, and maybe we can do some improvement in the future. The first priority for this PR is to make things work right.For example, in the comment #5933 (comment) and the CI failure https://github.com/pybind/pybind11/actions/runs/20414341520?pr=5933,
py::native_enum<...>inimport modcomplains that the enum type is already registered. However, the registration map is stored ininternalsandlocal_internals, which should be per-interpreter dependent.pybind11/include/pybind11/native_enum.h
Lines 31 to 40 in 3aeb113
This indicates there exist bugs in
internals_pp_manager. The bugs are not related togil_safe_call_once_and_store.I suspect that the
unrefanddestroyhooks forinternal_pp_manageronly work for interpreters created by thepybind11::subinterpreterAPI. The tls storage will be malformed when the interpreters are created/destroyed from the Python side. For example, usingconcurrent.interpretersandconcurrent.futures.InterpreterPoolExecutorandinternal_pp_manageris not aware that the interpreter is destroyed from the Python side.cc @b-pass
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.
That does seem likely. Can we fix it for everyone, instead of working around it for this specific case while leaving a problem in other areas? I believe everything would work out if we do per-interpreter cleanup from the internals capsule destructor, since clearing the interpreter state dict is one of the last actions on interpreter shutdown.