Repository navigation
fix(free-threading): cast self directly in cpp_function::dispatcher - #6177
espressolee wants to merge 4 commits into
Conversation
Part of pybind#6159 (item 59). Under Py_GIL_DISABLED, cpp_function::dispatcher() took the internals lock on every bound call: it reached its function_record through function_record_ptr_from_PyObject(), which calls get_function_record_PyTypeObject() (PYBIND11_LOCK_INTERNALS on internals.mutex). dispatcher is installed in exactly one place (initialize_generic), and the PyCFunction there is created with m_self set to the function_record_PyObject allocated a few lines above, so self is always that object. The new function_record_ptr_from_dispatcher_self() states that invariant once and casts directly; its assert compares the versioned tp_name instead of calling is_function_record_PyObject(), which would reacquire the lock in non-NDEBUG builds. is_function_record_PyObject() and function_record_ptr_from_PyObject() are unchanged, so the call sites that can see a foreign object keep the checked conversion. Adds test_dispatch_does_not_need_internals_lock (free-threaded builds only): a native thread holds internals.mutex through raw PyCFunction helpers and the test checks that a bound call returns before the holder's watchdog releases the mutex, with a positive control proving the held mutex is the one internals users take. Fails without the fix, passes with it. Assisted-by: Claude Code:claude-opus-5
pybind11_tests imports custom_exceptions from the source tests directory at init, so the subprocess must see that directory as well as the build directory, as test_custom_type_setup.py does. Assisted-by: Claude Code:claude-opus-5
| // This works across extension modules, and does not need the internals lock. | ||
| // Note that tp_name is versioned. | ||
| inline bool function_record_PyTypeObject_name_matches(PyTypeObject *obj_type) { | ||
| return strcmp(obj_type->tp_name, function_record_PyTypeObject_methods::tp_qualname_impl) == 0 |
There was a problem hiding this comment.
Should be std::strcmp nit
|
Thanks — I’ve updated both calls to |
Apply the review suggestion to both sibling comparisons; retain the existing cstring include. Assisted-by: Codex:GPT-6
Resolve the conflict with pybind#6136, which moved the definition of cpp_function::dispatcher from pybind11.h to pybind11-inl.h: keep master's pybind11.h and apply this PR's one-line change (function_record_ptr_from_dispatcher_self) to the dispatcher in pybind11-inl.h. Assisted-by: Hermes Agent:claude-opus-5-5
|
Merged master to resolve the conflict with #6136 (no rebase, so the approved commits are unchanged): the dispatcher now lives in |
Description
Part of #6159 (item 59).
Under
Py_GIL_DISABLED,cpp_function::dispatcher()took the internals lock on every bound call: it reached itsfunction_recordthroughfunction_record_ptr_from_PyObject(), which callsget_function_record_PyTypeObject()(PYBIND11_LOCK_INTERNALSoninternals.mutex).dispatcheris installed in exactly one place (initialize_generic), and thePyCFunctionthere is created withm_selfset to thefunction_record_PyObjectallocated a few lines above, soselfis always that object. The newfunction_record_ptr_from_dispatcher_self()states that invariant once and casts directly. Its assert compares the versionedtp_nameinstead of callingis_function_record_PyObject(), which would reacquire the lock in non-NDEBUGbuilds (the preset default); that costs one or twostrcmpper call in debug builds only.is_function_record_PyObject()andfunction_record_ptr_from_PyObject()are unchanged, and the three call sites that can see a foreign object (the overload chain ininitialize_generic,get_function_record,reduce_ex_impl) keep the checked conversion. The exception path (try_translate_exceptions->with_exception_translators) still takesexception_translator_mutex; that is a different mutex and is not touched here.Test:
test_dispatch_does_not_need_internals_lockintests/test_thread.*, free-threaded builds only. A native thread holdsinternals.mutex(through rawPyCFunctionhelpers, because a pybind11-bound helper would itself go through the dispatcher and deadlock on the lock it is setting up); the verdict is whether the bound call returned before the holder's watchdog (5 s) released the mutex, so on a regression the test fails after 5 s rather than hanging. A positive control (with_internals()with a 1 s watchdog) first proves the held mutex is the one internals users take, so the test cannot go vacuous if the dispatcher's lock ever moves. The body runs in a subprocess with a 60 s timeout. It fails without the fix and passes with it (3/3 each, Debug +PYBIND11_WERROR=ON, CPython 3.14.0rc1t, macOS arm64).Measured with the reproducer from the issue (one no-arg bound call; Apple M4 Pro, unpinned; CPython 3.14.0rc1 free-threaded; pybind11 3.1.0 headers with and without this patch;
-O2 -DNDEBUG -std=c++17; 9 runs each, medians, M calls/s):Ranges do not overlap at 4 and 8 threads; at 1 thread they do (one low sample on the patched side).
Suggested changelog entry:
cpp_function::dispatcher()no longer acquires the internals lock on every call in free-threaded builds: the function record is now read directly fromself, which is always the record object created for that dispatcher.AI assistance
Claude Opus 5 (
claude-opus-5[1m]) and Claude Fable 5.1, with review by me: the patch, the test, the builds and the measurements were produced with Claude Code on my machine and reviewed by me before submission.📚 Documentation preview 📚: https://pybind11--6177.org.readthedocs.build/