From 43d791e2b60f060b7f7add9d56d4456e1d8b7c51 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Thu, 18 Apr 2024 19:15:52 +0200 Subject: [PATCH] Add feature controlling the global reference pool to enable avoiding its overhead. --- Cargo.toml | 5 ++++- src/gil.rs | 42 ++++++++++++++++++++++++++++++-------- tests/test_class_basics.rs | 8 ++++---- tests/test_gc.rs | 2 ++ 4 files changed, 43 insertions(+), 14 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 90bcc03c80c..1c671a9abb6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -63,7 +63,7 @@ futures = "0.3.28" pyo3-build-config = { path = "pyo3-build-config", version = "=0.21.2", features = ["resolve-config"] } [features] -default = ["macros"] +default = ["macros", "reference-pool"] # Enables support for `async fn` for `#[pyfunction]` and `#[pymethods]`. experimental-async = ["macros", "pyo3-macros/experimental-async"] @@ -106,6 +106,9 @@ auto-initialize = [] # Allows use of the deprecated "GIL Refs" APIs. gil-refs = [] +# Allows `Clone`ing references to Python objects without the GIL being held. +reference-pool = [] + # Optimizes PyObject to Vec conversion and so on. nightly = [] diff --git a/src/gil.rs b/src/gil.rs index e2f36037755..d473c1af18e 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -2,7 +2,9 @@ use crate::impl_::not_send::{NotSend, NOT_SEND}; use crate::{ffi, Python}; -use parking_lot::{const_mutex, Mutex, Once}; +use parking_lot::Once; +#[cfg(feature = "reference-pool")] +use parking_lot::{const_mutex, Mutex}; use std::cell::Cell; #[cfg(debug_assertions)] use std::cell::RefCell; @@ -246,12 +248,14 @@ impl Drop for GILGuard { // Vector of PyObject type PyObjVec = Vec>; +#[cfg(feature = "reference-pool")] /// Thread-safe storage for objects which were inc_ref / dec_ref while the GIL was not held. struct ReferencePool { // .0 is INCREFs, .1 is DECREFs pointer_ops: Mutex<(PyObjVec, PyObjVec)>, } +#[cfg(feature = "reference-pool")] impl ReferencePool { const fn new() -> Self { Self { @@ -289,8 +293,10 @@ impl ReferencePool { } } +#[cfg(feature = "reference-pool")] unsafe impl Sync for ReferencePool {} +#[cfg(feature = "reference-pool")] static POOL: ReferencePool = ReferencePool::new(); /// A guard which can be used to temporarily release the GIL and restore on `Drop`. @@ -315,6 +321,7 @@ impl Drop for SuspendGIL { ffi::PyEval_RestoreThread(self.tstate); // Update counts of PyObjects / Py that were cloned or dropped while the GIL was released. + #[cfg(feature = "reference-pool")] POOL.update_counts(Python::assume_gil_acquired()); } } @@ -389,6 +396,7 @@ impl GILPool { pub unsafe fn new() -> GILPool { increment_gil_count(); // Update counts of PyObjects / Py that have been cloned or dropped since last acquisition + #[cfg(feature = "reference-pool")] POOL.update_counts(Python::assume_gil_acquired()); GILPool { start: OWNED_OBJECTS @@ -451,7 +459,10 @@ pub unsafe fn register_incref(obj: NonNull) { if gil_is_acquired() { ffi::Py_INCREF(obj.as_ptr()) } else { + #[cfg(feature = "reference-pool")] POOL.register_incref(obj); + #[cfg(not(feature = "reference-pool"))] + panic!("Cannot clone pointer into Python heap without the GIL being held."); } } @@ -467,7 +478,10 @@ pub unsafe fn register_decref(obj: NonNull) { if gil_is_acquired() { ffi::Py_DECREF(obj.as_ptr()) } else { + #[cfg(feature = "reference-pool")] POOL.register_decref(obj); + #[cfg(all(not(feature = "reference-pool"), debug_assertions))] + eprintln!("Leaking pointer into Python heap because the GIL is not held."); } } @@ -520,10 +534,12 @@ fn decrement_gil_count() { mod tests { #[allow(deprecated)] use super::GILPool; - use super::{gil_is_acquired, GIL_COUNT, OWNED_OBJECTS, POOL}; + use super::{gil_is_acquired, GIL_COUNT, OWNED_OBJECTS}; + #[cfg(feature = "reference-pool")] + use super::POOL; use crate::types::any::PyAnyMethods; use crate::{ffi, gil, PyObject, Python}; - #[cfg(not(target_arch = "wasm32"))] + #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] use parking_lot::{const_mutex, Condvar, Mutex}; use std::ptr::NonNull; @@ -539,6 +555,7 @@ mod tests { len } + #[cfg(feature = "reference-pool")] fn pool_inc_refs_does_not_contain(obj: &PyObject) -> bool { !POOL .pointer_ops @@ -547,6 +564,7 @@ mod tests { .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } + #[cfg(feature = "reference-pool")] fn pool_dec_refs_does_not_contain(obj: &PyObject) -> bool { !POOL .pointer_ops @@ -555,7 +573,7 @@ mod tests { .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } - #[cfg(not(target_arch = "wasm32"))] + #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] fn pool_dec_refs_contains(obj: &PyObject) -> bool { POOL.pointer_ops .lock() @@ -632,20 +650,24 @@ mod tests { let reference = obj.clone_ref(py); assert_eq!(obj.get_refcnt(py), 2); + #[cfg(feature = "reference-pool")] assert!(pool_inc_refs_does_not_contain(&obj)); + #[cfg(feature = "reference-pool")] assert!(pool_dec_refs_does_not_contain(&obj)); // With the GIL held, reference count will be decreased immediately. drop(reference); assert_eq!(obj.get_refcnt(py), 1); + #[cfg(feature = "reference-pool")] assert!(pool_inc_refs_does_not_contain(&obj)); + #[cfg(feature = "reference-pool")] assert!(pool_dec_refs_does_not_contain(&obj)); }); } #[test] - #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled + #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] // We are building wasm Python with pthreads disabled fn test_pyobject_drop_without_gil_doesnt_decrease_refcnt() { let obj = Python::with_gil(|py| { let obj = get_object(py); @@ -729,6 +751,7 @@ mod tests { } #[test] + #[cfg_attr(not(feature = "reference-pool"), should_panic)] fn test_allow_threads_updates_refcounts() { Python::with_gil(|py| { // Make a simple object with 1 reference @@ -768,13 +791,13 @@ mod tests { }) } - #[cfg(not(target_arch = "wasm32"))] + #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] struct Event { set: Mutex, wait: Condvar, } - #[cfg(not(target_arch = "wasm32"))] + #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] impl Event { const fn new() -> Self { Self { @@ -797,7 +820,7 @@ mod tests { } #[test] - #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled + #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] // We are building wasm Python with pthreads disabled fn test_clone_without_gil() { use crate::{Py, PyAny}; use std::{sync::Arc, thread}; @@ -862,7 +885,7 @@ mod tests { } #[test] - #[cfg(not(target_arch = "wasm32"))] // We are building wasm Python with pthreads disabled + #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] // We are building wasm Python with pthreads disabled fn test_clone_in_other_thread() { use crate::Py; use std::{sync::Arc, thread}; @@ -908,6 +931,7 @@ mod tests { } #[test] + #[cfg(feature = "reference-pool")] fn test_update_counts_does_not_deadlock() { // update_counts can run arbitrary Python code during Py_DECREF. // if the locking is implemented incorrectly, it will deadlock. diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index 8c6a1c04915..78f565d4c46 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -229,7 +229,7 @@ impl UnsendableChild { } fn test_unsendable() -> PyResult<()> { - let obj = Python::with_gil(|py| -> PyResult<_> { + let (obj, keep_obj_here) = Python::with_gil(|py| -> PyResult<_> { let obj: Py = PyType::new_bound::(py).call1((5,))?.extract()?; // Accessing the value inside this thread should not panic @@ -241,10 +241,10 @@ fn test_unsendable() -> PyResult<()> { .is_err(); assert!(!caught_panic); - Ok(obj) - })?; - let keep_obj_here = obj.clone(); + let keep_obj_here = obj.clone(); + Ok((obj, keep_obj_here)) + })?; let caught_panic = std::thread::spawn(move || { // This access must panic diff --git a/tests/test_gc.rs b/tests/test_gc.rs index 11d9221c7ad..9b6b356c6ef 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -445,6 +445,7 @@ impl DropDuringTraversal { } } +#[cfg(feature = "reference-pool")] #[test] fn drop_during_traversal_with_gil() { let drop_called = Arc::new(AtomicBool::new(false)); @@ -476,6 +477,7 @@ fn drop_during_traversal_with_gil() { assert!(drop_called.load(Ordering::Relaxed)); } +#[cfg(feature = "reference-pool")] #[test] fn drop_during_traversal_without_gil() { let drop_called = Arc::new(AtomicBool::new(false));