fix(free-threading): cast self directly in cpp_function::dispatcher - #6177
Open
espressolee wants to merge 2 commits into
Open
fix(free-threading): cast self directly in cpp_function::dispatcher#6177espressolee wants to merge 2 commits into
self directly in cpp_function::dispatcher#6177espressolee wants to merge 2 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 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
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.