From b9766cfa1146cca2ded5377c7eea0cd39526f533 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Tue, 16 May 2023 21:48:59 +0200 Subject: [PATCH] Add PyClass::get and Py::get for GIL-independent access to frozen classes. --- guide/pyclass_parameters.md | 2 +- guide/src/class.md | 26 +++++++++++ newsfragments/3158.added.md | 1 + src/instance.rs | 41 ++++++++++++++++- src/pycell.rs | 46 ++++++++++++++++++- tests/test_class_basics.rs | 24 ++++++++++ tests/ui/invalid_frozen_pyclass_borrow.rs | 14 ++++-- tests/ui/invalid_frozen_pyclass_borrow.stderr | 28 ++++++++++- 8 files changed, 173 insertions(+), 9 deletions(-) create mode 100644 newsfragments/3158.added.md diff --git a/guide/pyclass_parameters.md b/guide/pyclass_parameters.md index 8a88c4debb5..0ed15be6315 100644 --- a/guide/pyclass_parameters.md +++ b/guide/pyclass_parameters.md @@ -6,7 +6,7 @@ | `dict` | Gives instances of this class an empty `__dict__` to store custom attributes. | | `extends = BaseType` | Use a custom baseclass. Defaults to [`PyAny`][params-1] | | `freelist = N` | Implements a [free list][params-2] of size N. This can improve performance for types that are often created and deleted in quick succession. Profile your code to see whether `freelist` is right for you. | -| `frozen` | Declares that your pyclass is immutable. It removes the borrowchecker overhead when retrieving a shared reference to the Rust struct, but disables the ability to get a mutable reference. | +| `frozen` | Declares that your pyclass is immutable. It removes the borrow checker overhead when retrieving a shared reference to the Rust struct, but disables the ability to get a mutable reference. | | `get_all` | Generates getters for all fields of the pyclass. | | `mapping` | Inform PyO3 that this class is a [`Mapping`][params-mapping], and so leave its implementation of sequence C-API slots empty. | | `module = "module_name"` | Python code will see the class as being defined in this module. Defaults to `builtins`. | diff --git a/guide/src/class.md b/guide/src/class.md index a5aadabedea..dae5cbc04ec 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -211,6 +211,32 @@ Python::with_gil(|py| { }); ``` +### frozen classes: Opting out of interior mutability + +As detailed above, runtime borrow checking is currently enabled by default. But a class can opt of out it by declaring itself `frozen`. It can still use interior mutability via standard Rust types like `RefCell` or `Mutex`, but it is not bound to the implementation provided by PyO3 and can choose the most appropriate strategy on field-by-field basis. + +Classes which are `frozen` and also `Sync`, e.g. they do use `Mutex` but not `RefCell`, can be accessed without needing the Python GIL via the `PyCell::get` and `Py::get` methods: + +```rust +use std::sync::atomic::{AtomicUsize, Ordering}; +# use pyo3::prelude::*; + +#[pyclass(frozen)] +struct FrozenCounter { + value: AtomicUsize, +} + +let py_counter: Py = Python::with_gil(|py| { + let counter = FrozenCounter { value: AtomicUsize::new(0) }; + + Py::new(py, counter).unwrap() +}); + +py_counter.get().value.fetch_add(1, Ordering::Relaxed); +``` + +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. + ## Customizing the class {{#include ../pyclass_parameters.md}} diff --git a/newsfragments/3158.added.md b/newsfragments/3158.added.md new file mode 100644 index 00000000000..ff57b78f676 --- /dev/null +++ b/newsfragments/3158.added.md @@ -0,0 +1 @@ +Add `PyClass::get` and `Py::get` for GIL-indepedent access to internally synchronized frozen classes. diff --git a/src/instance.rs b/src/instance.rs index 7cf64247a30..d3927768ec9 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -1,9 +1,9 @@ -use crate::pyclass::boolean_struct::False; // Copyright (c) 2017-present PyO3 Project and Contributors use crate::conversion::PyTryFrom; use crate::err::{self, PyDowncastError, PyErr, PyResult}; use crate::gil; use crate::pycell::{PyBorrowError, PyBorrowMutError, PyCell}; +use crate::pyclass::boolean_struct::{False, True}; use crate::types::{PyDict, PyString, PyTuple}; use crate::{ ffi, AsPyPointer, FromPyObject, IntoPy, IntoPyPointer, PyAny, PyClass, PyClassInitializer, @@ -366,6 +366,8 @@ where /// This borrow lasts while the returned [`PyRef`] exists. /// Multiple immutable borrows can be taken out at the same time. /// + /// For frozen classes, the simpler [`get`][Self::get] is available. + /// /// Equivalent to `self.as_ref(py).borrow()` - /// see [`PyCell::borrow`](crate::pycell::PyCell::borrow). /// @@ -444,6 +446,8 @@ where /// /// This is the non-panicking variant of [`borrow`](#method.borrow). /// + /// For frozen classes, the simpler [`get`][Self::get] is available. + /// /// Equivalent to `self.as_ref(py).borrow_mut()` - /// see [`PyCell::try_borrow`](crate::pycell::PyCell::try_borrow). pub fn try_borrow<'py>(&'py self, py: Python<'py>) -> Result, PyBorrowError> { @@ -467,6 +471,41 @@ where { self.as_ref(py).try_borrow_mut() } + + /// Provide an immutable borrow of the value `T` without acquiring the GIL. + /// + /// This is available if the class is [`frozen`][macro@crate::pyclass] and [`Sync`]. + /// + /// # Examples + /// + /// ``` + /// use std::sync::atomic::{AtomicUsize, Ordering}; + /// # use pyo3::prelude::*; + /// + /// #[pyclass(frozen)] + /// struct FrozenCounter { + /// value: AtomicUsize, + /// } + /// + /// let cell = Python::with_gil(|py| { + /// let counter = FrozenCounter { value: AtomicUsize::new(0) }; + /// + /// Py::new(py, counter).unwrap() + /// }); + /// + /// cell.get().value.fetch_add(1, Ordering::Relaxed); + /// ``` + pub fn get(&self) -> &T + where + T: PyClass + Sync, + { + let any = self.as_ptr() as *const PyAny; + // SAFETY: The class itself is frozen and `Sync` and we do not access anything but `cell.contents.value`. + unsafe { + let cell: &PyCell = PyNativeType::unchecked_downcast(&*any); + &*cell.get_ptr() + } + } } impl Py { diff --git a/src/pycell.rs b/src/pycell.rs index dbd5fff9b9b..7e301c3e2d7 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -195,7 +195,10 @@ use crate::exceptions::PyRuntimeError; use crate::impl_::pyclass::{ PyClassBaseType, PyClassDict, PyClassImpl, PyClassThreadChecker, PyClassWeakRef, }; -use crate::pyclass::{boolean_struct::False, PyClass}; +use crate::pyclass::{ + boolean_struct::{False, True}, + PyClass, +}; use crate::pyclass_init::PyClassInitializer; use crate::type_object::{PyLayout, PySizedLayout}; use crate::types::PyAny; @@ -290,6 +293,8 @@ impl PyCell { /// Immutably borrows the value `T`. This borrow lasts as long as the returned `PyRef` exists. /// + /// For frozen classes, the simpler [`get`][Self::get] is available. + /// /// # Panics /// /// Panics if the value is currently mutably borrowed. For a non-panicking variant, use @@ -316,6 +321,8 @@ impl PyCell { /// /// This is the non-panicking variant of [`borrow`](#method.borrow). /// + /// For frozen classes, the simpler [`get`][Self::get] is available. + /// /// # Examples /// /// ``` @@ -410,6 +417,41 @@ impl PyCell { .map(|_: ()| &*self.contents.value.get()) } + /// Provide an immutable borrow of the value `T` without acquiring the GIL. + /// + /// This is available if the class is [`frozen`][macro@crate::pyclass] and [`Sync`]. + /// + /// While the GIL is usually required to get access to `&PyCell`, + /// compared to [`borrow`][Self::borrow] or [`try_borrow`][Self::try_borrow] + /// this avoids any thread or borrow checking overhead at runtime. + /// + /// # Examples + /// + /// ``` + /// use std::sync::atomic::{AtomicUsize, Ordering}; + /// # use pyo3::prelude::*; + /// + /// #[pyclass(frozen)] + /// struct FrozenCounter { + /// value: AtomicUsize, + /// } + /// + /// Python::with_gil(|py| { + /// let counter = FrozenCounter { value: AtomicUsize::new(0) }; + /// + /// let cell = PyCell::new(py, counter).unwrap(); + /// + /// cell.get().value.fetch_add(1, Ordering::Relaxed); + /// }); + /// ``` + pub fn get(&self) -> &T + where + T: PyClass + Sync, + { + // SAFETY: The class itself is frozen and `Sync` and we do not access anything but `self.contents.value`. + unsafe { &*self.get_ptr() } + } + /// Replaces the wrapped value with a new one, returning the old value. /// /// # Panics @@ -450,7 +492,7 @@ impl PyCell { std::mem::swap(&mut *self.borrow_mut(), &mut *other.borrow_mut()) } - fn get_ptr(&self) -> *mut T { + pub(crate) fn get_ptr(&self) -> *mut T { self.contents.value.get() } diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index 59ba5fbadba..ac3c98d0844 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -502,3 +502,27 @@ fn inherited_weakref() { ); }); } + +#[test] +fn access_frozen_class_without_gil() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + #[pyclass(frozen)] + struct FrozenCounter { + value: AtomicUsize, + } + + let py_counter: Py = Python::with_gil(|py| { + let counter = FrozenCounter { + value: AtomicUsize::new(0), + }; + + let cell = PyCell::new(py, counter).unwrap(); + + cell.get().value.fetch_add(1, Ordering::Relaxed); + + cell.into() + }); + + assert_eq!(py_counter.get().value.load(Ordering::Relaxed), 1); +} diff --git a/tests/ui/invalid_frozen_pyclass_borrow.rs b/tests/ui/invalid_frozen_pyclass_borrow.rs index 37f7ff69ee0..c7b2f27b5b1 100644 --- a/tests/ui/invalid_frozen_pyclass_borrow.rs +++ b/tests/ui/invalid_frozen_pyclass_borrow.rs @@ -6,7 +6,7 @@ pub struct Foo { field: u32, } -fn borrow_mut_fails(foo: Py, py: Python){ +fn borrow_mut_fails(foo: Py, py: Python) { let borrow = foo.as_ref(py).borrow_mut(); } @@ -16,8 +16,16 @@ struct MutableBase; #[pyclass(frozen, extends = MutableBase)] struct ImmutableChild; -fn borrow_mut_of_child_fails(child: Py, py: Python){ +fn borrow_mut_of_child_fails(child: Py, py: Python) { let borrow = child.as_ref(py).borrow_mut(); } -fn main(){} +fn py_get_of_mutable_class_fails(class: Py) { + class.get(); +} + +fn pyclass_get_of_mutable_class_fails(class: &PyCell) { + class.get(); +} + +fn main() {} diff --git a/tests/ui/invalid_frozen_pyclass_borrow.stderr b/tests/ui/invalid_frozen_pyclass_borrow.stderr index 9755fe25469..b91d5c0cefb 100644 --- a/tests/ui/invalid_frozen_pyclass_borrow.stderr +++ b/tests/ui/invalid_frozen_pyclass_borrow.stderr @@ -4,7 +4,7 @@ error[E0271]: type mismatch resolving `::Frozen == False` 10 | let borrow = foo.as_ref(py).borrow_mut(); | ^^^^^^^^^^ expected `False`, found `True` | -note: required by a bound in `PyCell::::borrow_mut` +note: required by a bound in `pyo3::PyCell::::borrow_mut` --> src/pycell.rs | | T: PyClass, @@ -16,8 +16,32 @@ error[E0271]: type mismatch resolving `::Frozen == Fa 20 | let borrow = child.as_ref(py).borrow_mut(); | ^^^^^^^^^^ expected `False`, found `True` | -note: required by a bound in `PyCell::::borrow_mut` +note: required by a bound in `pyo3::PyCell::::borrow_mut` --> src/pycell.rs | | T: PyClass, | ^^^^^^^^^^^^^^ required by this bound in `PyCell::::borrow_mut` + +error[E0271]: type mismatch resolving `::Frozen == True` + --> tests/ui/invalid_frozen_pyclass_borrow.rs:24:11 + | +24 | class.get(); + | ^^^ expected `True`, found `False` + | +note: required by a bound in `pyo3::Py::::get` + --> src/instance.rs + | + | T: PyClass + Sync, + | ^^^^^^^^^^^^^ required by this bound in `Py::::get` + +error[E0271]: type mismatch resolving `::Frozen == True` + --> tests/ui/invalid_frozen_pyclass_borrow.rs:28:11 + | +28 | class.get(); + | ^^^ expected `True`, found `False` + | +note: required by a bound in `pyo3::PyCell::::get` + --> src/pycell.rs + | + | T: PyClass + Sync, + | ^^^^^^^^^^^^^ required by this bound in `PyCell::::get`