From eb121f040f48d89febba3ca2ef272ff03613f522 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Thu, 12 Aug 2021 12:11:21 +0200 Subject: [PATCH] Cheaper and safe caching for py_capsule! and py_capsule_fn! The `py_capsule!` and `py_capsule_fn!` macros generate `retrieve` functions that cache their results for subsequent calls. Prior to this commit, caching is done with a generated unsafe `static mut` item whose access is made thread-safe by a `std::sync::Once` on the side. However this synchronization is unnecessary since `retreive` takes a `Python<'_>` parameter that indicates that the GIL is held. This changes the cache to use safe `static` item instead, with `GILProtected` for synchronization-free thread-safety and `OnceCell` for interior mutability. As a bonus, this removes the need for cache-related `unsafe` code in generated `retrieve` functions. This adds a dependency to the `once_cell` crate, which can be removed when `OnceCell` becomes stable in the standard library: https://github.com/rust-lang/rust/issues/74465 Alternatively `OnceCell` could be replaced with `UnsafeCell` plus some `unsafe` code, effectively inlining the core of the logic of `OnceCell`. `RefCell` might work too. Fixes https://github.com/dgrunwald/rust-cpython/issues/263 --- Cargo.toml | 10 ++++++++-- src/lib.rs | 1 + src/objects/capsule.rs | 38 +++++++++++++++++--------------------- 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index be215248..e6e6eb06 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,6 +35,12 @@ appveyor = { repository = "dgrunwald/rust-cpython" } [dependencies] libc = "0.2" num-traits = "0.2" + +# TODO: use `std::lazy::OnceCell` instead and when it’s stable +# and this crate requires a minimum Rust version that has it stable: +# https://github.com/rust-lang/rust/issues/74465 +once_cell = "1.8" + paste = "1" serde = { version = "1", features = ["derive"], optional = true } @@ -43,7 +49,7 @@ rustversion = "1.0" serde_bytes = { version = "0.11" } serde_cbor = { version = "0.11" } -# These features are both optional, but you must pick one to +# These features are both optional, but you must pick one to # indicate which python ffi you are trying to bind to. [dependencies.python27-sys] optional = true @@ -83,7 +89,7 @@ py-link-mode-default = [ "python3-sys/link-mode-default" ] py-link-mode-unresolved-static = [ "python3-sys/link-mode-unresolved-static" ] # Optional features to support explicitly specifying python minor version. -# If you don't care which minor version, just specify python3-sys as a +# If you don't care which minor version, just specify python3-sys as a # feature. python-3-9 = ["python3-sys/python-3-9"] python-3-8 = ["python3-sys/python-3-8"] diff --git a/src/lib.rs b/src/lib.rs index 80e42746..6ae6ec64 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -235,6 +235,7 @@ pub mod _detail { handle_callback, py_fn_impl, AbortOnDrop, PyObjectCallbackConverter, PythonObjectCallbackConverter, }; + pub use once_cell::unsync::OnceCell; pub use paste; } diff --git a/src/objects/capsule.rs b/src/objects/capsule.rs index aaf7adf4..6aab5739 100644 --- a/src/objects/capsule.rs +++ b/src/objects/capsule.rs @@ -347,28 +347,27 @@ macro_rules! py_capsule { (from $($capsmod:ident).+ import $capsname:ident as $rustmod:ident for $ruststruct: ident ) => ( mod $rustmod { use super::*; - use std::sync::Once; + use $crate::_detail::OnceCell; + use $crate::GILProtected; use $crate::PyClone; - static mut CAPS_DATA: Option<$crate::PyResult<&$ruststruct>> = None; - - static INIT: Once = Once::new(); + static CAPS_DATA: GILProtected>> = + GILProtected::new(OnceCell::new()); pub type RawPyObject = $crate::_detail::ffi::PyObject; pub unsafe fn retrieve<'a>(py: $crate::Python) -> $crate::PyResult<&'a $ruststruct> { - INIT.call_once(|| { + let ref_to_result = CAPS_DATA.get(py).get_or_init(|| { let caps_name = std::ffi::CStr::from_bytes_with_nul_unchecked( concat!($( stringify!($capsmod), "."),*, stringify!($capsname), "\0").as_bytes()); - CAPS_DATA = Some($crate::PyCapsule::import_data(py, caps_name)); + $crate::PyCapsule::import_data(py, caps_name) }); - match CAPS_DATA { - Some(Ok(d)) => Ok(d), - Some(Err(ref e)) => Err(e.clone_ref(py)), - _ => panic!("Uninitialized"), // can't happen + match ref_to_result { + &Ok(ref x) => Ok(x), + &Err(ref e) => Err(e.clone_ref(py)) } } } @@ -397,7 +396,7 @@ macro_rules! py_capsule { /// ```ignore /// mod $rustmod { /// pub type CapsuleFn = unsafe extern "C" (args) -> ret_type ; -/// pub unsafe fn retrieve<'a>(py: Python) -> PyResult(py: Python) -> PyResult { ... } /// } /// ``` /// - a `RawPyObject` type suitable for signatures that involve Python C objects; @@ -482,15 +481,15 @@ macro_rules! py_capsule_fn { (from $($capsmod:ident).+ import $capsname:ident as $rustmod:ident signature $( $sig: tt)* ) => ( mod $rustmod { use super::*; - use std::sync::Once; + use $crate::_detail::OnceCell; + use $crate::GILProtected; use $crate::PyClone; pub type CapsuleFn = unsafe extern "C" fn $( $sig )*; pub type RawPyObject = $crate::_detail::ffi::PyObject; - static mut CAPS_FN: Option<$crate::PyResult> = None; - - static INIT: Once = Once::new(); + static CAPS_FN: GILProtected>> = + GILProtected::new(OnceCell::new()); fn import(py: $crate::Python) -> $crate::PyResult { unsafe { @@ -504,12 +503,9 @@ macro_rules! py_capsule_fn { } pub fn retrieve(py: $crate::Python) -> $crate::PyResult { - unsafe { - INIT.call_once(|| { CAPS_FN = Some(import(py)) }); - match CAPS_FN.as_ref().unwrap() { - &Ok(f) => Ok(f), - &Err(ref e) => Err(e.clone_ref(py)), - } + match CAPS_FN.get(py).get_or_init(|| import(py)) { + &Ok(f) => Ok(f), + &Err(ref e) => Err(e.clone_ref(py)), } } }