fix: support free-threaded CPython with GIL disabled #5148

colesbury · 2024-05-29T18:49:45Z

Description

Accesses to the internals struct now goes through a with_internals([] { ... }); statement. In the free-threaded build, the internals mutex is locked for the duration of the access.

The registered_instances map has its own locking scheme because it may be frequently accesses by multiple thread. The map is shareded by address and a lock per shard is used to increase concurrency.

Other smaller changes:

Add gil_not_used() tag to indicate that a module supports running with the GIL disabled. See PyUnstable_Module_SetGIL.
Use PyMem_MALLOC for tp_doc in 3.13+
Add dict_getitemstringref to wrap new PyDict_GetItemStringRef API

Suggested changelog entry:

* Support free-threaded CPython (3.13t)
* Add ``py::mod_gil_not_used()`` tag to indicate if a module supports running with the GIL disabled.

include/pybind11/detail/internals.h

tests/test_cmake_build/main.cpp

include/pybind11/pybind11.h

tests/test_kwargs_and_defaults.py

Some additional locking is added in the free-threaded build when `Py_GIL_DISABLED` is defined: - Most accesses to internals are protected by a single mutex - The registered_instances uses a striped lock to improve concurrency Pybind11 modules can indicate support for running with the GIL disabled by calling `set_gil_not_used()`.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

include/pybind11/detail/internals.h

include/pybind11/pybind11.h

include/pybind11/detail/internals.h

rwgk · 2024-06-08T14:39:31Z

include/pybind11/detail/internals.h

@@ -542,6 +577,14 @@ PYBIND11_NOINLINE internals &get_internals() {
        internals_ptr->static_property_type = make_static_property_type();
        internals_ptr->default_metaclass = make_default_metaclass();
        internals_ptr->instance_base = make_object_base_type(internals_ptr->default_metaclass);
+#ifdef Py_GIL_DISABLED
+        size_t num_shards = (size_t) next_pow2(2 * std::thread::hardware_concurrency());


Maybe

auto num_shards = static_cast<size_t>(...);

?

(The main motivation is to minimize C-style casts. Similarly in a few other places changed in this PR.)

Could it be worth adding a comment to explain why 2 * ...?

include/pybind11/detail/internals.h

rwgk · 2024-06-08T14:54:13Z

include/pybind11/detail/internals.h

+    auto &internals = get_internals();
+
+#ifdef Py_GIL_DISABLED
+    // Hash address to compute shard, but ignore low bits. We'd like allocations


Suggestion to move this comment to the splitmix64 function.

This comment is specific to this section of code: it's mostly about why we are ignoring the low bits (addr >> 20) when hashing the address.

rwgk · 2024-06-08T15:03:21Z

include/pybind11/detail/internals.h

+    // other threads/cores to map to other shards. Using the high bits is a good
+    // heuristic because memory allocators often have a per-thread
+    // arena/superblock/segment from which smaller allocations are served.
+    auto addr = reinterpret_cast<uintptr_t>(ptr);


static_cast seems to work here (with Linux gcc at least). (I see we're already using reinterpret_cast in a bunch of similar situations, but maybe that isn't ideal?)

Would using std::uintptr_t and std::uint64_t be slightly better?

static_cast here causes a compiler error for me: https://gcc.godbolt.org/z/bTnosGf61

I've updated the other casts and used the std namespace.

include/pybind11/detail/internals.h

rwgk · 2024-06-08T17:36:42Z

include/pybind11/pybind11.h

@@ -1199,6 +1206,8 @@ struct handle_type_name<cpp_function> {

 PYBIND11_NAMESPACE_END(detail)

+struct gil_not_used {};


What do you think about the alternative API here:
rwgk@a1c0f8e

?

I like how much simpler it is. I'm not sure about the mod; if we put anything, it should have the full module, we don't use mod elsewhere for Python, and it isn't clearly module. But it can't be misplaced, and we don't "namespace" anything else like it.

I'm not sure it would actually be any simpler if we added a second option later (like some sort of multiphase init option). But maybe it's better to simplify now rather than worry too much about a future (which would not be more complex by this style either, at least).

py::free_threaded() seems like it would match the naming of CPython better, though most of the CPython internals related to the feature seem to refer to the GIL.

I like how much simpler it is.

It's simpler only here, but my suggested alternative is simpler overall, because it avoids the overloads and the private implementation of the create_extension_module function. For the user it doesn't make a difference.

My suggestion includes the mod_ so that it comes out very similar to the existing name: Py_MOD_GIL_NOT_USED vs. py::mod_gil_not_used().

Another thought was that it's very easy to experiment, e.g. if

PYBIND11_MODULE(external_module, m, py::mod_gil_not_used())

has some issues that are difficult to resolve in the short term, it can be changed to

PYBIND11_MODULE(external_module, m, py::mod_gil_not_used(false))

and it'll be very obvious that free-threading support was attempted, and how to start trying again. It'll also be very easy to make the free-threading support platform-specific this way.

but my suggested alternative is simpler overall, because it avoids the overloads and the private implementation

That's what I meant by simpler.

It's also called PyUnstable_Module_SetGIL, which does have the module in it. But I hadn't noticed how that mimics the slot value name. Personally still like the more readable, less cryptic names, but happy with whatever we go with.

And py::free_threaded(false) avoids the double negative. ;)

That's what I meant by simpler.

Oh, got it. Thanks. (The way it appeared in the email message got me on the wrong track.)

And py::free_threaded(false) avoids the double negative.

That works for me, too. — Usually I strive to not introduce new names if there is one already, but the double negative bothered me, too.

Happy to have @colesbury have final say on what name he prefers.

I would lean towards matching the name from the Python C API because I think using the same names helps developers when they switch between the C API and pybind11.

This PR looks great to me, except the question about adopting the simplification under rwgk@a1c0f8e. @colesbury What's your opinion about that?

colesbury

Thanks for the review @rwgk. I've added some comments and renamed a few functions.

include/pybind11/detail/internals.h

colesbury · 2024-06-17T21:39:56Z

include/pybind11/detail/internals.h

+    auto &internals = get_internals();
+
+#ifdef Py_GIL_DISABLED
+    // Hash address to compute shard, but ignore low bits. We'd like allocations


This comment is specific to this section of code: it's mostly about why we are ignoring the low bits (addr >> 20) when hashing the address.

colesbury · 2024-06-17T21:42:11Z

include/pybind11/detail/internals.h

+    // other threads/cores to map to other shards. Using the high bits is a good
+    // heuristic because memory allocators often have a per-thread
+    // arena/superblock/segment from which smaller allocations are served.
+    auto addr = reinterpret_cast<uintptr_t>(ptr);


static_cast here causes a compiler error for me: https://gcc.godbolt.org/z/bTnosGf61

I've updated the other casts and used the std namespace.

include/pybind11/detail/internals.h

colesbury · 2024-06-17T21:57:56Z

include/pybind11/pybind11.h

@@ -1199,6 +1206,8 @@ struct handle_type_name<cpp_function> {

 PYBIND11_NAMESPACE_END(detail)

+struct gil_not_used {};


I would lean towards matching the name from the Python C API because I think using the same names helps developers when they switch between the C API and pybind11.

henryiii

@colesbury If you could let us know if you like @rwgk's variation (I don't mind applying it if you do), otherwise it looks good to me.

henryiii · 2024-06-18T19:20:34Z

Changelog in the description is a little out of date, too.

colesbury · 2024-06-18T19:26:09Z

Yes, please apply it

…into pep703-support

include/pybind11/pybind11.h

rwgk

Thanks @colesbury for transporting pybind11 into the free-threading world :-)

colesbury requested a review from henryiii as a code owner May 29, 2024 18:49

henryiii reviewed May 29, 2024

View reviewed changes

include/pybind11/detail/internals.h Show resolved Hide resolved

henryiii reviewed May 29, 2024

View reviewed changes

tests/test_cmake_build/main.cpp Outdated Show resolved Hide resolved

henryiii reviewed May 29, 2024

View reviewed changes

include/pybind11/pybind11.h Outdated Show resolved Hide resolved

henryiii requested review from rwgk and wjakob May 29, 2024 21:04

rwgk reviewed May 29, 2024

View reviewed changes

tests/test_kwargs_and_defaults.py Outdated Show resolved Hide resolved

henryiii mentioned this pull request May 30, 2024

tests: avoid immortal objects in tests #5150

Merged

henryiii force-pushed the pep703-support branch 4 times, most recently from a165d28 to e5470eb Compare May 30, 2024 14:33

henryiii mentioned this pull request May 30, 2024

Status of pybind11 compatibility Quansight-Labs/free-threaded-compatibility#13

Closed

refactor: use PYBIND11_MODULE (#11)

1831bde

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

wjakob reviewed Jun 4, 2024

View reviewed changes

colesbury added 2 commits June 5, 2024 15:50

Address code review

0a57d1d

Suppress MSVC warning

f53dcb5

rwgk reviewed Jun 8, 2024

View reviewed changes

colesbury and others added 2 commits June 17, 2024 22:07

Changes from review

2a29578

style: pre-commit fixes

85f3ff4

colesbury commented Jun 17, 2024

View reviewed changes

henryiii approved these changes Jun 18, 2024

View reviewed changes

Ralf W. Grosse-Kunstleve added 3 commits June 18, 2024 12:57

Merge branch 'master' into pep703-support

3cafb88

py::mod_gil_not_used() suggestion.

44d1fc7

Merge branch 'pep703-support' of https://github.com/colesbury/pybind11 …

c61f197

…into pep703-support

henryiii reviewed Jun 18, 2024

View reviewed changes

include/pybind11/pybind11.h Outdated Show resolved Hide resolved

Update include/pybind11/pybind11.h

d40299a

rwgk approved these changes Jun 18, 2024

View reviewed changes

rwgk merged commit baa540e into pybind:master Jun 18, 2024
83 checks passed

github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 18, 2024

colesbury deleted the pep703-support branch June 19, 2024 00:05

henryiii removed the needs changelog Possibly needs a changelog entry label Jun 26, 2024

rgommers mentioned this pull request Jun 26, 2024

free-threaded x86-64 macOS wheel build job failed with pybind11 2.13.0 scipy/scipy#21056

Closed

Skylion007 mentioned this pull request Jul 2, 2024

[BE][Ez]: Update pybind11 submodule to v2.13.1 pytorch/pytorch#129827

Closed

rgommers mentioned this pull request Jul 17, 2024

docs: extend PYBIND11_MODULE documentation, mention mod_gil_not_used #5250

Merged

rwgk mentioned this pull request Aug 20, 2024

[BUG]: pybind11's initialization of internals needs to be made more thread safe in the presence of free threading #5316

Open

3 tasks

wjakob mentioned this pull request Aug 21, 2024

Support for free-threaded Python wjakob/nanobind#695

Merged

6 tasks

colesbury mentioned this pull request Sep 12, 2024

Added exception translator specific mutex used with try_translate_exceptions #5362

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support free-threaded CPython with GIL disabled #5148

fix: support free-threaded CPython with GIL disabled #5148

colesbury commented May 29, 2024 •

edited by henryiii

Loading

rwgk Jun 8, 2024

rwgk Jun 8, 2024

colesbury Jun 17, 2024

rwgk Jun 8, 2024

colesbury Jun 17, 2024

rwgk Jun 8, 2024

henryiii Jun 9, 2024

rwgk Jun 9, 2024

henryiii Jun 9, 2024

henryiii Jun 9, 2024 •

edited

Loading

rwgk Jun 9, 2024

henryiii Jun 11, 2024

colesbury Jun 17, 2024

rwgk Jun 18, 2024

colesbury left a comment

colesbury Jun 17, 2024

colesbury Jun 17, 2024

colesbury Jun 17, 2024

henryiii left a comment

henryiii commented Jun 18, 2024

colesbury commented Jun 18, 2024

rwgk left a comment

		@@ -1199,6 +1206,8 @@ struct handle_type_name<cpp_function> {

		PYBIND11_NAMESPACE_END(detail)

		struct gil_not_used {};

fix: support free-threaded CPython with GIL disabled #5148

fix: support free-threaded CPython with GIL disabled #5148

Conversation

colesbury commented May 29, 2024 • edited by henryiii Loading

Description

Suggested changelog entry:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii left a comment

Choose a reason for hiding this comment

henryiii commented Jun 18, 2024

colesbury commented Jun 18, 2024

rwgk left a comment

Choose a reason for hiding this comment

colesbury commented May 29, 2024 •

edited by henryiii

Loading

henryiii Jun 9, 2024 •

edited

Loading