From d50071b85e9a0631a138768f3ea448c7bc3cd4de Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Thu, 18 Apr 2024 19:15:52 +0200 Subject: [PATCH 01/10] Add feature controlling the global reference pool to enable avoiding its overhead. --- Cargo.toml | 5 ++++- examples/Cargo.toml | 2 +- src/gil.rs | 50 +++++++++++++++++++++++++++++++++++++-------- src/instance.rs | 2 ++ 4 files changed, 48 insertions(+), 11 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4c5a083060d..87b4845e138 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -65,7 +65,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"] @@ -108,6 +108,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/examples/Cargo.toml b/examples/Cargo.toml index e54b3b5cde2..81557e7f534 100644 --- a/examples/Cargo.toml +++ b/examples/Cargo.toml @@ -10,5 +10,5 @@ pyo3 = { path = "..", features = ["auto-initialize", "extension-module"] } [[example]] name = "decorator" path = "decorator/src/lib.rs" -crate_type = ["cdylib"] +crate-type = ["cdylib"] doc-scrape-examples = true diff --git a/src/gil.rs b/src/gil.rs index 29c4ffbe389..b34bcce0cd9 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -1,6 +1,8 @@ //! Interaction with Python's global interpreter lock use crate::impl_::not_send::{NotSend, NOT_SEND}; +#[cfg(not(feature = "reference-pool"))] +use crate::impl_::panic::PanicTrap; use crate::{ffi, Python}; use std::cell::Cell; #[cfg(debug_assertions)] @@ -233,12 +235,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: sync::Mutex<(PyObjVec, PyObjVec)>, } +#[cfg(feature = "reference-pool")] impl ReferencePool { const fn new() -> Self { Self { @@ -276,8 +280,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`. @@ -302,6 +308,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()); } } @@ -376,6 +383,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 @@ -434,11 +442,15 @@ impl Drop for GILPool { /// /// # Safety /// The object must be an owned Python reference. +#[track_caller] 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."); } } @@ -450,11 +462,18 @@ pub unsafe fn register_incref(obj: NonNull) { /// /// # Safety /// The object must be an owned Python reference. +#[track_caller] 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(not(feature = "reference-pool"))] + { + let _trap = PanicTrap::new("Aborting the process to avoid panic-from-drop."); + panic!("Cannot drop pointer into Python heap without the GIL being held."); + } } } @@ -508,14 +527,19 @@ fn decrement_gil_count() { mod tests { #[allow(deprecated)] use super::GILPool; - use super::{gil_is_acquired, GIL_COUNT, POOL}; + #[cfg(feature = "reference-pool")] + use super::POOL; + use super::{gil_is_acquired, GIL_COUNT}; + #[cfg(feature = "reference-pool")] + use crate::ffi; use crate::types::any::PyAnyMethods; - use crate::{ffi, PyObject, Python}; + use crate::{PyObject, Python}; #[cfg(feature = "gil-refs")] use {super::OWNED_OBJECTS, crate::gil}; + #[cfg(feature = "reference-pool")] use std::ptr::NonNull; - #[cfg(not(target_arch = "wasm32"))] + #[cfg(all(not(target_arch = "wasm32"), feature = "reference-pool"))] use std::sync; fn get_object(py: Python<'_>) -> PyObject { @@ -531,6 +555,7 @@ mod tests { len } + #[cfg(feature = "reference-pool")] fn pool_inc_refs_does_not_contain(obj: &PyObject) -> bool { !POOL .pointer_ops @@ -540,6 +565,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 @@ -549,7 +575,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() @@ -629,20 +655,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); @@ -726,6 +756,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 @@ -765,13 +796,13 @@ mod tests { }) } - #[cfg(not(target_arch = "wasm32"))] + #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] struct Event { set: sync::Mutex, wait: sync::Condvar, } - #[cfg(not(target_arch = "wasm32"))] + #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] impl Event { const fn new() -> Self { Self { @@ -795,7 +826,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}; @@ -860,7 +891,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}; @@ -906,6 +937,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/src/instance.rs b/src/instance.rs index 1c510b90be5..a143018e446 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -1807,6 +1807,7 @@ where /// Otherwise this registers the [`Py`]`` instance to have its reference count /// incremented the next time PyO3 acquires the GIL. impl Clone for Py { + #[track_caller] fn clone(&self) -> Self { unsafe { gil::register_incref(self.0); @@ -1817,6 +1818,7 @@ impl Clone for Py { /// Dropping a `Py` instance decrements the reference count on the object by 1. impl Drop for Py { + #[track_caller] fn drop(&mut self) { unsafe { gil::register_decref(self.0); From 6a374b0b7d914af52be93d14fa752ef124018878 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Fri, 19 Apr 2024 13:51:09 +0200 Subject: [PATCH 02/10] Document reference-pool feature in the performance guide. --- guide/src/performance.md | 42 +++++++++++++++++++++++++++++++++++++ newsfragments/4095.added.md | 1 + 2 files changed, 43 insertions(+) create mode 100644 newsfragments/4095.added.md diff --git a/guide/src/performance.md b/guide/src/performance.md index c47a91deee5..b99986dc9e9 100644 --- a/guide/src/performance.md +++ b/guide/src/performance.md @@ -96,3 +96,45 @@ impl PartialEq for FooBound<'_> { } } ``` + +## Disable the global reference pool + +PyO3 uses global mutable state to keep track of deferred reference count updates implied by `impl Clone for Py` and `impl Drop for Py` being called without the GIL being held. The necessary synchronization to obtain and apply these reference count updates when PyO3-based code next acquires the GIL is somewhat expensive and can become a significant part of the cost of crossing the Python-Rust boundary. + +This functionality can be avoided by disabling default features and not enabling the `reference-pool` feature. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Clone` and `Drop` implementations for `Py` which are often necessary to fulfil trait requirements imposed by existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementations but panic when `Clone` is called without the GIL being held and abort when `Drop` is called without the GIL being held. + +This limitation is important to keep in mind when this setting is used, especially when embedding Python code into a Rust application as it is quite easy to accidentally drop a `Py` returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code + +```rust,ignore +# use pyo3::prelude::*; +# use pyo3::types::PyList; +let numbers: Py = Python::with_gil(|py| PyList::empty_bound(py).unbind()); + +Python::with_gil(|py| { + numbers.bind(py).append(23).unwrap(); +}); + +Python::with_gil(|py| { + numbers.bind(py).append(42).unwrap(); +}); +``` + +will abort if the list not explicitly disposed via + +```rust +# use pyo3::prelude::*; +# use pyo3::types::PyList; +let numbers: Py = Python::with_gil(|py| PyList::empty_bound(py).unbind()); + +Python::with_gil(|py| { + numbers.bind(py).append(23).unwrap(); +}); + +Python::with_gil(|py| { + numbers.bind(py).append(42).unwrap(); +}); + +Python::with_gil(move |py| { + drop(numbers); +}); +``` diff --git a/newsfragments/4095.added.md b/newsfragments/4095.added.md new file mode 100644 index 00000000000..fd0eff08207 --- /dev/null +++ b/newsfragments/4095.added.md @@ -0,0 +1 @@ +Add `reference-pool` feature to avoid the overhead of the global reference pool at the cost of known limitations as explained in the performance section of the guide. From dc846ff609a7f65e1e410b72aed657cfd6c0885c Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 21 Apr 2024 20:14:19 +0200 Subject: [PATCH 03/10] Invert semantics of feature to disable reference pool so the new behaviour becomes opt-in --- Cargo.toml | 6 ++-- guide/src/performance.md | 2 +- newsfragments/4095.added.md | 2 +- src/gil.rs | 58 ++++++++++++++++++------------------- 4 files changed, 34 insertions(+), 34 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 87b4845e138..b565c566680 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -65,7 +65,7 @@ futures = "0.3.28" pyo3-build-config = { path = "pyo3-build-config", version = "=0.21.2", features = ["resolve-config"] } [features] -default = ["macros", "reference-pool"] +default = ["macros"] # Enables support for `async fn` for `#[pyfunction]` and `#[pymethods]`. experimental-async = ["macros", "pyo3-macros/experimental-async"] @@ -108,8 +108,8 @@ 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 = [] +# Disables global reference pool which allows `Drop`ing references to Python objects without the GIL being held. +disable-reference-pool = [] # Optimizes PyObject to Vec conversion and so on. nightly = [] diff --git a/guide/src/performance.md b/guide/src/performance.md index b99986dc9e9..8ccc3c1c765 100644 --- a/guide/src/performance.md +++ b/guide/src/performance.md @@ -101,7 +101,7 @@ impl PartialEq for FooBound<'_> { PyO3 uses global mutable state to keep track of deferred reference count updates implied by `impl Clone for Py` and `impl Drop for Py` being called without the GIL being held. The necessary synchronization to obtain and apply these reference count updates when PyO3-based code next acquires the GIL is somewhat expensive and can become a significant part of the cost of crossing the Python-Rust boundary. -This functionality can be avoided by disabling default features and not enabling the `reference-pool` feature. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Clone` and `Drop` implementations for `Py` which are often necessary to fulfil trait requirements imposed by existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementations but panic when `Clone` is called without the GIL being held and abort when `Drop` is called without the GIL being held. +This functionality can be avoided by adding the `disable-reference-pool` feature. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Clone` and `Drop` implementations for `Py` which are often necessary to fulfil trait requirements imposed by existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementations but panic when `Clone` is called without the GIL being held and abort when `Drop` is called without the GIL being held. This limitation is important to keep in mind when this setting is used, especially when embedding Python code into a Rust application as it is quite easy to accidentally drop a `Py` returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code diff --git a/newsfragments/4095.added.md b/newsfragments/4095.added.md index fd0eff08207..206bbba6446 100644 --- a/newsfragments/4095.added.md +++ b/newsfragments/4095.added.md @@ -1 +1 @@ -Add `reference-pool` feature to avoid the overhead of the global reference pool at the cost of known limitations as explained in the performance section of the guide. +Add `disable-reference-pool` feature to avoid the overhead of the global reference pool at the cost of known limitations as explained in the performance section of the guide. diff --git a/src/gil.rs b/src/gil.rs index b34bcce0cd9..5acba9c81c5 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -1,7 +1,7 @@ //! Interaction with Python's global interpreter lock use crate::impl_::not_send::{NotSend, NOT_SEND}; -#[cfg(not(feature = "reference-pool"))] +#[cfg(feature = "disable-reference-pool")] use crate::impl_::panic::PanicTrap; use crate::{ffi, Python}; use std::cell::Cell; @@ -235,14 +235,14 @@ impl Drop for GILGuard { // Vector of PyObject type PyObjVec = Vec>; -#[cfg(feature = "reference-pool")] +#[cfg(not(feature = "disable-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: sync::Mutex<(PyObjVec, PyObjVec)>, } -#[cfg(feature = "reference-pool")] +#[cfg(not(feature = "disable-reference-pool"))] impl ReferencePool { const fn new() -> Self { Self { @@ -280,10 +280,10 @@ impl ReferencePool { } } -#[cfg(feature = "reference-pool")] +#[cfg(not(feature = "disable-reference-pool"))] unsafe impl Sync for ReferencePool {} -#[cfg(feature = "reference-pool")] +#[cfg(not(feature = "disable-reference-pool"))] static POOL: ReferencePool = ReferencePool::new(); /// A guard which can be used to temporarily release the GIL and restore on `Drop`. @@ -308,7 +308,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")] + #[cfg(not(feature = "disable-reference-pool"))] POOL.update_counts(Python::assume_gil_acquired()); } } @@ -383,7 +383,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")] + #[cfg(not(feature = "disable-reference-pool"))] POOL.update_counts(Python::assume_gil_acquired()); GILPool { start: OWNED_OBJECTS @@ -447,9 +447,9 @@ pub unsafe fn register_incref(obj: NonNull) { if gil_is_acquired() { ffi::Py_INCREF(obj.as_ptr()) } else { - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-reference-pool"))] POOL.register_incref(obj); - #[cfg(not(feature = "reference-pool"))] + #[cfg(feature = "disable-reference-pool")] panic!("Cannot clone pointer into Python heap without the GIL being held."); } } @@ -467,9 +467,9 @@ pub unsafe fn register_decref(obj: NonNull) { if gil_is_acquired() { ffi::Py_DECREF(obj.as_ptr()) } else { - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-reference-pool"))] POOL.register_decref(obj); - #[cfg(not(feature = "reference-pool"))] + #[cfg(feature = "disable-reference-pool")] { let _trap = PanicTrap::new("Aborting the process to avoid panic-from-drop."); panic!("Cannot drop pointer into Python heap without the GIL being held."); @@ -527,19 +527,19 @@ fn decrement_gil_count() { mod tests { #[allow(deprecated)] use super::GILPool; - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-reference-pool"))] use super::POOL; use super::{gil_is_acquired, GIL_COUNT}; - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-reference-pool"))] use crate::ffi; use crate::types::any::PyAnyMethods; use crate::{PyObject, Python}; #[cfg(feature = "gil-refs")] use {super::OWNED_OBJECTS, crate::gil}; - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-reference-pool"))] use std::ptr::NonNull; - #[cfg(all(not(target_arch = "wasm32"), feature = "reference-pool"))] + #[cfg(all(not(target_arch = "wasm32"), not(feature = "disable-reference-pool")))] use std::sync; fn get_object(py: Python<'_>) -> PyObject { @@ -555,7 +555,7 @@ mod tests { len } - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-reference-pool"))] fn pool_inc_refs_does_not_contain(obj: &PyObject) -> bool { !POOL .pointer_ops @@ -565,7 +565,7 @@ mod tests { .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-reference-pool"))] fn pool_dec_refs_does_not_contain(obj: &PyObject) -> bool { !POOL .pointer_ops @@ -575,7 +575,7 @@ mod tests { .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } - #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] + #[cfg(all(not(feature = "disable-reference-pool"), not(target_arch = "wasm32")))] fn pool_dec_refs_contains(obj: &PyObject) -> bool { POOL.pointer_ops .lock() @@ -655,24 +655,24 @@ mod tests { let reference = obj.clone_ref(py); assert_eq!(obj.get_refcnt(py), 2); - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-reference-pool"))] assert!(pool_inc_refs_does_not_contain(&obj)); - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-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")] + #[cfg(not(feature = "disable-reference-pool"))] assert!(pool_inc_refs_does_not_contain(&obj)); - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-reference-pool"))] assert!(pool_dec_refs_does_not_contain(&obj)); }); } #[test] - #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] // We are building wasm Python with pthreads disabled + #[cfg(all(not(feature = "disable-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); @@ -756,7 +756,7 @@ mod tests { } #[test] - #[cfg_attr(not(feature = "reference-pool"), should_panic)] + #[cfg_attr(feature = "disable-reference-pool", should_panic)] fn test_allow_threads_updates_refcounts() { Python::with_gil(|py| { // Make a simple object with 1 reference @@ -796,13 +796,13 @@ mod tests { }) } - #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] + #[cfg(all(not(feature = "disable-reference-pool"), not(target_arch = "wasm32")))] struct Event { set: sync::Mutex, wait: sync::Condvar, } - #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] + #[cfg(all(not(feature = "disable-reference-pool"), not(target_arch = "wasm32")))] impl Event { const fn new() -> Self { Self { @@ -826,7 +826,7 @@ mod tests { } #[test] - #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] // We are building wasm Python with pthreads disabled + #[cfg(all(not(feature = "disable-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}; @@ -891,7 +891,7 @@ mod tests { } #[test] - #[cfg(all(feature = "reference-pool", not(target_arch = "wasm32")))] // We are building wasm Python with pthreads disabled + #[cfg(all(not(feature = "disable-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}; @@ -937,7 +937,7 @@ mod tests { } #[test] - #[cfg(feature = "reference-pool")] + #[cfg(not(feature = "disable-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. From df81e9aa49eb766daf20d39d07d6fbd6a7ee1ca5 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 21 Apr 2024 20:35:18 +0200 Subject: [PATCH 04/10] Remove delayed reference count increments as we cannot prevent reference count errors as long as these are available --- Cargo.toml | 4 + guide/src/migration.md | 9 ++ guide/src/performance.md | 4 +- newsfragments/4095.changed.md | 1 + src/err/err_state.rs | 14 ++- src/err/mod.rs | 2 +- src/gil.rs | 209 +++------------------------------- src/instance.rs | 1 + src/pybacked.rs | 6 +- 9 files changed, 48 insertions(+), 202 deletions(-) create mode 100644 newsfragments/4095.changed.md diff --git a/Cargo.toml b/Cargo.toml index b565c566680..51652455328 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -108,6 +108,9 @@ auto-initialize = [] # Allows use of the deprecated "GIL Refs" APIs. gil-refs = [] +# Enables `Clone`ing references to Python objects which aborts if the GIL is not held. +py-clone = [] + # Disables global reference pool which allows `Drop`ing references to Python objects without the GIL being held. disable-reference-pool = [] @@ -132,6 +135,7 @@ full = [ "num-bigint", "num-complex", "num-rational", + "py-clone", "rust_decimal", "serde", "smallvec", diff --git a/guide/src/migration.md b/guide/src/migration.md index 7e0420de22d..3002ae89181 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -35,7 +35,16 @@ fn increment(x: u64, amount: Option) -> u64 { x + amount.unwrap_or(1) } ``` + + +### `Py::clone` is now gated behind the `py-clone` feature +
+Click to expand +If you rely on `impl Clone for Py` to fulfil trait requirements imposed by existing Rust code written without PyO3-based code in mind, the newly introduced feature `py-clone` must be enabled. + +However, take care to note that the behaviour is different from previous versions. If `Clone` was called without the GIL being held, we tried to delay the application of these reference count increments until PyO3-based code would re-acquire it. This turned out to be impossible to implement in a sound manner and hence was removed. Now, if `Clone` is called without the GIL being held, we panic instead for which calling code might not be prepared. +Related to this, we also added a `disable-reference-pool` feature which removes the infrastructure necessary to apply delayed reference count decrements implied by `impl Drop for Py`. They do not appear to be a soundness hazard as they should lead to memory leaks in the worst case. However, the global synchronization adds significant overhead to cross the Python-Rust boundary. Enabling this feature will remove these costs and make the `Drop` implementation abort the process if called without the GIL being held instead.
## from 0.20.* to 0.21 diff --git a/guide/src/performance.md b/guide/src/performance.md index 8ccc3c1c765..4d3c381f7db 100644 --- a/guide/src/performance.md +++ b/guide/src/performance.md @@ -99,9 +99,9 @@ impl PartialEq for FooBound<'_> { ## Disable the global reference pool -PyO3 uses global mutable state to keep track of deferred reference count updates implied by `impl Clone for Py` and `impl Drop for Py` being called without the GIL being held. The necessary synchronization to obtain and apply these reference count updates when PyO3-based code next acquires the GIL is somewhat expensive and can become a significant part of the cost of crossing the Python-Rust boundary. +PyO3 uses global mutable state to keep track of deferred reference count updates implied by `impl Drop for Py` being called without the GIL being held. The necessary synchronization to obtain and apply these reference count updates when PyO3-based code next acquires the GIL is somewhat expensive and can become a significant part of the cost of crossing the Python-Rust boundary. -This functionality can be avoided by adding the `disable-reference-pool` feature. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Clone` and `Drop` implementations for `Py` which are often necessary to fulfil trait requirements imposed by existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementations but panic when `Clone` is called without the GIL being held and abort when `Drop` is called without the GIL being held. +This functionality can be avoided by adding the `disable-reference-pool` feature. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Drop` implementation for `Py` which is necessary to interoperate with existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementation but abort when `Drop` is called without the GIL being held. This limitation is important to keep in mind when this setting is used, especially when embedding Python code into a Rust application as it is quite easy to accidentally drop a `Py` returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code diff --git a/newsfragments/4095.changed.md b/newsfragments/4095.changed.md new file mode 100644 index 00000000000..7f155ae04ef --- /dev/null +++ b/newsfragments/4095.changed.md @@ -0,0 +1 @@ +`Clone`ing pointers into the Python heap has been moved behind the `py-clone` feature, as it must panic without the GIL being held as a soundness fix. diff --git a/src/err/err_state.rs b/src/err/err_state.rs index 9f85296f661..14345b275c9 100644 --- a/src/err/err_state.rs +++ b/src/err/err_state.rs @@ -5,7 +5,6 @@ use crate::{ Bound, IntoPy, Py, PyAny, PyObject, PyTypeInfo, Python, }; -#[derive(Clone)] pub(crate) struct PyErrStateNormalized { #[cfg(not(Py_3_12))] ptype: Py, @@ -63,6 +62,19 @@ impl PyErrStateNormalized { ptraceback: Py::from_owned_ptr_or_opt(py, ptraceback), } } + + pub fn clone_ref(&self, py: Python<'_>) -> Self { + Self { + #[cfg(not(Py_3_12))] + ptype: self.ptype.clone_ref(py), + pvalue: self.pvalue.clone_ref(py), + #[cfg(not(Py_3_12))] + ptraceback: self + .ptraceback + .as_ref() + .map(|ptraceback| ptraceback.clone_ref(py)), + } + } } pub(crate) struct PyErrStateLazyFnOutput { diff --git a/src/err/mod.rs b/src/err/mod.rs index 52e4b6c616a..29d4ac36294 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -837,7 +837,7 @@ impl PyErr { /// ``` #[inline] pub fn clone_ref(&self, py: Python<'_>) -> PyErr { - PyErr::from_state(PyErrState::Normalized(self.normalized(py).clone())) + PyErr::from_state(PyErrState::Normalized(self.normalized(py).clone_ref(py))) } /// Return the cause (either an exception instance, or None, set by `raise ... from ...`) diff --git a/src/gil.rs b/src/gil.rs index 5acba9c81c5..47e902e6375 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -238,41 +238,29 @@ type PyObjVec = Vec>; #[cfg(not(feature = "disable-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: sync::Mutex<(PyObjVec, PyObjVec)>, + pending_decrefs: sync::Mutex, } #[cfg(not(feature = "disable-reference-pool"))] impl ReferencePool { const fn new() -> Self { Self { - pointer_ops: sync::Mutex::new((Vec::new(), Vec::new())), + pending_decrefs: sync::Mutex::new(Vec::new()), } } - fn register_incref(&self, obj: NonNull) { - self.pointer_ops.lock().unwrap().0.push(obj); - } - fn register_decref(&self, obj: NonNull) { - self.pointer_ops.lock().unwrap().1.push(obj); + self.pending_decrefs.lock().unwrap().push(obj); } fn update_counts(&self, _py: Python<'_>) { - let mut ops = self.pointer_ops.lock().unwrap(); - if ops.0.is_empty() && ops.1.is_empty() { + let mut pending_decrefs = self.pending_decrefs.lock().unwrap(); + if pending_decrefs.is_empty() { return; } - let (increfs, decrefs) = mem::take(&mut *ops); - drop(ops); - - // Always increase reference counts first - as otherwise objects which have a - // nonzero total reference count might be incorrectly dropped by Python during - // this update. - for ptr in increfs { - unsafe { ffi::Py_INCREF(ptr.as_ptr()) }; - } + let decrefs = mem::take(&mut *pending_decrefs); + drop(pending_decrefs); for ptr in decrefs { unsafe { ffi::Py_DECREF(ptr.as_ptr()) }; @@ -442,14 +430,12 @@ impl Drop for GILPool { /// /// # Safety /// The object must be an owned Python reference. +#[cfg(feature = "py-clone")] #[track_caller] pub unsafe fn register_incref(obj: NonNull) { if gil_is_acquired() { ffi::Py_INCREF(obj.as_ptr()) } else { - #[cfg(not(feature = "disable-reference-pool"))] - POOL.register_incref(obj); - #[cfg(feature = "disable-reference-pool")] panic!("Cannot clone pointer into Python heap without the GIL being held."); } } @@ -539,8 +525,6 @@ mod tests { #[cfg(not(feature = "disable-reference-pool"))] use std::ptr::NonNull; - #[cfg(all(not(target_arch = "wasm32"), not(feature = "disable-reference-pool")))] - use std::sync; fn get_object(py: Python<'_>) -> PyObject { py.eval_bound("object()", None, None).unwrap().unbind() @@ -555,32 +539,20 @@ mod tests { len } - #[cfg(not(feature = "disable-reference-pool"))] - fn pool_inc_refs_does_not_contain(obj: &PyObject) -> bool { - !POOL - .pointer_ops - .lock() - .unwrap() - .0 - .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) - } - #[cfg(not(feature = "disable-reference-pool"))] fn pool_dec_refs_does_not_contain(obj: &PyObject) -> bool { !POOL - .pointer_ops + .pending_decrefs .lock() .unwrap() - .1 .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } #[cfg(all(not(feature = "disable-reference-pool"), not(target_arch = "wasm32")))] fn pool_dec_refs_contains(obj: &PyObject) -> bool { - POOL.pointer_ops + POOL.pending_decrefs .lock() .unwrap() - .1 .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } @@ -656,8 +628,6 @@ mod tests { assert_eq!(obj.get_refcnt(py), 2); #[cfg(not(feature = "disable-reference-pool"))] - assert!(pool_inc_refs_does_not_contain(&obj)); - #[cfg(not(feature = "disable-reference-pool"))] assert!(pool_dec_refs_does_not_contain(&obj)); // With the GIL held, reference count will be decreased immediately. @@ -665,8 +635,6 @@ mod tests { assert_eq!(obj.get_refcnt(py), 1); #[cfg(not(feature = "disable-reference-pool"))] - assert!(pool_inc_refs_does_not_contain(&obj)); - #[cfg(not(feature = "disable-reference-pool"))] assert!(pool_dec_refs_does_not_contain(&obj)); }); } @@ -680,7 +648,6 @@ mod tests { let reference = obj.clone_ref(py); assert_eq!(obj.get_refcnt(py), 2); - assert!(pool_inc_refs_does_not_contain(&obj)); assert!(pool_dec_refs_does_not_contain(&obj)); // Drop reference in a separate thread which doesn't have the GIL. @@ -689,7 +656,6 @@ mod tests { // The reference count should not have changed (the GIL has always // been held by this thread), it is remembered to release later. assert_eq!(obj.get_refcnt(py), 2); - assert!(pool_inc_refs_does_not_contain(&obj)); assert!(pool_dec_refs_contains(&obj)); obj }); @@ -697,9 +663,7 @@ mod tests { // Next time the GIL is acquired, the reference is released Python::with_gil(|py| { assert_eq!(obj.get_refcnt(py), 1); - let non_null = unsafe { NonNull::new_unchecked(obj.as_ptr()) }; - assert!(!POOL.pointer_ops.lock().unwrap().0.contains(&non_null)); - assert!(!POOL.pointer_ops.lock().unwrap().1.contains(&non_null)); + assert!(pool_dec_refs_does_not_contain(&obj)); }); } @@ -756,19 +720,14 @@ mod tests { } #[test] - #[cfg_attr(feature = "disable-reference-pool", should_panic)] + #[should_panic] fn test_allow_threads_updates_refcounts() { Python::with_gil(|py| { // Make a simple object with 1 reference let obj = get_object(py); assert!(obj.get_refcnt(py) == 1); - // Clone the object without the GIL to use internal tracking - let escaped_ref = py.allow_threads(|| obj.clone()); - // But after the block the refcounts are updated - assert!(obj.get_refcnt(py) == 2); - drop(escaped_ref); - assert!(obj.get_refcnt(py) == 1); - drop(obj); + // Clone the object without the GIL which should panic + py.allow_threads(|| obj.clone()); }); } @@ -796,146 +755,6 @@ mod tests { }) } - #[cfg(all(not(feature = "disable-reference-pool"), not(target_arch = "wasm32")))] - struct Event { - set: sync::Mutex, - wait: sync::Condvar, - } - - #[cfg(all(not(feature = "disable-reference-pool"), not(target_arch = "wasm32")))] - impl Event { - const fn new() -> Self { - Self { - set: sync::Mutex::new(false), - wait: sync::Condvar::new(), - } - } - - fn set(&self) { - *self.set.lock().unwrap() = true; - self.wait.notify_all(); - } - - fn wait(&self) { - drop( - self.wait - .wait_while(self.set.lock().unwrap(), |s| !*s) - .unwrap(), - ); - } - } - - #[test] - #[cfg(all(not(feature = "disable-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}; - - // Some events for synchronizing - static GIL_ACQUIRED: Event = Event::new(); - static OBJECT_CLONED: Event = Event::new(); - static REFCNT_CHECKED: Event = Event::new(); - - Python::with_gil(|py| { - let obj: Arc> = Arc::new(get_object(py)); - let thread_obj = Arc::clone(&obj); - - let count = obj.get_refcnt(py); - println!( - "1: The object has been created and its reference count is {}", - count - ); - - let handle = thread::spawn(move || { - Python::with_gil(move |py| { - println!("3. The GIL has been acquired on another thread."); - GIL_ACQUIRED.set(); - - // Wait while the main thread registers obj in POOL - OBJECT_CLONED.wait(); - println!("5. Checking refcnt"); - assert_eq!(thread_obj.get_refcnt(py), count); - - REFCNT_CHECKED.set(); - }) - }); - - let cloned = py.allow_threads(|| { - println!("2. The GIL has been released."); - - // Wait until the GIL has been acquired on the thread. - GIL_ACQUIRED.wait(); - - println!("4. The other thread is now hogging the GIL, we clone without it held"); - // Cloning without GIL should not update reference count - let cloned = Py::clone(&*obj); - OBJECT_CLONED.set(); - cloned - }); - - REFCNT_CHECKED.wait(); - - println!("6. The main thread has acquired the GIL again and processed the pool."); - - // Total reference count should be one higher - assert_eq!(obj.get_refcnt(py), count + 1); - - // Clone dropped - drop(cloned); - // Ensure refcount of the arc is 1 - handle.join().unwrap(); - - // Overall count is now back to the original, and should be no pending change - assert_eq!(Arc::try_unwrap(obj).unwrap().get_refcnt(py), count); - }); - } - - #[test] - #[cfg(all(not(feature = "disable-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}; - - // Some events for synchronizing - static OBJECT_CLONED: Event = Event::new(); - - let (obj, count, ptr) = Python::with_gil(|py| { - let obj = Arc::new(get_object(py)); - let count = obj.get_refcnt(py); - let thread_obj = Arc::clone(&obj); - - // Start a thread which does not have the GIL, and clone it - let t = thread::spawn(move || { - // Cloning without GIL should not update reference count - #[allow(clippy::redundant_clone)] - let _ = Py::clone(&*thread_obj); - OBJECT_CLONED.set(); - }); - - OBJECT_CLONED.wait(); - assert_eq!(count, obj.get_refcnt(py)); - - t.join().unwrap(); - let ptr = NonNull::new(obj.as_ptr()).unwrap(); - - // The pointer should appear once in the incref pool, and once in the - // decref pool (for the clone being created and also dropped) - assert!(POOL.pointer_ops.lock().unwrap().0.contains(&ptr)); - assert!(POOL.pointer_ops.lock().unwrap().1.contains(&ptr)); - - (obj, count, ptr) - }); - - Python::with_gil(|py| { - // Acquiring the gil clears the pool - assert!(!POOL.pointer_ops.lock().unwrap().0.contains(&ptr)); - assert!(!POOL.pointer_ops.lock().unwrap().1.contains(&ptr)); - - // Overall count is still unchanged - assert_eq!(count, obj.get_refcnt(py)); - }); - } - #[test] #[cfg(not(feature = "disable-reference-pool"))] fn test_update_counts_does_not_deadlock() { diff --git a/src/instance.rs b/src/instance.rs index a143018e446..1fb1b2f502d 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -1806,6 +1806,7 @@ where /// If the GIL is held this increments `self`'s reference count. /// Otherwise this registers the [`Py`]`` instance to have its reference count /// incremented the next time PyO3 acquires the GIL. +#[cfg(feature = "py-clone")] impl Clone for Py { #[track_caller] fn clone(&self) -> Self { diff --git a/src/pybacked.rs b/src/pybacked.rs index e0bacb86144..504e62e7866 100644 --- a/src/pybacked.rs +++ b/src/pybacked.rs @@ -13,7 +13,7 @@ use crate::{ /// A wrapper around `str` where the storage is owned by a Python `bytes` or `str` object. /// /// This type gives access to the underlying data via a `Deref` implementation. -#[derive(Clone)] +#[cfg_attr(feature = "py-clone", derive(Clone))] pub struct PyBackedStr { #[allow(dead_code)] // only held so that the storage is not dropped storage: Py, @@ -88,7 +88,7 @@ impl FromPyObject<'_> for PyBackedStr { /// A wrapper around `[u8]` where the storage is either owned by a Python `bytes` object, or a Rust `Box<[u8]>`. /// /// This type gives access to the underlying data via a `Deref` implementation. -#[derive(Clone)] +#[cfg_attr(feature = "py-clone", derive(Clone))] pub struct PyBackedBytes { #[allow(dead_code)] // only held so that the storage is not dropped storage: PyBackedBytesStorage, @@ -96,7 +96,7 @@ pub struct PyBackedBytes { } #[allow(dead_code)] -#[derive(Clone)] +#[cfg_attr(feature = "py-clone", derive(Clone))] enum PyBackedBytesStorage { Python(Py), Rust(Arc<[u8]>), From 207c3d7c091ef7f22ca70016e332fb392f70156b Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 21 Apr 2024 21:18:55 +0200 Subject: [PATCH 05/10] Adjust tests to be compatible with disable-reference-pool feature --- guide/src/class.md | 4 +++- guide/src/memory.md | 9 ++++++--- guide/src/migration.md | 4 ++-- src/instance.rs | 11 ++++++++--- src/marker.rs | 4 ++-- src/types/capsule.rs | 8 ++++---- src/types/iterator.rs | 2 +- src/types/sequence.rs | 2 +- tests/test_bytes.rs | 2 ++ tests/test_class_basics.rs | 11 ++++++----- tests/test_gc.rs | 2 ++ 11 files changed, 37 insertions(+), 22 deletions(-) diff --git a/guide/src/class.md b/guide/src/class.md index 3fcfaca4bdc..91a6fb2c495 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -249,7 +249,7 @@ fn return_myclass() -> Py { let obj = return_myclass(); -Python::with_gil(|py| { +Python::with_gil(move |py| { let bound = obj.bind(py); // Py::bind returns &Bound<'py, MyClass> let obj_ref = bound.borrow(); // Get PyRef assert_eq!(obj_ref.num, 1); @@ -280,6 +280,8 @@ let py_counter: Py = Python::with_gil(|py| { }); py_counter.get().value.fetch_add(1, Ordering::Relaxed); + +Python::with_gil(move |_py| drop(py_counter)); ``` Frozen classes are likely to become the default thereby guiding the PyO3 ecosystem towards a more deliberate application of interior mutability. Eventually, this should enable further optimizations of PyO3's internals and avoid downstream code paying the cost of interior mutability when it is not actually required. diff --git a/guide/src/memory.md b/guide/src/memory.md index b37d83563e5..f1b09e5b4aa 100644 --- a/guide/src/memory.md +++ b/guide/src/memory.md @@ -212,7 +212,8 @@ This example wasn't very interesting. We could have just used a GIL-bound we are *not* holding the GIL? ```rust -# #![allow(unused_imports)] +# #![allow(unused_imports, dead_code)] +# #[cfg(not(feature = "disable-reference-pool"))] { # use pyo3::prelude::*; # use pyo3::types::PyString; # fn main() -> PyResult<()> { @@ -239,12 +240,14 @@ Python::with_gil(|py| # } # Ok(()) # } +# } ``` When `hello` is dropped *nothing* happens to the pointed-to memory on Python's heap because nothing _can_ happen if we're not holding the GIL. Fortunately, -the memory isn't leaked. PyO3 keeps track of the memory internally and will -release it the next time we acquire the GIL. +the memory isn't leaked. If the `disable-reference-pool` feature is not enabled, +PyO3 keeps track of the memory internally and will release it the next time +we acquire the GIL. We can avoid the delay in releasing memory if we are careful to drop the `Py` while the GIL is held. diff --git a/guide/src/migration.md b/guide/src/migration.md index 3002ae89181..7cb5d6d543f 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -685,7 +685,7 @@ drop(second); The replacement is [`Python::with_gil`](https://docs.rs/pyo3/0.18.3/pyo3/marker/struct.Python.html#method.with_gil) which is more cumbersome but enforces the proper nesting by design, e.g. -```rust +```rust,ignore # #![allow(dead_code)] # use pyo3::prelude::*; @@ -710,7 +710,7 @@ let second = Python::with_gil(|py| Object::new(py)); drop(first); drop(second); -// Or it ensure releasing the inner lock before the outer one. +// Or it ensures releasing the inner lock before the outer one. Python::with_gil(|py| { let first = Object::new(py); let second = Python::with_gil(|py| Object::new(py)); diff --git a/src/instance.rs b/src/instance.rs index 1fb1b2f502d..70c0bb53e24 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -81,10 +81,11 @@ where /// struct Foo {/* fields omitted */} /// /// # fn main() -> PyResult<()> { - /// Python::with_gil(|py| -> PyResult> { + /// let foo: Py = Python::with_gil(|py| -> PyResult<_> { /// let foo: Bound<'_, Foo> = Bound::new(py, Foo {})?; /// Ok(foo.into()) /// })?; + /// # Python::with_gil(move |_py| drop(foo)); /// # Ok(()) /// # } /// ``` @@ -935,10 +936,11 @@ where /// struct Foo {/* fields omitted */} /// /// # fn main() -> PyResult<()> { - /// Python::with_gil(|py| -> PyResult> { + /// let foo = Python::with_gil(|py| -> PyResult<_> { /// let foo: Py = Py::new(py, Foo {})?; /// Ok(foo) /// })?; + /// # Python::with_gil(move |_py| drop(foo)); /// # Ok(()) /// # } /// ``` @@ -1244,6 +1246,7 @@ where /// }); /// /// cell.get().value.fetch_add(1, Ordering::Relaxed); + /// # Python::with_gil(move |_py| drop(cell)); /// ``` #[inline] pub fn get(&self) -> &T @@ -2042,7 +2045,9 @@ mod tests { Py::from(native) }); - assert_eq!(Python::with_gil(|py| dict.get_refcnt(py)), 1); + Python::with_gil(move |py| { + assert_eq!(dict.get_refcnt(py), 1); + }); } #[test] diff --git a/src/marker.rs b/src/marker.rs index b6821deb036..0c3a238e6ab 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -1178,7 +1178,6 @@ impl<'unbound> Python<'unbound> { mod tests { use super::*; use crate::types::{IntoPyDict, PyList}; - use std::sync::Arc; #[test] fn test_eval() { @@ -1264,11 +1263,12 @@ mod tests { }); } + #[cfg(not(feature = "disable-reference-pool"))] #[test] fn test_allow_threads_pass_stuff_in() { let list = Python::with_gil(|py| PyList::new_bound(py, vec!["foo", "bar"]).unbind()); let mut v = vec![1, 2, 3]; - let a = Arc::new(String::from("foo")); + let a = std::sync::Arc::new(String::from("foo")); Python::with_gil(|py| { py.allow_threads(|| { diff --git a/src/types/capsule.rs b/src/types/capsule.rs index c2b73a0d0f7..8628f9344aa 100644 --- a/src/types/capsule.rs +++ b/src/types/capsule.rs @@ -487,7 +487,7 @@ mod tests { cap.into() }); - Python::with_gil(|py| { + Python::with_gil(move |py| { let f = unsafe { cap.bind(py).reference:: u32>() }; assert_eq!(f(123), 123); }); @@ -551,7 +551,7 @@ mod tests { cap.into() }); - Python::with_gil(|py| { + Python::with_gil(move |py| { let ctx: &Vec = unsafe { cap.bind(py).reference() }; assert_eq!(ctx, &[1, 2, 3, 4]); }) @@ -570,7 +570,7 @@ mod tests { cap.into() }); - Python::with_gil(|py| { + Python::with_gil(move |py| { let ctx_ptr: *mut c_void = cap.bind(py).context().unwrap(); let ctx = unsafe { *Box::from_raw(ctx_ptr.cast::<&Vec>()) }; assert_eq!(ctx, &vec![1_u8, 2, 3, 4]); @@ -587,7 +587,7 @@ mod tests { context.send(true).unwrap(); } - Python::with_gil(|py| { + Python::with_gil(move |py| { let name = CString::new("foo").unwrap(); let cap = PyCapsule::new_bound_with_destructor(py, 0, Some(name), destructor).unwrap(); cap.set_context(Box::into_raw(Box::new(tx)).cast()).unwrap(); diff --git a/src/types/iterator.rs b/src/types/iterator.rs index 4562efde3f8..1835f484adf 100644 --- a/src/types/iterator.rs +++ b/src/types/iterator.rs @@ -195,7 +195,7 @@ mod tests { ); }); - Python::with_gil(|py| { + Python::with_gil(move |py| { assert_eq!(count, obj.get_refcnt(py)); }); } diff --git a/src/types/sequence.rs b/src/types/sequence.rs index f75d851973d..a5765ebc8b2 100644 --- a/src/types/sequence.rs +++ b/src/types/sequence.rs @@ -823,7 +823,7 @@ mod tests { assert!(seq.get_item(1).unwrap().as_ptr() == obj.as_ptr()); }); - Python::with_gil(|py| { + Python::with_gil(move |py| { assert_eq!(1, obj.get_refcnt(py)); }); } diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index a3f1e2fcafe..5adca3f154a 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -48,4 +48,6 @@ fn test_py_as_bytes() { let data = Python::with_gil(|py| pyobj.as_bytes(py)); assert_eq!(data, b"abc"); + + Python::with_gil(move |_py| drop(pyobj)); } diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index b7bee2638ca..f2f7b6c21ee 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 (keep_obj_here, obj) = 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,14 +241,13 @@ fn test_unsendable() -> PyResult<()> { .is_err(); assert!(!caught_panic); - Ok(obj) - })?; - let keep_obj_here = obj.clone(); + Ok((obj.clone(), obj)) + })?; let caught_panic = std::thread::spawn(move || { // This access must panic - Python::with_gil(|py| { + Python::with_gil(move |py| { obj.borrow(py); }); }) @@ -549,6 +548,8 @@ fn access_frozen_class_without_gil() { }); assert_eq!(py_counter.get().value.load(Ordering::Relaxed), 1); + + Python::with_gil(move |_py| drop(py_counter)); } #[test] diff --git a/tests/test_gc.rs b/tests/test_gc.rs index 11d9221c7ad..e3338276758 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -445,6 +445,7 @@ impl DropDuringTraversal { } } +#[cfg(not(feature = "disable-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(not(feature = "disable-reference-pool"))] #[test] fn drop_during_traversal_without_gil() { let drop_called = Arc::new(AtomicBool::new(false)); From 30daeb746ff75e2dca27ca98a0ada77367fd5f8a Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 21 Apr 2024 22:00:51 +0200 Subject: [PATCH 06/10] Adjust tests to be compatible with py-clone feature --- guide/src/faq.md | 7 +++++-- src/conversions/std/option.rs | 2 +- src/gil.rs | 2 ++ src/instance.rs | 4 ++++ src/pybacked.rs | 3 +++ src/tests/hygiene/pyclass.rs | 2 +- tests/test_buffer_protocol.rs | 2 +- tests/test_class_basics.rs | 5 ++++- tests/test_class_conversion.rs | 4 ++++ tests/test_methods.rs | 3 +++ tests/test_no_imports.rs | 3 +++ tests/test_sequence.rs | 3 +++ tests/test_serde.rs | 5 +++-- 13 files changed, 37 insertions(+), 8 deletions(-) diff --git a/guide/src/faq.md b/guide/src/faq.md index 19f9b5d50ab..b79641a3803 100644 --- a/guide/src/faq.md +++ b/guide/src/faq.md @@ -127,12 +127,10 @@ If you don't want that cloning to happen, a workaround is to allocate the field ```rust # use pyo3::prelude::*; #[pyclass] -#[derive(Clone)] struct Inner {/* fields omitted */} #[pyclass] struct Outer { - #[pyo3(get)] inner: Py, } @@ -144,6 +142,11 @@ impl Outer { inner: Py::new(py, Inner {})?, }) } + + #[getter] + fn inner(&self, py: Python<'_>) -> Py { + self.inner.clone_ref(py) + } } ``` This time `a` and `b` *are* the same object: diff --git a/src/conversions/std/option.rs b/src/conversions/std/option.rs index 2fa082ba16a..13527315e70 100644 --- a/src/conversions/std/option.rs +++ b/src/conversions/std/option.rs @@ -61,7 +61,7 @@ mod tests { assert_eq!(option.as_ptr(), std::ptr::null_mut()); let none = py.None(); - option = Some(none.clone()); + option = Some(none.clone_ref(py)); let ref_cnt = none.get_refcnt(py); assert_eq!(option.as_ptr(), none.as_ptr()); diff --git a/src/gil.rs b/src/gil.rs index 47e902e6375..8091c365d4c 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -719,6 +719,7 @@ mod tests { assert!(!gil_is_acquired()); } + #[cfg(feature = "py-clone")] #[test] #[should_panic] fn test_allow_threads_updates_refcounts() { @@ -742,6 +743,7 @@ mod tests { }) } + #[cfg(feature = "py-clone")] #[test] fn test_clone_with_gil() { Python::with_gil(|py| { diff --git a/src/instance.rs b/src/instance.rs index 70c0bb53e24..262d496a47a 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -866,7 +866,9 @@ impl IntoPy for Borrowed<'_, '_, T> { /// // All of these are valid syntax /// let second = Py::clone_ref(&first, py); /// let third = first.clone_ref(py); +/// #[cfg(feature = "py-clone")] /// let fourth = Py::clone(&first); +/// #[cfg(feature = "py-clone")] /// let fifth = first.clone(); /// /// // Disposing of our original `Py` just decrements the reference count. @@ -874,7 +876,9 @@ impl IntoPy for Borrowed<'_, '_, T> { /// /// // They all point to the same object /// assert!(second.is(&third)); +/// #[cfg(feature = "py-clone")] /// assert!(fourth.is(&fifth)); +/// #[cfg(feature = "py-clone")] /// assert!(second.is(&fourth)); /// }); /// # } diff --git a/src/pybacked.rs b/src/pybacked.rs index 504e62e7866..ea5face516b 100644 --- a/src/pybacked.rs +++ b/src/pybacked.rs @@ -336,6 +336,7 @@ mod test { is_sync::(); } + #[cfg(feature = "py-clone")] #[test] fn test_backed_str_clone() { Python::with_gil(|py| { @@ -398,6 +399,7 @@ mod test { }) } + #[cfg(feature = "py-clone")] #[test] fn test_backed_bytes_from_bytes_clone() { Python::with_gil(|py| { @@ -410,6 +412,7 @@ mod test { }); } + #[cfg(feature = "py-clone")] #[test] fn test_backed_bytes_from_bytearray_clone() { Python::with_gil(|py| { diff --git a/src/tests/hygiene/pyclass.rs b/src/tests/hygiene/pyclass.rs index 0bdb280d066..34b30a8c6f4 100644 --- a/src/tests/hygiene/pyclass.rs +++ b/src/tests/hygiene/pyclass.rs @@ -25,7 +25,7 @@ pub struct Bar { a: u8, #[pyo3(get, set)] b: Foo, - #[pyo3(get, set)] + #[pyo3(set)] c: ::std::option::Option>, } diff --git a/tests/test_buffer_protocol.rs b/tests/test_buffer_protocol.rs index dca900808a8..700bcdcd206 100644 --- a/tests/test_buffer_protocol.rs +++ b/tests/test_buffer_protocol.rs @@ -122,7 +122,7 @@ fn test_releasebuffer_unraisable_error() { let capture = UnraisableCapture::install(py); let instance = Py::new(py, ReleaseBufferError {}).unwrap(); - let env = [("ob", instance.clone())].into_py_dict_bound(py); + let env = [("ob", instance.clone_ref(py))].into_py_dict_bound(py); assert!(capture.borrow(py).capture.is_none()); diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index f2f7b6c21ee..d0e745377c8 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -172,6 +172,7 @@ fn empty_class_in_module() { }); } +#[cfg(feature = "py-clone")] #[pyclass] struct ClassWithObjectField { // It used to be that PyObject was not supported with (get, set) @@ -180,6 +181,7 @@ struct ClassWithObjectField { value: PyObject, } +#[cfg(feature = "py-clone")] #[pymethods] impl ClassWithObjectField { #[new] @@ -188,6 +190,7 @@ impl ClassWithObjectField { } } +#[cfg(feature = "py-clone")] #[test] fn class_with_object_field() { Python::with_gil(|py| { @@ -242,7 +245,7 @@ fn test_unsendable() -> PyResult<()> { assert!(!caught_panic); - Ok((obj.clone(), obj)) + Ok((obj.clone_ref(py), obj)) })?; let caught_panic = std::thread::spawn(move || { diff --git a/tests/test_class_conversion.rs b/tests/test_class_conversion.rs index ede8928f865..a46132b9586 100644 --- a/tests/test_class_conversion.rs +++ b/tests/test_class_conversion.rs @@ -54,12 +54,14 @@ impl SubClass { } } +#[cfg(feature = "py-clone")] #[pyclass] struct PolymorphicContainer { #[pyo3(get, set)] inner: Py, } +#[cfg(feature = "py-clone")] #[test] fn test_polymorphic_container_stores_base_class() { Python::with_gil(|py| { @@ -76,6 +78,7 @@ fn test_polymorphic_container_stores_base_class() { }); } +#[cfg(feature = "py-clone")] #[test] fn test_polymorphic_container_stores_sub_class() { Python::with_gil(|py| { @@ -103,6 +106,7 @@ fn test_polymorphic_container_stores_sub_class() { }); } +#[cfg(feature = "py-clone")] #[test] fn test_polymorphic_container_does_not_accept_other_types() { Python::with_gil(|py| { diff --git a/tests/test_methods.rs b/tests/test_methods.rs index 37f3b2d8bd6..615e2dba0af 100644 --- a/tests/test_methods.rs +++ b/tests/test_methods.rs @@ -874,6 +874,7 @@ fn test_from_sequence() { }); } +#[cfg(feature = "py-clone")] #[pyclass] struct r#RawIdents { #[pyo3(get, set)] @@ -882,6 +883,7 @@ struct r#RawIdents { r#subsubtype: PyObject, } +#[cfg(feature = "py-clone")] #[pymethods] impl r#RawIdents { #[new] @@ -946,6 +948,7 @@ impl r#RawIdents { } } +#[cfg(feature = "py-clone")] #[test] fn test_raw_idents() { Python::with_gil(|py| { diff --git a/tests/test_no_imports.rs b/tests/test_no_imports.rs index 3509a11f4be..89d54f4e057 100644 --- a/tests/test_no_imports.rs +++ b/tests/test_no_imports.rs @@ -143,12 +143,14 @@ fn test_basic() { }); } +#[cfg(feature = "py-clone")] #[pyo3::pyclass] struct NewClassMethod { #[pyo3(get)] cls: pyo3::PyObject, } +#[cfg(feature = "py-clone")] #[pyo3::pymethods] impl NewClassMethod { #[new] @@ -160,6 +162,7 @@ impl NewClassMethod { } } +#[cfg(feature = "py-clone")] #[test] fn test_new_class_method() { pyo3::Python::with_gil(|py| { diff --git a/tests/test_sequence.rs b/tests/test_sequence.rs index 9627f06ca75..8adba35c86a 100644 --- a/tests/test_sequence.rs +++ b/tests/test_sequence.rs @@ -248,12 +248,14 @@ fn test_inplace_repeat() { // Check that #[pyo3(get, set)] works correctly for Vec +#[cfg(feature = "py-clone")] #[pyclass] struct GenericList { #[pyo3(get, set)] items: Vec, } +#[cfg(feature = "py-clone")] #[test] fn test_generic_list_get() { Python::with_gil(|py| { @@ -266,6 +268,7 @@ fn test_generic_list_get() { }); } +#[cfg(feature = "py-clone")] #[test] fn test_generic_list_set() { Python::with_gil(|py| { diff --git a/tests/test_serde.rs b/tests/test_serde.rs index f1d5bee4bee..9e97946b5f2 100644 --- a/tests/test_serde.rs +++ b/tests/test_serde.rs @@ -11,7 +11,7 @@ mod test_serde { } #[pyclass] - #[derive(Debug, Clone, Serialize, Deserialize)] + #[derive(Debug, Serialize, Deserialize)] struct User { username: String, group: Option>, @@ -27,7 +27,8 @@ mod test_serde { }; let friend2 = User { username: "friend 2".into(), - ..friend1.clone() + group: None, + friends: vec![], }; let user = Python::with_gil(|py| { From 8e3aca6699174914b904df5dbf19bd384491512d Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 21 Apr 2024 23:01:12 +0200 Subject: [PATCH 07/10] Adjust the GIL benchmark to the updated reference pool semantics. --- pyo3-benches/benches/bench_gil.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pyo3-benches/benches/bench_gil.rs b/pyo3-benches/benches/bench_gil.rs index 59b9ff9686f..cede8836f35 100644 --- a/pyo3-benches/benches/bench_gil.rs +++ b/pyo3-benches/benches/bench_gil.rs @@ -1,4 +1,4 @@ -use codspeed_criterion_compat::{criterion_group, criterion_main, BatchSize, Bencher, Criterion}; +use codspeed_criterion_compat::{criterion_group, criterion_main, Bencher, Criterion}; use pyo3::prelude::*; @@ -9,14 +9,8 @@ fn bench_clean_acquire_gil(b: &mut Bencher<'_>) { fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) { let obj = Python::with_gil(|py| py.None()); - b.iter_batched( - || { - // Clone and drop an object so that the GILPool has work to do. - let _ = obj.clone(); - }, - |_| Python::with_gil(|_| {}), - BatchSize::NumBatches(1), - ); + // Drop the returned clone of the object so that the reference pool has work to do. + b.iter(|| Python::with_gil(|py| obj.clone_ref(py))); } fn criterion_benchmark(c: &mut Criterion) { From 69f5d932b21bb6bcfb5b853d48c3b7b480286fa2 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 28 Apr 2024 08:17:48 +0200 Subject: [PATCH 08/10] Further extend and clarify the documentation of the py-clone and disable-reference-pool features --- Cargo.toml | 4 ++-- guide/src/features.md | 10 ++++++++++ guide/src/performance.md | 2 +- src/instance.rs | 14 +++++++++++--- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 51652455328..e37e654804a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -108,10 +108,10 @@ auto-initialize = [] # Allows use of the deprecated "GIL Refs" APIs. gil-refs = [] -# Enables `Clone`ing references to Python objects which aborts if the GIL is not held. +# Enables `Clone`ing references to Python objects `Py` which panics if the GIL is not held. py-clone = [] -# Disables global reference pool which allows `Drop`ing references to Python objects without the GIL being held. +# Disables global reference pool which allows `Drop`ing references to Python objects `Py` without the GIL being held. disable-reference-pool = [] # Optimizes PyObject to Vec conversion and so on. diff --git a/guide/src/features.md b/guide/src/features.md index 07085a9e89c..8ba08b17527 100644 --- a/guide/src/features.md +++ b/guide/src/features.md @@ -75,6 +75,16 @@ This feature is a backwards-compatibility feature to allow continued use of the This feature and the APIs it enables is expected to be removed in a future PyO3 version. +### `py-clone` + +This feature was introduced to ease migration. It was found that delayed reference counts cannot be made sound and hence `Clon`ing an instance of `Py` must panic without the GIL being held. To avoid migrations introducing new panics without warning, the `Clone` implementation itself is now gated behind this feature. + +### `disable-reference-pool` + +This is a performance-oriented feature which disabled the global reference pool and the assocaited overhead for the crossing the Python-Rust boundary. However, if enabled, `Drop`ping an instance of `Py` without the GIL being held will abort the process. + +Since this feature does not really have additive semantics, it should only be enabled in leaf crates, i.e. in a crate producing a Python extension or embedding the Python interpreter. + ### `macros` This feature enables a dependency on the `pyo3-macros` crate, which provides the procedural macros portion of PyO3's API: diff --git a/guide/src/performance.md b/guide/src/performance.md index 4d3c381f7db..b5032e14569 100644 --- a/guide/src/performance.md +++ b/guide/src/performance.md @@ -103,7 +103,7 @@ PyO3 uses global mutable state to keep track of deferred reference count updates This functionality can be avoided by adding the `disable-reference-pool` feature. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Drop` implementation for `Py` which is necessary to interoperate with existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementation but abort when `Drop` is called without the GIL being held. -This limitation is important to keep in mind when this setting is used, especially when embedding Python code into a Rust application as it is quite easy to accidentally drop a `Py` returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code +This limitation is important to keep in mind when this setting is used, especially when embedding Python code into a Rust application as it is quite easy to accidentally drop a `Py` (or types containing it like `PyErr`, `PyBackedStr` or `PyBackedBytes`) returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code ```rust,ignore # use pyo3::prelude::*; diff --git a/src/instance.rs b/src/instance.rs index 262d496a47a..5e4daeb7526 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -1811,8 +1811,9 @@ where } /// If the GIL is held this increments `self`'s reference count. -/// Otherwise this registers the [`Py`]`` instance to have its reference count -/// incremented the next time PyO3 acquires the GIL. +/// Otherwise, it will panic. +/// +/// Only available if the `py-clone` feature is enabled. #[cfg(feature = "py-clone")] impl Clone for Py { #[track_caller] @@ -1824,7 +1825,14 @@ impl Clone for Py { } } -/// Dropping a `Py` instance decrements the reference count on the object by 1. +/// Dropping a `Py` instance decrements the reference count +/// on the object by one if the GIL is held. +/// +/// Otherwise and by default, this registers the underlying pointer to have its reference count +/// decremented the next time PyO3 acquires the GIL. +/// +/// However, if the `disable-reference-pool` feature is enabled, +/// it will abort the process. impl Drop for Py { #[track_caller] fn drop(&mut self) { From 2f492fd12395e44236e9875aed9256709e134e17 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sun, 28 Apr 2024 08:39:15 +0200 Subject: [PATCH 09/10] Replace disable-reference-pool feature by pyo3_disable_reference_pool conditional compilation flag Such a flag is harder to use and thereby also harder to abuse. This seems appropriate as this is purely a performance-oriented change which show only be enabled by leaf crates and brings with it additional highly implicit sources of process aborts. --- Cargo.toml | 3 --- guide/src/features.md | 8 ++++---- guide/src/memory.md | 8 ++++---- guide/src/migration.md | 2 +- guide/src/performance.md | 4 +++- newsfragments/4095.added.md | 2 +- pyo3-build-config/src/lib.rs | 1 + src/gil.rs | 36 ++++++++++++++++++------------------ src/instance.rs | 4 ++-- src/marker.rs | 2 +- tests/test_gc.rs | 4 ++-- 11 files changed, 37 insertions(+), 37 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e37e654804a..aba8fd1bc98 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -111,9 +111,6 @@ gil-refs = [] # Enables `Clone`ing references to Python objects `Py` which panics if the GIL is not held. py-clone = [] -# Disables global reference pool which allows `Drop`ing references to Python objects `Py` without the GIL being held. -disable-reference-pool = [] - # Optimizes PyObject to Vec conversion and so on. nightly = [] diff --git a/guide/src/features.md b/guide/src/features.md index 8ba08b17527..6a25d40cedc 100644 --- a/guide/src/features.md +++ b/guide/src/features.md @@ -79,11 +79,9 @@ This feature and the APIs it enables is expected to be removed in a future PyO3 This feature was introduced to ease migration. It was found that delayed reference counts cannot be made sound and hence `Clon`ing an instance of `Py` must panic without the GIL being held. To avoid migrations introducing new panics without warning, the `Clone` implementation itself is now gated behind this feature. -### `disable-reference-pool` +### `pyo3_disable_reference_pool` -This is a performance-oriented feature which disabled the global reference pool and the assocaited overhead for the crossing the Python-Rust boundary. However, if enabled, `Drop`ping an instance of `Py` without the GIL being held will abort the process. - -Since this feature does not really have additive semantics, it should only be enabled in leaf crates, i.e. in a crate producing a Python extension or embedding the Python interpreter. +This is a performance-oriented conditional compilation flag, e.g. [set via `$RUSTFLAGS`][set-configuration-options], which disabled the global reference pool and the assocaited overhead for the crossing the Python-Rust boundary. However, if enabled, `Drop`ping an instance of `Py` without the GIL being held will abort the process. ### `macros` @@ -205,3 +203,5 @@ struct User { ### `smallvec` Adds a dependency on [smallvec](https://docs.rs/smallvec) and enables conversions into its [`SmallVec`](https://docs.rs/smallvec/latest/smallvec/struct.SmallVec.html) type. + +[set-configuration-options]: https://doc.rust-lang.org/reference/conditional-compilation.html#set-configuration-options diff --git a/guide/src/memory.md b/guide/src/memory.md index f1b09e5b4aa..38a31f4d0ef 100644 --- a/guide/src/memory.md +++ b/guide/src/memory.md @@ -213,7 +213,7 @@ we are *not* holding the GIL? ```rust # #![allow(unused_imports, dead_code)] -# #[cfg(not(feature = "disable-reference-pool"))] { +# #[cfg(not(pyo3_disable_reference_pool))] { # use pyo3::prelude::*; # use pyo3::types::PyString; # fn main() -> PyResult<()> { @@ -245,9 +245,9 @@ Python::with_gil(|py| When `hello` is dropped *nothing* happens to the pointed-to memory on Python's heap because nothing _can_ happen if we're not holding the GIL. Fortunately, -the memory isn't leaked. If the `disable-reference-pool` feature is not enabled, -PyO3 keeps track of the memory internally and will release it the next time -we acquire the GIL. +the memory isn't leaked. If the `pyo3_disable_reference_pool` conditional compilation flag +is not enabled, PyO3 keeps track of the memory internally and will release it +the next time we acquire the GIL. We can avoid the delay in releasing memory if we are careful to drop the `Py` while the GIL is held. diff --git a/guide/src/migration.md b/guide/src/migration.md index 7cb5d6d543f..10b62002a02 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -44,7 +44,7 @@ If you rely on `impl Clone for Py` to fulfil trait requirements imposed by However, take care to note that the behaviour is different from previous versions. If `Clone` was called without the GIL being held, we tried to delay the application of these reference count increments until PyO3-based code would re-acquire it. This turned out to be impossible to implement in a sound manner and hence was removed. Now, if `Clone` is called without the GIL being held, we panic instead for which calling code might not be prepared. -Related to this, we also added a `disable-reference-pool` feature which removes the infrastructure necessary to apply delayed reference count decrements implied by `impl Drop for Py`. They do not appear to be a soundness hazard as they should lead to memory leaks in the worst case. However, the global synchronization adds significant overhead to cross the Python-Rust boundary. Enabling this feature will remove these costs and make the `Drop` implementation abort the process if called without the GIL being held instead. +Related to this, we also added a `pyo3_disable_reference_pool` conditional compilation flag which removes the infrastructure necessary to apply delayed reference count decrements implied by `impl Drop for Py`. They do not appear to be a soundness hazard as they should lead to memory leaks in the worst case. However, the global synchronization adds significant overhead to cross the Python-Rust boundary. Enabling this feature will remove these costs and make the `Drop` implementation abort the process if called without the GIL being held instead. ## from 0.20.* to 0.21 diff --git a/guide/src/performance.md b/guide/src/performance.md index b5032e14569..a21c4ef5832 100644 --- a/guide/src/performance.md +++ b/guide/src/performance.md @@ -101,7 +101,7 @@ impl PartialEq for FooBound<'_> { PyO3 uses global mutable state to keep track of deferred reference count updates implied by `impl Drop for Py` being called without the GIL being held. The necessary synchronization to obtain and apply these reference count updates when PyO3-based code next acquires the GIL is somewhat expensive and can become a significant part of the cost of crossing the Python-Rust boundary. -This functionality can be avoided by adding the `disable-reference-pool` feature. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Drop` implementation for `Py` which is necessary to interoperate with existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementation but abort when `Drop` is called without the GIL being held. +This functionality can be avoided by setting the `pyo3_disable_reference_pool` conditional compilation flag. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Drop` implementation for `Py` which is necessary to interoperate with existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementation but abort when `Drop` is called without the GIL being held. This limitation is important to keep in mind when this setting is used, especially when embedding Python code into a Rust application as it is quite easy to accidentally drop a `Py` (or types containing it like `PyErr`, `PyBackedStr` or `PyBackedBytes`) returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code @@ -138,3 +138,5 @@ Python::with_gil(move |py| { drop(numbers); }); ``` + +[conditional-compilation]: https://doc.rust-lang.org/reference/conditional-compilation.html diff --git a/newsfragments/4095.added.md b/newsfragments/4095.added.md index 206bbba6446..c9940f70f12 100644 --- a/newsfragments/4095.added.md +++ b/newsfragments/4095.added.md @@ -1 +1 @@ -Add `disable-reference-pool` feature to avoid the overhead of the global reference pool at the cost of known limitations as explained in the performance section of the guide. +Add `pyo3_disable_reference_pool` conditional compilation flag to avoid the overhead of the global reference pool at the cost of known limitations as explained in the performance section of the guide. diff --git a/pyo3-build-config/src/lib.rs b/pyo3-build-config/src/lib.rs index 24d3ae28124..09cb7331730 100644 --- a/pyo3-build-config/src/lib.rs +++ b/pyo3-build-config/src/lib.rs @@ -165,6 +165,7 @@ pub fn print_expected_cfgs() { println!("cargo:rustc-check-cfg=cfg(GraalPy)"); println!("cargo:rustc-check-cfg=cfg(py_sys_config, values(\"Py_DEBUG\", \"Py_REF_DEBUG\", \"Py_TRACE_REFS\", \"COUNT_ALLOCS\"))"); println!("cargo:rustc-check-cfg=cfg(invalid_from_utf8_lint)"); + println!("cargo:rustc-check-cfg=cfg(pyo3_disable_reference_pool)"); // allow `Py_3_*` cfgs from the minimum supported version up to the // maximum minor version (+1 for development for the next) diff --git a/src/gil.rs b/src/gil.rs index 8091c365d4c..d11060c6179 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -1,7 +1,7 @@ //! Interaction with Python's global interpreter lock use crate::impl_::not_send::{NotSend, NOT_SEND}; -#[cfg(feature = "disable-reference-pool")] +#[cfg(pyo3_disable_reference_pool)] use crate::impl_::panic::PanicTrap; use crate::{ffi, Python}; use std::cell::Cell; @@ -235,13 +235,13 @@ impl Drop for GILGuard { // Vector of PyObject type PyObjVec = Vec>; -#[cfg(not(feature = "disable-reference-pool"))] +#[cfg(not(pyo3_disable_reference_pool))] /// Thread-safe storage for objects which were inc_ref / dec_ref while the GIL was not held. struct ReferencePool { pending_decrefs: sync::Mutex, } -#[cfg(not(feature = "disable-reference-pool"))] +#[cfg(not(pyo3_disable_reference_pool))] impl ReferencePool { const fn new() -> Self { Self { @@ -268,10 +268,10 @@ impl ReferencePool { } } -#[cfg(not(feature = "disable-reference-pool"))] +#[cfg(not(pyo3_disable_reference_pool))] unsafe impl Sync for ReferencePool {} -#[cfg(not(feature = "disable-reference-pool"))] +#[cfg(not(pyo3_disable_reference_pool))] static POOL: ReferencePool = ReferencePool::new(); /// A guard which can be used to temporarily release the GIL and restore on `Drop`. @@ -296,7 +296,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(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_reference_pool))] POOL.update_counts(Python::assume_gil_acquired()); } } @@ -371,7 +371,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(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_reference_pool))] POOL.update_counts(Python::assume_gil_acquired()); GILPool { start: OWNED_OBJECTS @@ -453,9 +453,9 @@ pub unsafe fn register_decref(obj: NonNull) { if gil_is_acquired() { ffi::Py_DECREF(obj.as_ptr()) } else { - #[cfg(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_reference_pool))] POOL.register_decref(obj); - #[cfg(feature = "disable-reference-pool")] + #[cfg(pyo3_disable_reference_pool)] { let _trap = PanicTrap::new("Aborting the process to avoid panic-from-drop."); panic!("Cannot drop pointer into Python heap without the GIL being held."); @@ -513,17 +513,17 @@ fn decrement_gil_count() { mod tests { #[allow(deprecated)] use super::GILPool; - #[cfg(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_reference_pool))] use super::POOL; use super::{gil_is_acquired, GIL_COUNT}; - #[cfg(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_reference_pool))] use crate::ffi; use crate::types::any::PyAnyMethods; use crate::{PyObject, Python}; #[cfg(feature = "gil-refs")] use {super::OWNED_OBJECTS, crate::gil}; - #[cfg(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_reference_pool))] use std::ptr::NonNull; fn get_object(py: Python<'_>) -> PyObject { @@ -539,7 +539,7 @@ mod tests { len } - #[cfg(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_reference_pool))] fn pool_dec_refs_does_not_contain(obj: &PyObject) -> bool { !POOL .pending_decrefs @@ -548,7 +548,7 @@ mod tests { .contains(&unsafe { NonNull::new_unchecked(obj.as_ptr()) }) } - #[cfg(all(not(feature = "disable-reference-pool"), not(target_arch = "wasm32")))] + #[cfg(all(not(pyo3_disable_reference_pool), not(target_arch = "wasm32")))] fn pool_dec_refs_contains(obj: &PyObject) -> bool { POOL.pending_decrefs .lock() @@ -627,20 +627,20 @@ mod tests { let reference = obj.clone_ref(py); assert_eq!(obj.get_refcnt(py), 2); - #[cfg(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_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(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_reference_pool))] assert!(pool_dec_refs_does_not_contain(&obj)); }); } #[test] - #[cfg(all(not(feature = "disable-reference-pool"), not(target_arch = "wasm32")))] // We are building wasm Python with pthreads disabled + #[cfg(all(not(pyo3_disable_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); @@ -758,7 +758,7 @@ mod tests { } #[test] - #[cfg(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_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/src/instance.rs b/src/instance.rs index 5e4daeb7526..7835b9c79b6 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -1831,8 +1831,8 @@ impl Clone for Py { /// Otherwise and by default, this registers the underlying pointer to have its reference count /// decremented the next time PyO3 acquires the GIL. /// -/// However, if the `disable-reference-pool` feature is enabled, -/// it will abort the process. +/// However, if the `pyo3_disable_reference_pool` conditional compilation flag +/// is enabled, it will abort the process. impl Drop for Py { #[track_caller] fn drop(&mut self) { diff --git a/src/marker.rs b/src/marker.rs index 0c3a238e6ab..072b882c875 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -1263,7 +1263,7 @@ mod tests { }); } - #[cfg(not(feature = "disable-reference-pool"))] + #[cfg(not(pyo3_disable_reference_pool))] #[test] fn test_allow_threads_pass_stuff_in() { let list = Python::with_gil(|py| PyList::new_bound(py, vec!["foo", "bar"]).unbind()); diff --git a/tests/test_gc.rs b/tests/test_gc.rs index e3338276758..b95abd4adea 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -445,7 +445,7 @@ impl DropDuringTraversal { } } -#[cfg(not(feature = "disable-reference-pool"))] +#[cfg(not(pyo3_disable_reference_pool))] #[test] fn drop_during_traversal_with_gil() { let drop_called = Arc::new(AtomicBool::new(false)); @@ -477,7 +477,7 @@ fn drop_during_traversal_with_gil() { assert!(drop_called.load(Ordering::Relaxed)); } -#[cfg(not(feature = "disable-reference-pool"))] +#[cfg(not(pyo3_disable_reference_pool))] #[test] fn drop_during_traversal_without_gil() { let drop_called = Arc::new(AtomicBool::new(false)); From 04f0f05f61b655de3b3ef31aacb6b916f2bbffc5 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sat, 11 May 2024 12:28:33 +0200 Subject: [PATCH 10/10] Add pyo3_leak_on_drop_without_reference_pool to turn aborts into leaks when the global reference pool is disabled and the GIL is not held --- guide/src/performance.md | 2 +- pyo3-build-config/src/lib.rs | 1 + src/gil.rs | 5 ++++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/guide/src/performance.md b/guide/src/performance.md index a21c4ef5832..b3d160fe6b1 100644 --- a/guide/src/performance.md +++ b/guide/src/performance.md @@ -101,7 +101,7 @@ impl PartialEq for FooBound<'_> { PyO3 uses global mutable state to keep track of deferred reference count updates implied by `impl Drop for Py` being called without the GIL being held. The necessary synchronization to obtain and apply these reference count updates when PyO3-based code next acquires the GIL is somewhat expensive and can become a significant part of the cost of crossing the Python-Rust boundary. -This functionality can be avoided by setting the `pyo3_disable_reference_pool` conditional compilation flag. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Drop` implementation for `Py` which is necessary to interoperate with existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementation but abort when `Drop` is called without the GIL being held. +This functionality can be avoided by setting the `pyo3_disable_reference_pool` conditional compilation flag. This removes the global reference pool and the associated costs completely. However, it does _not_ remove the `Drop` implementation for `Py` which is necessary to interoperate with existing Rust code written without PyO3-based code in mind. To stay compatible with the wider Rust ecosystem in these cases, we keep the implementation but abort when `Drop` is called without the GIL being held. If `pyo3_leak_on_drop_without_reference_pool` is additionally enabled, objects dropped without the GIL being held will be leaked instead which is always sound but might have determinal effects like resource exhaustion in the long term. This limitation is important to keep in mind when this setting is used, especially when embedding Python code into a Rust application as it is quite easy to accidentally drop a `Py` (or types containing it like `PyErr`, `PyBackedStr` or `PyBackedBytes`) returned from `Python::with_gil` without making sure to re-acquire the GIL beforehand. For example, the following code diff --git a/pyo3-build-config/src/lib.rs b/pyo3-build-config/src/lib.rs index 09cb7331730..54aff4d10de 100644 --- a/pyo3-build-config/src/lib.rs +++ b/pyo3-build-config/src/lib.rs @@ -166,6 +166,7 @@ pub fn print_expected_cfgs() { println!("cargo:rustc-check-cfg=cfg(py_sys_config, values(\"Py_DEBUG\", \"Py_REF_DEBUG\", \"Py_TRACE_REFS\", \"COUNT_ALLOCS\"))"); println!("cargo:rustc-check-cfg=cfg(invalid_from_utf8_lint)"); println!("cargo:rustc-check-cfg=cfg(pyo3_disable_reference_pool)"); + println!("cargo:rustc-check-cfg=cfg(pyo3_leak_on_drop_without_reference_pool)"); // allow `Py_3_*` cfgs from the minimum supported version up to the // maximum minor version (+1 for development for the next) diff --git a/src/gil.rs b/src/gil.rs index d11060c6179..d4b92a4cff4 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -455,7 +455,10 @@ pub unsafe fn register_decref(obj: NonNull) { } else { #[cfg(not(pyo3_disable_reference_pool))] POOL.register_decref(obj); - #[cfg(pyo3_disable_reference_pool)] + #[cfg(all( + pyo3_disable_reference_pool, + not(pyo3_leak_on_drop_without_reference_pool) + ))] { let _trap = PanicTrap::new("Aborting the process to avoid panic-from-drop."); panic!("Cannot drop pointer into Python heap without the GIL being held.");