Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide GILPool behind a feature to test ability to fully avoid it #3685

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ experimental-inspect = []
# Enables macros: #[pyclass], #[pymodule], #[pyfunction] etc.
macros = ["pyo3-macros", "indoc", "unindent"]

# Enables GIL-bound references backed by a thread-local pool
pool = []

# Enables multiple #[pymethods] per #[pyclass]
multiple-pymethods = ["inventory", "pyo3-macros/multiple-pymethods"]

Expand Down
8 changes: 4 additions & 4 deletions src/conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@ use crate::inspect::types::TypeInfo;
use crate::pyclass::boolean_struct::False;
use crate::type_object::PyTypeInfo;
use crate::types::PyTuple;
use crate::{
ffi, gil, Py, PyAny, PyCell, PyClass, PyNativeType, PyObject, PyRef, PyRefMut, Python,
};
use crate::{ffi, Py, PyAny, PyCell, PyClass, PyNativeType, PyObject, PyRef, PyRefMut, Python};
use std::cell::Cell;
#[cfg(feature = "pool")]
use std::ptr::NonNull;

/// Returns a borrowed pointer to a Python object.
Expand Down Expand Up @@ -543,12 +542,13 @@ pub unsafe trait FromPyPointer<'p>: Sized {
}
}

#[cfg(feature = "pool")]
unsafe impl<'p, T> FromPyPointer<'p> for T
where
T: 'p + crate::PyNativeType,
{
unsafe fn from_owned_ptr_or_opt(py: Python<'p>, ptr: *mut ffi::PyObject) -> Option<&'p Self> {
gil::register_owned(py, NonNull::new(ptr)?);
crate::gil::register_owned(py, NonNull::new(ptr)?);
Some(&*(ptr as *mut Self))
}
unsafe fn from_borrowed_ptr_or_opt(
Expand Down
61 changes: 53 additions & 8 deletions src/gil.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
//! Interaction with Python's global interpreter lock

#[cfg(feature = "pool")]
use crate::impl_::not_send::{NotSend, NOT_SEND};
use crate::{ffi, Python};
use parking_lot::{const_mutex, Mutex, Once};
use std::cell::Cell;
#[cfg(debug_assertions)]
#[cfg(all(feature = "pool", debug_assertions))]
use std::cell::RefCell;
#[cfg(not(debug_assertions))]
#[cfg(all(feature = "pool", not(debug_assertions)))]
use std::cell::UnsafeCell;
use std::{mem, ptr::NonNull};

Expand Down Expand Up @@ -37,9 +38,9 @@ thread_local_const_init! {
static GIL_COUNT: Cell<isize> = const { Cell::new(0) };

/// Temporarily hold objects that will be released when the GILPool drops.
#[cfg(debug_assertions)]
#[cfg(all(feature = "pool", debug_assertions))]
static OWNED_OBJECTS: RefCell<PyObjVec> = const { RefCell::new(Vec::new()) };
#[cfg(not(debug_assertions))]
#[cfg(all(feature = "pool", not(debug_assertions)))]
static OWNED_OBJECTS: UnsafeCell<PyObjVec> = const { UnsafeCell::new(Vec::new()) };
}

Expand Down Expand Up @@ -139,17 +140,27 @@ where
ffi::Py_InitializeEx(0);

// Safety: the GIL is already held because of the Py_IntializeEx call.
#[cfg(feature = "pool")]
let pool = GILPool::new();
#[cfg(feature = "pool")]
let py = pool.python();
#[cfg(not(feature = "pool"))]
let gil = WithGIL::during_call();
#[cfg(not(feature = "pool"))]
let py = gil.python();
Comment on lines +147 to +150
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep this looks the right way to keep py bounded correctly 👍


// Import the threading module - this ensures that it will associate this thread as the "main"
// thread, which is important to avoid an `AssertionError` at finalization.
pool.python().import("threading").unwrap();
py.import("threading").unwrap();

// Execute the closure.
let result = f(pool.python());
let result = f(py);

// Drop the pool before finalizing.
#[cfg(feature = "pool")]
drop(pool);
#[cfg(not(feature = "pool"))]
drop(gil);

// Finalize the Python interpreter.
ffi::Py_Finalize();
Expand All @@ -160,6 +171,7 @@ where
/// RAII type that represents the Global Interpreter Lock acquisition.
pub(crate) struct GILGuard {
gstate: ffi::PyGILState_STATE,
#[cfg(feature = "pool")]
pool: mem::ManuallyDrop<GILPool>,
}

Expand Down Expand Up @@ -222,9 +234,14 @@ impl GILGuard {
}

let gstate = unsafe { ffi::PyGILState_Ensure() }; // acquire GIL
#[cfg(feature = "pool")]
let pool = unsafe { mem::ManuallyDrop::new(GILPool::new()) };

Some(GILGuard { gstate, pool })
Some(GILGuard {
gstate,
#[cfg(feature = "pool")]
pool,
})
}
}

Expand All @@ -233,6 +250,7 @@ impl Drop for GILGuard {
fn drop(&mut self) {
unsafe {
// Drop the objects in the pool before attempting to release the thread state
#[cfg(feature = "pool")]
mem::ManuallyDrop::drop(&mut self.pool);

ffi::PyGILState_Release(self.gstate);
Expand Down Expand Up @@ -317,6 +335,30 @@ impl Drop for SuspendGIL {
}
}

/// Used to indicate safe access to the GIL
pub(crate) struct WithGIL;

impl WithGIL {
pub unsafe fn during_call() -> Self {
increment_gil_count();

// Update counts of PyObjects / Py that have been cloned or dropped since last acquisition
POOL.update_counts(Python::assume_gil_acquired());

Self
}

pub fn python(&self) -> Python<'_> {
unsafe { Python::assume_gil_acquired() }
}
}

impl Drop for WithGIL {
fn drop(&mut self) {
decrement_gil_count();
}
}

/// Used to lock safe access to the GIL
pub(crate) struct LockGIL {
count: isize,
Expand Down Expand Up @@ -355,16 +397,17 @@ impl Drop for LockGIL {
///
/// See the [Memory Management] chapter of the guide for more information about how PyO3 uses
/// [`GILPool`] to manage memory.

///
/// [Memory Management]: https://pyo3.rs/main/memory.html#gil-bound-memory
#[cfg(feature = "pool")]
pub struct GILPool {
/// Initial length of owned objects and anys.
/// `Option` is used since TSL can be broken when `new` is called from `atexit`.
start: Option<usize>,
_not_send: NotSend,
}

#[cfg(feature = "pool")]
impl GILPool {
/// Creates a new [`GILPool`]. This function should only ever be called with the GIL held.
///
Expand Down Expand Up @@ -401,6 +444,7 @@ impl GILPool {
}
}

#[cfg(feature = "pool")]
impl Drop for GILPool {
fn drop(&mut self) {
if let Some(start) = self.start {
Expand Down Expand Up @@ -463,6 +507,7 @@ pub unsafe fn register_decref(obj: NonNull<ffi::PyObject>) {
///
/// # Safety
/// The object must be an owned Python reference.
#[cfg(feature = "pool")]
pub unsafe fn register_owned(_py: Python<'_>, obj: NonNull<ffi::PyObject>) {
debug_assert!(gil_is_acquired());
// Ignores the error in case this function called from `atexit`.
Expand Down
1 change: 1 addition & 0 deletions src/impl_/not_send.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ use crate::Python;
/// Workaround for lack of !Send on stable (<https://github.com/rust-lang/rust/issues/68318>).
pub(crate) struct NotSend(PhantomData<*mut Python<'static>>);

#[cfg(feature = "pool")]
pub(crate) const NOT_SEND: NotSend = NotSend(PhantomData);
20 changes: 18 additions & 2 deletions src/impl_/trampoline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@ use std::{
panic::{self, UnwindSafe},
};

#[cfg(feature = "pool")]
use crate::gil::GILPool;
#[cfg(not(feature = "pool"))]
use crate::gil::WithGIL;
use crate::{
callback::PyCallbackOutput, ffi, impl_::panic::PanicTrap, methods::IPowModulo,
panic::PanicException, types::PyModule, GILPool, Py, PyResult, Python,
panic::PanicException, types::PyModule, Py, PyResult, Python,
};

#[inline]
Expand Down Expand Up @@ -174,8 +178,14 @@ where
R: PyCallbackOutput,
{
let trap = PanicTrap::new("uncaught panic at ffi boundary");
#[cfg(feature = "pool")]
let pool = unsafe { GILPool::new() };
#[cfg(feature = "pool")]
let py = pool.python();
#[cfg(not(feature = "pool"))]
let gil = unsafe { WithGIL::during_call() };
#[cfg(not(feature = "pool"))]
let py = gil.python();
let out = panic_result_into_callback_output(
py,
panic::catch_unwind(move || -> PyResult<_> { body(py) }),
Expand Down Expand Up @@ -219,8 +229,14 @@ where
F: for<'py> FnOnce(Python<'py>) -> PyResult<()> + UnwindSafe,
{
let trap = PanicTrap::new("uncaught panic at ffi boundary");
let pool = GILPool::new();
#[cfg(feature = "pool")]
let pool = unsafe { GILPool::new() };
#[cfg(feature = "pool")]
let py = pool.python();
#[cfg(not(feature = "pool"))]
let gil = unsafe { WithGIL::during_call() };
#[cfg(not(feature = "pool"))]
let py = gil.python();
if let Err(py_err) = panic::catch_unwind(move || body(py))
.unwrap_or_else(|payload| Err(PanicException::from_panic_payload(payload)))
{
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ pub use crate::conversion::{AsPyPointer, FromPyObject, FromPyPointer, IntoPy, To
#[allow(deprecated)]
pub use crate::conversion::{PyTryFrom, PyTryInto};
pub use crate::err::{PyDowncastError, PyErr, PyErrArguments, PyResult};
#[cfg(feature = "pool")]
pub use crate::gil::GILPool;
#[cfg(not(PyPy))]
pub use crate::gil::{prepare_freethreaded_python, with_embedded_python_interpreter};
Expand Down
6 changes: 5 additions & 1 deletion src/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,9 @@
//! [`Rc`]: std::rc::Rc
//! [`Py`]: crate::Py
use crate::err::{self, PyDowncastError, PyErr, PyResult};
use crate::gil::{GILGuard, GILPool, SuspendGIL};
#[cfg(feature = "pool")]
use crate::gil::GILPool;
use crate::gil::{GILGuard, SuspendGIL};
use crate::impl_::not_send::NotSend;
use crate::type_object::HasPyGilRef;
use crate::types::{
Expand Down Expand Up @@ -967,6 +969,7 @@ impl<'py> Python<'py> {
///
/// [`.python()`]: crate::GILPool::python
#[inline]
#[cfg(feature = "pool")]
pub unsafe fn new_pool(self) -> GILPool {
GILPool::new()
}
Expand Down Expand Up @@ -1025,6 +1028,7 @@ impl Python<'_> {
/// });
/// ```
#[inline]
#[cfg(feature = "pool")]
pub fn with_pool<F, R>(&self, f: F) -> R
where
F: for<'py> FnOnce(Python<'py>) -> R + Ungil,
Expand Down