Merge #3158

3158: Add Py::get for GIL-independent access to frozen classes. r=davidhewitt a=adamreichold `@davidhewitt` Is this what you had in mind for #3154? The name is an obvious candidate for bikeshedding. Trying to write an example, I noticed that making `PyCell::get_frozen` public is most likely not useful as there is no way to safely get a `&PyCell` without acquiring the GIL first? Co-authored-by: Adam Reichold <[email protected]>
PyO3 · May 18, 2023 · c2986df · c2986df
2 parents 3b4c7d3 + b9766cf
commit c2986df
Show file tree

Hide file tree

Showing 8 changed files with 173 additions and 9 deletions.
diff --git 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. |
 | <span style="white-space: pre">`extends = BaseType`</span>  | Use a custom baseclass. Defaults to [`PyAny`][params-1] |
 | <span style="white-space: pre">`freelist = N`</span> |  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.  |
-| <span style="white-space: pre">`frozen`</span> | 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. |
+| <span style="white-space: pre">`frozen`</span> | 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. |
 | <span style="white-space: pre">`module = "module_name"`</span> |  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
@@ -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<FrozenCounter>  = 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
@@ -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
@@ -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<PyRef<'py, T>, 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<Frozen = True> + 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<T> = PyNativeType::unchecked_downcast(&*any);
+            &*cell.get_ptr()
+        }
+    }
 }
 
 impl<T> Py<T> {

diff --git 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<T: PyClass> PyCell<T> {
 
     /// 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<T: PyClass> PyCell<T> {
     ///
     /// 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<T: PyClass> PyCell<T> {
             .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<T>`,
+    /// 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<Frozen = True> + 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<T: PyClass> PyCell<T> {
         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
@@ -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<FrozenCounter> = 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
@@ -6,7 +6,7 @@ pub struct Foo {
     field: u32,
 }
 
-fn borrow_mut_fails(foo: Py<Foo>, py: Python){
+fn borrow_mut_fails(foo: Py<Foo>, 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<ImmutableChild>, py: Python){
+fn borrow_mut_of_child_fails(child: Py<ImmutableChild>, py: Python) {
     let borrow = child.as_ref(py).borrow_mut();
 }
 
-fn main(){}
+fn py_get_of_mutable_class_fails(class: Py<MutableBase>) {
+    class.get();
+}
+
+fn pyclass_get_of_mutable_class_fails(class: &PyCell<MutableBase>) {
+    class.get();
+}
+
+fn main() {}
diff --git 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 `<Foo as PyClass>::Frozen == False`
 10 |     let borrow = foo.as_ref(py).borrow_mut();
    |                                 ^^^^^^^^^^ expected `False`, found `True`
    |
-note: required by a bound in `PyCell::<T>::borrow_mut`
+note: required by a bound in `pyo3::PyCell::<T>::borrow_mut`
   --> src/pycell.rs
    |
    |         T: PyClass<Frozen = False>,
@@ -16,8 +16,32 @@ error[E0271]: type mismatch resolving `<ImmutableChild as PyClass>::Frozen == Fa
 20 |     let borrow = child.as_ref(py).borrow_mut();
    |                                   ^^^^^^^^^^ expected `False`, found `True`
    |
-note: required by a bound in `PyCell::<T>::borrow_mut`
+note: required by a bound in `pyo3::PyCell::<T>::borrow_mut`
   --> src/pycell.rs
    |
    |         T: PyClass<Frozen = False>,
    |                    ^^^^^^^^^^^^^^ required by this bound in `PyCell::<T>::borrow_mut`
+
+error[E0271]: type mismatch resolving `<MutableBase as PyClass>::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::<T>::get`
+  --> src/instance.rs
+   |
+   |         T: PyClass<Frozen = True> + Sync,
+   |                    ^^^^^^^^^^^^^ required by this bound in `Py::<T>::get`
+
+error[E0271]: type mismatch resolving `<MutableBase as PyClass>::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::<T>::get`
+  --> src/pycell.rs
+   |
+   |         T: PyClass<Frozen = True> + Sync,
+   |                    ^^^^^^^^^^^^^ required by this bound in `PyCell::<T>::get`
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Add `PyClass::get` and `Py::get` for GIL-indepedent access to internally synchronized frozen classes.