From c78c6137bc473d2fb66e830bd7af8530be78e12f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 25 Oct 2022 23:12:59 -0700 Subject: [PATCH] All bells & whistles. --- .github/workflows/ci.yml | 4 ++ CMakeLists.txt | 7 ++-- docs/upgrade.rst | 18 ++++----- include/pybind11/detail/common.h | 4 ++ include/pybind11/detail/internals.h | 12 ++++++ include/pybind11/gil.h | 53 ++++++++++++++++++++------- tests/test_embed/test_interpreter.cpp | 1 - 7 files changed, 72 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2951a0ad190..fe48770372e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -102,10 +102,12 @@ jobs: run: python -m pip install pytest-github-actions-annotate-failures # First build - C++11 mode and inplace + # More-or-less randomly adding -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON here. - name: Configure C++11 ${{ matrix.args }} run: > cmake -S . -B . -DPYBIND11_WERROR=ON + -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=11 @@ -129,10 +131,12 @@ jobs: run: git clean -fdx # Second build - C++17 mode and in a build directory + # More-or-less randomly adding -DPYBIND11_SIMPLE_GIL_MANAGEMENT=OFF here. - name: Configure C++17 run: > cmake -S . -B build2 -DPYBIND11_WERROR=ON + -DPYBIND11_SIMPLE_GIL_MANAGEMENT=OFF -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=17 diff --git a/CMakeLists.txt b/CMakeLists.txt index 632da9bb028..0d93203881e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -91,13 +91,14 @@ endif() option(PYBIND11_INSTALL "Install pybind11 header files?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_TEST "Build pybind11 test suite?" ${PYBIND11_MASTER_PROJECT}) option(PYBIND11_NOPYTHON "Disable search for Python" OFF) -option(PYBIND11_SIMPLE_GIL "Use simpler GIL access logic that does not support disassociation" OFF) +option(PYBIND11_SIMPLE_GIL_MANAGEMENT + "Use simpler GIL management logic that does not support disassociation" OFF) set(PYBIND11_INTERNALS_VERSION "" CACHE STRING "Override the ABI version, may be used to enable the unstable ABI.") -if(PYBIND11_SIMPLE_GIL) - add_compile_definitions(PYBIND11_SIMPLE_GIL) +if(PYBIND11_SIMPLE_GIL_MANAGEMENT) + add_compile_definitions(PYBIND11_SIMPLE_GIL_MANAGEMENT) endif() cmake_dependent_option( diff --git a/docs/upgrade.rst b/docs/upgrade.rst index 63c3821d08e..19b0b09b7df 100644 --- a/docs/upgrade.rst +++ b/docs/upgrade.rst @@ -13,16 +13,14 @@ modernization and other useful information. v2.10 ===== -The current scoped GIL implementation doesn't support nested access. In pybind11 -In 2.10.1, a configuration option ``PYBIND11_SIMPLE_GIL`` was added, defaulting -to OFF; the simpler GIL implementation supports nested access, but does not -support dissociation (the ``true`` parameter of ``gil_scope_release``). In -pybind11 2.11, we plan to change the default to ON. If you need the old -behavior, please set ``PYBIND11_SIMPLE_GIL`` to OFF. We plan to have an example -for manually supporting dissociation. - -There may be an unconfirmed ABI breakage between 2.9 and 2.10. We plan to bump -the internals number in 2.11. +``py::gil_scoped_acquire`` & ``py::gil_scoped_release`` in pybind11 versions +< v2.10.1 do not support nested access. In v2.10.1, a configuration option +``PYBIND11_SIMPLE_GIL_MANAGEMENT`` was added, defaulting to ``OFF``; the +simpler implementations support nested access, but do not support dissociation +(``py::gil_scoped_release(true)``). In pybind11 2.11, we plan to change the +default to ``ON``, to avoid pitfalls of the implementations with dissociation +(see #4216 for more information). Note that the dissociation feature is very +rarely used and not exercised in any pybind11 unit tests. .. _upgrade-guide-2.9: diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index b43100b95aa..e446c78f433 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -229,6 +229,10 @@ # undef copysign #endif +#if defined(PYPY_VERSION) && !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) +# define PYBIND11_SIMPLE_GIL_MANAGEMENT +#endif + #if defined(_MSC_VER) # if defined(PYBIND11_DEBUG_MARKER) # define _DEBUG diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index d47084e263d..d2a77472b8a 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -9,6 +9,10 @@ #pragma once +#if defined(WITH_THREAD) && defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) +# include "../gil.h" +#endif + #include "../pytypes.h" #include @@ -169,10 +173,12 @@ struct internals { PyTypeObject *default_metaclass; PyObject *instance_base; #if defined(WITH_THREAD) + // Unused if PYBIND11_SIMPLE_GIL_MANAGEMENT is defined: PYBIND11_TLS_KEY_INIT(tstate) # if PYBIND11_INTERNALS_VERSION > 4 PYBIND11_TLS_KEY_INIT(loader_life_support_tls_key) # endif // PYBIND11_INTERNALS_VERSION > 4 + // Unused if PYBIND11_SIMPLE_GIL_MANAGEMENT is defined: PyInterpreterState *istate = nullptr; ~internals() { # if PYBIND11_INTERNALS_VERSION > 4 @@ -408,6 +414,10 @@ PYBIND11_NOINLINE internals &get_internals() { return **internals_pp; } +#if defined(WITH_THREAD) +# if defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) + gil_scoped_acquire gil; +# else // Ensure that the GIL is held since we will need to make Python calls. // Cannot use py::gil_scoped_acquire here since that constructor calls get_internals. struct gil_scoped_acquire_local { @@ -417,6 +427,8 @@ PYBIND11_NOINLINE internals &get_internals() { ~gil_scoped_acquire_local() { PyGILState_Release(state); } const PyGILState_STATE state; } gil; +# endif +#endif error_scope err_scope; PYBIND11_STR_TYPE id(PYBIND11_INTERNALS_ID); diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index e709725f94f..cb0028d5051 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -10,7 +10,10 @@ #pragma once #include "detail/common.h" -#include "detail/internals.h" + +#if defined(WITH_THREAD) && !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) +# include "detail/internals.h" +#endif PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -21,7 +24,9 @@ PyThreadState *get_thread_state_unchecked(); PYBIND11_NAMESPACE_END(detail) -#if defined(WITH_THREAD) && !defined(PYPY_VERSION) && !defined(PYBIND11_SIMPLE_GIL) +#if defined(WITH_THREAD) + +# if !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) /* The functions below essentially reproduce the PyGILState_* API using a RAII * pattern, but there are a few important differences: @@ -62,11 +67,11 @@ class gil_scoped_acquire { if (!tstate) { tstate = PyThreadState_New(internals.istate); -# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) +# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) if (!tstate) { pybind11_fail("scoped_acquire: could not create thread state!"); } -# endif +# endif tstate->gilstate_counter = 0; PYBIND11_TLS_REPLACE_VALUE(internals.tstate, tstate); } else { @@ -87,20 +92,20 @@ class gil_scoped_acquire { PYBIND11_NOINLINE void dec_ref() { --tstate->gilstate_counter; -# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) +# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) if (detail::get_thread_state_unchecked() != tstate) { pybind11_fail("scoped_acquire::dec_ref(): thread state must be current!"); } if (tstate->gilstate_counter < 0) { pybind11_fail("scoped_acquire::dec_ref(): reference count underflow!"); } -# endif +# endif if (tstate->gilstate_counter == 0) { -# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) +# if defined(PYBIND11_DETAILED_ERROR_MESSAGES) if (!release) { pybind11_fail("scoped_acquire::dec_ref(): internal error!"); } -# endif +# endif PyThreadState_Clear(tstate); if (active) { PyThreadState_DeleteCurrent(); @@ -178,12 +183,14 @@ class gil_scoped_release { bool disassoc; bool active = true; }; -#elif defined(PYPY_VERSION) || defined(PYBIND11_SIMPLE_GIL) + +# else // PYBIND11_SIMPLE_GIL_MANAGEMENT + class gil_scoped_acquire { PyGILState_STATE state; public: - gil_scoped_acquire() { state = PyGILState_Ensure(); } + gil_scoped_acquire() : state{PyGILState_Ensure()} {} gil_scoped_acquire(const gil_scoped_acquire &) = delete; gil_scoped_acquire &operator=(const gil_scoped_acquire &) = delete; ~gil_scoped_acquire() { PyGILState_Release(state); } @@ -194,19 +201,39 @@ class gil_scoped_release { PyThreadState *state; public: - gil_scoped_release() { state = PyEval_SaveThread(); } + gil_scoped_release() : state{PyEval_SaveThread()} {} gil_scoped_release(const gil_scoped_release &) = delete; gil_scoped_release &operator=(const gil_scoped_acquire &) = delete; ~gil_scoped_release() { PyEval_RestoreThread(state); } void disarm() {} }; -#else + +# endif // PYBIND11_SIMPLE_GIL_MANAGEMENT + +#else // WITH_THREAD + class gil_scoped_acquire { +public: + gil_scoped_acquire() { + // Trick to suppress `unused variable` error messages (at call sites). + (void) (this != (this + 1)); + } + gil_scoped_acquire(const gil_scoped_acquire &) = delete; + gil_scoped_acquire &operator=(const gil_scoped_acquire &) = delete; void disarm() {} }; + class gil_scoped_release { +public: + gil_scoped_release() { + // Trick to suppress `unused variable` error messages (at call sites). + (void) (this != (this + 1)); + } + gil_scoped_release(const gil_scoped_release &) = delete; + gil_scoped_release &operator=(const gil_scoped_acquire &) = delete; void disarm() {} }; -#endif + +#endif // WITH_THREAD PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index 6299293b919..44dcd1fdb0d 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -293,7 +293,6 @@ TEST_CASE("Threads") { { py::gil_scoped_release gil_release{}; - REQUIRE(has_pybind11_internals_static()); auto threads = std::vector(); for (auto i = 0; i < num_threads; ++i) {