From 62f424b6901bdfd41aebb17120af5a38abf9f112 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sat, 20 May 2023 11:18:57 +0200 Subject: [PATCH] Drop EnsureGIL to remove one layer of indirection from the implementation of Python::with_gil --- src/gil.rs | 96 +++++++++------------------------------- src/marker.rs | 14 ++++-- tests/ui/not_send.stderr | 4 +- 3 files changed, 32 insertions(+), 82 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index aa4ed2e7825..e358a410f2f 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -29,6 +29,7 @@ thread_local! { /// 1) for performance /// 2) PyGILState_Check always returns 1 if the sub-interpreter APIs have ever been called, /// which could lead to incorrect conclusions that the GIL is held. +#[inline(always)] fn gil_is_acquired() -> bool { GIL_COUNT.try_with(|c| c.get() > 0).unwrap_or(false) } @@ -135,24 +136,21 @@ where } /// RAII type that represents the Global Interpreter Lock acquisition. -struct GILGuard { +pub(crate) struct GILGuard { gstate: ffi::PyGILState_STATE, - pool: Option, - _not_send: NotSend, + pool: mem::ManuallyDrop, } impl GILGuard { - /// Retrieves the marker type that proves that the GIL was acquired. - #[inline] - pub fn python(&self) -> Python<'_> { - unsafe { Python::assume_gil_acquired() } - } - /// PyO3 internal API for acquiring the GIL. The public API is Python::with_gil. /// - /// If PyO3 does not yet have a `GILPool` for tracking owned PyObject references, then this new - /// `GILGuard` will also contain a `GILPool`. - fn acquire() -> GILGuard { + /// If the GIL was already acquired via PyO3, this returns `None`. Otherwise, + /// the GIL will be acquired and a new `GILPool` created. + pub(crate) fn acquire() -> Option { + if gil_is_acquired() { + return None; + } + // Maybe auto-initialize the GIL: // - If auto-initialize feature set and supported, try to initialize the interpreter. // - If the auto-initialize feature is set but unsupported, emit hard errors only when the @@ -196,39 +194,25 @@ impl GILGuard { /// This can be called in "unsafe" contexts where the normal interpreter state /// checking performed by `GILGuard::acquire` may fail. This includes calling /// as part of multi-phase interpreter initialization. - fn acquire_unchecked() -> GILGuard { + pub(crate) fn acquire_unchecked() -> Option { + if gil_is_acquired() { + return None; + } + let gstate = unsafe { ffi::PyGILState_Ensure() }; // acquire GIL + let pool = unsafe { mem::ManuallyDrop::new(GILPool::new()) }; - // If there's already a GILPool, we should not create another or this could lead to - // incorrect dangling references in safe code (see #864). - let pool = if !gil_is_acquired() { - Some(unsafe { GILPool::new() }) - } else { - // As no GILPool was created, need to update the gil count manually. - increment_gil_count(); - None - }; - - GILGuard { - gstate, - pool, - _not_send: NOT_SEND, - } + Some(GILGuard { gstate, pool }) } } /// The Drop implementation for `GILGuard` will release the GIL. impl Drop for GILGuard { fn drop(&mut self) { - // Drop the objects in the pool before attempting to release the thread state - if let Some(pool) = self.pool.take() { - drop(pool) - } else { - // This GILGuard didn't own a pool, need to decrease the count manually. - decrement_gil_count() - } - unsafe { + // Drop the objects in the pool before attempting to release the thread state + mem::ManuallyDrop::drop(&mut self.pool); + ffi::PyGILState_Release(self.gstate); } } @@ -442,46 +426,6 @@ fn decrement_gil_count() { }); } -/// Ensures the GIL is held, used in the implementation of `Python::with_gil`. -pub(crate) fn ensure_gil() -> EnsureGIL { - if gil_is_acquired() { - EnsureGIL(None) - } else { - EnsureGIL(Some(GILGuard::acquire())) - } -} - -/// Ensures the GIL is held, without interpreter state checking. -/// -/// This bypasses interpreter state checking that would normally be performed -/// before acquiring the GIL. -pub(crate) fn ensure_gil_unchecked() -> EnsureGIL { - if gil_is_acquired() { - EnsureGIL(None) - } else { - EnsureGIL(Some(GILGuard::acquire_unchecked())) - } -} - -/// Struct used internally which avoids acquiring the GIL where it's not necessary. -pub(crate) struct EnsureGIL(Option); - -impl EnsureGIL { - /// Get the GIL token. - /// - /// # Safety - /// If `self.0` is `None`, then this calls [Python::assume_gil_acquired]. - /// Thus this method could be used to get access to a GIL token while the GIL is not held. - /// Care should be taken to only use the returned Python in contexts where it is certain the - /// GIL continues to be held. - pub(crate) unsafe fn python(&self) -> Python<'_> { - match &self.0 { - Some(gil) => gil.python(), - None => Python::assume_gil_acquired(), - } - } -} - #[cfg(test)] mod tests { use super::{gil_is_acquired, GILPool, GIL_COUNT, OWNED_OBJECTS, POOL}; diff --git a/src/marker.rs b/src/marker.rs index bd30d3c3396..c0649057298 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -120,7 +120,7 @@ //! [`Rc`]: std::rc::Rc //! [`Py`]: crate::Py use crate::err::{self, PyDowncastError, PyErr, PyResult}; -use crate::gil::{self, EnsureGIL, GILPool, SuspendGIL}; +use crate::gil::{GILGuard, GILPool, SuspendGIL}; use crate::impl_::not_send::NotSend; use crate::types::{PyAny, PyDict, PyModule, PyString, PyType}; use crate::version::PythonVersionInfo; @@ -273,7 +273,7 @@ mod negative_impls { /// [`Py::clone_ref`]: crate::Py::clone_ref /// [Memory Management]: https://pyo3.rs/main/memory.html#gil-bound-memory #[derive(Copy, Clone)] -pub struct Python<'py>(PhantomData<(&'py EnsureGIL, NotSend)>); +pub struct Python<'py>(PhantomData<(&'py GILGuard, NotSend)>); impl Python<'_> { /// Acquires the global interpreter lock, allowing access to the Python interpreter. The @@ -321,7 +321,10 @@ impl Python<'_> { where F: for<'py> FnOnce(Python<'py>) -> R, { - f(unsafe { gil::ensure_gil().python() }) + let _guard = GILGuard::acquire(); + + // SAFETY: Either the GIL was already acquired or we just created a new `GILGuard`. + f(unsafe { Python::assume_gil_acquired() }) } /// Like [`Python::with_gil`] except Python interpreter state checking is skipped. @@ -352,7 +355,10 @@ impl Python<'_> { where F: for<'py> FnOnce(Python<'py>) -> R, { - f(gil::ensure_gil_unchecked().python()) + let _guard = GILGuard::acquire_unchecked(); + + // SAFETY: Either the GIL was already acquired or we just created a new `GILGuard`. + f(Python::assume_gil_acquired()) } } diff --git a/tests/ui/not_send.stderr b/tests/ui/not_send.stderr index a1a55db338f..18547a10f80 100644 --- a/tests/ui/not_send.stderr +++ b/tests/ui/not_send.stderr @@ -9,8 +9,8 @@ error[E0277]: `*mut pyo3::Python<'static>` cannot be shared between threads safe = help: within `pyo3::Python<'_>`, the trait `Sync` is not implemented for `*mut pyo3::Python<'static>` = note: required because it appears within the type `PhantomData<*mut Python<'static>>` = note: required because it appears within the type `NotSend` - = note: required because it appears within the type `(&EnsureGIL, NotSend)` - = note: required because it appears within the type `PhantomData<(&EnsureGIL, NotSend)>` + = note: required because it appears within the type `(&GILGuard, NotSend)` + = note: required because it appears within the type `PhantomData<(&GILGuard, NotSend)>` = note: required because it appears within the type `Python<'_>` = note: required for `&pyo3::Python<'_>` to implement `Send` note: required because it's used within this closure