Skip to content

Use thread_local for loader_life_support to improve performance #5830

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

Merged
merged 7 commits into from
Sep 12, 2025
Merged

Use thread_local for loader_life_support to improve performance #5830

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a912648
Use thread_local for loader_life_support to improve performance
swolchok Sep 5, 2025
728a8d9
Remove PYBIND11_CAN_USE_THREAD_LOCAL
swolchok Sep 8, 2025
efc678f
clarify comment
swolchok Sep 8, 2025
f40c22b
Simplify loader_life_support TLS storage
rwgk Sep 12, 2025
6bdc29f
loader_life_support: avoid duplicate tls_current_frame() calls
rwgk Sep 12, 2025
fb13fdd
Add REMINDER for next version bump in internals.h
rwgk Sep 12, 2025
508f408
Merge branch 'master' into @swolchok=thread-local
rwgk Sep 12, 2025
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
28 changes: 21 additions & 7 deletions include/pybind11/detail/type_caster_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,24 +42,38 @@ PYBIND11_NAMESPACE_BEGIN(detail)
/// Adding a patient will keep it alive up until the enclosing function returns.
class loader_life_support {
private:
// Thread-local top-of-stack for loader_life_support frames (linked via parent).
// Observation: loader_life_support needs to be thread-local,
// but we don't need to go to extra effort to keep it
// per-interpreter (i.e., by putting it in internals) since
// individual function calls are already isolated to a single
// interpreter, even though they could potentially call into a
// different interpreter later in the same call chain. This
// saves a significant cost per function call spent in
// loader_life_support destruction.
// Note for future C++17 simplification:
// inline static thread_local loader_life_support *tls_current_frame = nullptr;
static loader_life_support *&tls_current_frame() {
static thread_local loader_life_support *frame_ptr = nullptr;
return frame_ptr;
}

loader_life_support *parent = nullptr;
std::unordered_set<PyObject *> keep_alive;

public:
/// A new patient frame is created when a function is entered
loader_life_support() {
auto &stack_top = get_internals().loader_life_support_tls;
parent = stack_top.get();
stack_top = this;
parent = tls_current_frame();
tls_current_frame() = this;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm concerned that this might check if the thread_local has been initialized twice. looking into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, this pattern is fine. https://godbolt.org/z/3Gzrjr5K5

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about

loader_life_support() {
    auto &frame = tls_current_frame();
    parent = frame;
    frame = this;
}

~loader_life_support() {
    auto &frame = tls_current_frame();
    if (frame == nullptr || frame != this) {
        pybind11_fail("loader_life_support: internal error");
    }
    frame = parent;
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's fine the way it is, but I like that version as well.

}

/// ... and destroyed after it returns
~loader_life_support() {
auto &stack_top = get_internals().loader_life_support_tls;
if (stack_top.get() != this) {
if (tls_current_frame() != this) {
pybind11_fail("loader_life_support: internal error");
}
stack_top = parent;
tls_current_frame() = parent;
for (auto *item : keep_alive) {
Py_DECREF(item);
}
Expand All @@ -68,7 +82,7 @@ class loader_life_support {
/// This can only be used inside a pybind11-bound function, either by `argument_loader`
/// at argument preparation time or by `py::cast()` at execution time.
PYBIND11_NOINLINE static void add_patient(handle h) {
loader_life_support *frame = get_internals().loader_life_support_tls.get();
loader_life_support *frame = tls_current_frame();
if (!frame) {
// NOTE: It would be nice to include the stack frames here, as this indicates
// use of pybind11::cast<> outside the normal call framework, finding such
Expand Down
Loading