Merge #3029

3029: use dynamic trampoline for all getters and setters r=adamreichold a=davidhewitt

This is an extension to the "trampoline" changes made in #2705 to re-use a single trampoline for all `#[getter]`s (and similar for all `#[setters]`). It works by setting the currently-unused `closure` member of the `ffi::PyGetSetDef` structure to point at a new `struct GetSetDefClosure` which contains function pointers to the `getter` / `setter` implementations.

A universal trampoline for all `getter`, for example, then works by reading the actual getter implementation out of the `GetSetDefClosure`.

Advantages of doing this:
- Very minimal simplification to the macro code / generated code size. It made a 4.4% reduction to `test_getter_setter` generated size, which is an exaggerated result as most code will probably have lots of bulk that isn't just the macro code.

Disadvantages:
- Additional level of complexity in the `getter` and `setter` trampolines and accompanying code.
- To keep the `GetSetDefClosure` objects alive, I've added them to the static `LazyTypeObject` inner.
- Very slight performance overhead at runtime (shouldn't be more than an additional pointer read). It's so slight I couldn't measure it.

Overall I'm happy to either merge or close this based on what reviewers think!

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

newsfragments/3029.changed.md

@@ -0,0 +1 @@
+- Change `#[getter]` and `#[setter]` to use a common call "trampoline" to slightly reduce generated code size and compile times.

pyo3-macros-backend/src/pymethod.rs

@@ -582,22 +582,7 @@ pub fn impl_py_setter_def(
             #deprecations
             _pyo3::class::PySetterDef::new(
                 #python_name,
-                _pyo3::impl_::pymethods::PySetter({
-                    unsafe extern "C" fn trampoline(
-                        slf: *mut _pyo3::ffi::PyObject,
-                        value: *mut _pyo3::ffi::PyObject,
-                        closure: *mut ::std::os::raw::c_void,
-                    ) -> ::std::os::raw::c_int
-                    {
-                        _pyo3::impl_::trampoline::setter(
-                            slf,
-                            value,
-                            closure,
-                            #cls::#wrapper_ident
-                        )
-                    }
-                    trampoline
-                }),
+                _pyo3::impl_::pymethods::PySetter(#cls::#wrapper_ident),
                 #doc
             )
         })
@@ -718,20 +703,7 @@ pub fn impl_py_getter_def(
             #deprecations
             _pyo3::class::PyGetterDef::new(
                 #python_name,
-                _pyo3::impl_::pymethods::PyGetter({
-                    unsafe extern "C" fn trampoline(
-                        slf: *mut _pyo3::ffi::PyObject,
-                        closure: *mut ::std::os::raw::c_void,
-                    ) -> *mut _pyo3::ffi::PyObject
-                    {
-                        _pyo3::impl_::trampoline::getter(
-                            slf,
-                            closure,
-                            #cls::#wrapper_ident
-                        )
-                    }
-                    trampoline
-                }),
+                _pyo3::impl_::pymethods::PyGetter(#cls::#wrapper_ident),
                 #doc
             )
         })

src/impl_/pyclass/lazy_type_object.rs

@@ -9,10 +9,10 @@ use std::{
 use crate::{
     exceptions::PyRuntimeError,
     ffi,
-    pyclass::create_type_object,
+    pyclass::{create_type_object, PyClassTypeObject},
     sync::{GILOnceCell, GILProtected},
     types::PyType,
-    AsPyPointer, IntoPyPointer, Py, PyClass, PyErr, PyMethodDefType, PyObject, PyResult, Python,
+    AsPyPointer, IntoPyPointer, PyClass, PyErr, PyMethodDefType, PyObject, PyResult, Python,
 };
 
 use super::PyClassItemsIter;
@@ -23,7 +23,7 @@ pub struct LazyTypeObject<T>(LazyTypeObjectInner, PhantomData<T>);
 
 // Non-generic inner of LazyTypeObject to keep code size down
 struct LazyTypeObjectInner {
-    value: GILOnceCell<Py<PyType>>,
+    value: GILOnceCell<PyClassTypeObject>,
     // Threads which have begun initialization of the `tp_dict`. Used for
     // reentrant initialization detection.
     initializing_threads: GILProtected<RefCell<Vec<ThreadId>>>,
@@ -67,12 +67,16 @@ impl LazyTypeObjectInner {
     fn get_or_try_init<'py>(
         &'py self,
         py: Python<'py>,
-        init: fn(Python<'py>) -> PyResult<Py<PyType>>,
+        init: fn(Python<'py>) -> PyResult<PyClassTypeObject>,
         name: &str,
         items_iter: PyClassItemsIter,
     ) -> PyResult<&'py PyType> {
         (|| -> PyResult<_> {
-            let type_object = self.value.get_or_try_init(py, || init(py))?.as_ref(py);
+            let type_object = self
+                .value
+                .get_or_try_init(py, || init(py))?
+                .type_object
+                .as_ref(py);
             self.ensure_init(type_object, name, items_iter)?;
             Ok(type_object)
         })()

src/impl_/pymethods.rs

@@ -39,7 +39,6 @@ impl IPowModulo {
 
 /// `PyMethodDefType` represents different types of Python callable objects.
 /// It is used by the `#[pymethods]` attribute.
-#[derive(Debug)]
 pub enum PyMethodDefType {
     /// Represents class method
     Class(PyMethodDef),
@@ -72,10 +71,10 @@ pub struct PyCFunctionWithKeywords(pub ffi::PyCFunctionWithKeywords);
 #[cfg(not(Py_LIMITED_API))]
 #[derive(Clone, Copy, Debug)]
 pub struct PyCFunctionFastWithKeywords(pub ffi::_PyCFunctionFastWithKeywords);
-#[derive(Clone, Copy, Debug)]
-pub struct PyGetter(pub ffi::getter);
-#[derive(Clone, Copy, Debug)]
-pub struct PySetter(pub ffi::setter);
+#[derive(Clone, Copy)]
+pub struct PyGetter(pub Getter);
+#[derive(Clone, Copy)]
+pub struct PySetter(pub Setter);
 #[derive(Clone, Copy)]
 pub struct PyClassAttributeFactory(pub for<'p> fn(Python<'p>) -> PyResult<PyObject>);
 
@@ -102,18 +101,18 @@ impl PyClassAttributeDef {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone)]
 pub struct PyGetterDef {
     pub(crate) name: &'static str,
     pub(crate) meth: PyGetter,
-    doc: &'static str,
+    pub(crate) doc: &'static str,
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone)]
 pub struct PySetterDef {
     pub(crate) name: &'static str,
     pub(crate) meth: PySetter,
-    doc: &'static str,
+    pub(crate) doc: &'static str,
 }
 
 unsafe impl Sync for PyMethodDef {}
@@ -212,6 +211,12 @@ impl fmt::Debug for PyClassAttributeDef {
     }
 }
 
+/// Class getter / setters
+pub(crate) type Getter =
+    for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject) -> PyResult<*mut ffi::PyObject>;
+pub(crate) type Setter =
+    for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject, *mut ffi::PyObject) -> PyResult<c_int>;
+
 impl PyGetterDef {
     /// Define a getter.
     pub const fn new(name: &'static str, getter: PyGetter, doc: &'static str) -> Self {
@@ -221,23 +226,6 @@ impl PyGetterDef {
             doc,
         }
     }
-
-    /// Copy descriptor information to `ffi::PyGetSetDef`
-    pub fn copy_to(&self, dst: &mut ffi::PyGetSetDef) {
-        if dst.name.is_null() {
-            let name = get_name(self.name).unwrap();
-            dst.name = name.as_ptr() as _;
-            // FIXME: stop leaking name
-            std::mem::forget(name);
-        }
-        if dst.doc.is_null() {
-            let doc = get_doc(self.doc).unwrap();
-            dst.doc = doc.as_ptr() as _;
-            // FIXME: stop leaking doc
-            std::mem::forget(doc);
-        }
-        dst.get = Some(self.meth.0);
-    }
 }
 
 impl PySetterDef {
@@ -249,31 +237,6 @@ impl PySetterDef {
             doc,
         }
     }
-
-    /// Copy descriptor information to `ffi::PyGetSetDef`
-    pub fn copy_to(&self, dst: &mut ffi::PyGetSetDef) {
-        if dst.name.is_null() {
-            let name = get_name(self.name).unwrap();
-            dst.name = name.as_ptr() as _;
-            // FIXME: stop leaking name
-            std::mem::forget(name);
-        }
-        if dst.doc.is_null() {
-            let doc = get_doc(self.doc).unwrap();
-            dst.doc = doc.as_ptr() as _;
-            // FIXME: stop leaking doc
-            std::mem::forget(doc);
-        }
-        dst.set = Some(self.meth.0);
-    }
-}
-
-fn get_name(name: &'static str) -> PyResult<Cow<'static, CStr>> {
-    extract_c_string(name, "Function name cannot contain NUL byte.")
-}
-
-fn get_doc(doc: &'static str) -> PyResult<Cow<'static, CStr>> {
-    extract_c_string(doc, "Document cannot contain NUL byte.")
 }
 
 /// Unwraps the result of __traverse__ for tp_traverse
@@ -319,3 +282,11 @@ where
         self.map(|o| o.into_py(py))
     }
 }
+
+pub(crate) fn get_name(name: &'static str) -> PyResult<Cow<'static, CStr>> {
+    extract_c_string(name, "function name cannot contain NUL byte.")
+}
+
+pub(crate) fn get_doc(doc: &'static str) -> PyResult<Cow<'static, CStr>> {
+    extract_c_string(doc, "function doc cannot contain NUL byte.")
+}

src/impl_/trampoline.rs

@@ -5,7 +5,7 @@
 
 use std::{
     any::Any,
-    os::raw::{c_int, c_void},
+    os::raw::c_int,
     panic::{self, UnwindSafe},
 };
 
@@ -64,29 +64,6 @@ trampolines!(
     ) -> *mut ffi::PyObject;
 );
 
-#[inline]
-pub unsafe fn getter(
-    slf: *mut ffi::PyObject,
-    closure: *mut c_void,
-    f: for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject) -> PyResult<*mut ffi::PyObject>,
-) -> *mut ffi::PyObject {
-    // PyO3 doesn't use the closure argument at present.
-    debug_assert!(closure.is_null());
-    trampoline_inner(|py| f(py, slf))
-}
-
-#[inline]
-pub unsafe fn setter(
-    slf: *mut ffi::PyObject,
-    value: *mut ffi::PyObject,
-    closure: *mut c_void,
-    f: for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject, *mut ffi::PyObject) -> PyResult<c_int>,
-) -> c_int {
-    // PyO3 doesn't use the closure argument at present.
-    debug_assert!(closure.is_null());
-    trampoline_inner(|py| f(py, slf, value))
-}
-
 // Trampolines used by slot methods
 trampolines!(
     pub fn getattrofunc(slf: *mut ffi::PyObject, attr: *mut ffi::PyObject) -> *mut ffi::PyObject;

src/pyclass.rs

@@ -8,7 +8,7 @@ use std::{cmp::Ordering, os::raw::c_int};
 mod create_type_object;
 mod gc;
 
-pub(crate) use self::create_type_object::create_type_object;
+pub(crate) use self::create_type_object::{create_type_object, PyClassTypeObject};
 pub use self::gc::{PyTraverseError, PyVisit};
 
 /// Types that can be used as Python classes.

src/pyclass/create_type_object.rs

@@ -5,10 +5,15 @@ use crate::{
         assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc,
         PyClassItemsIter,
     },
+    impl_::{
+        pymethods::{get_doc, get_name, Getter, Setter},
+        trampoline::trampoline_inner,
+    },
     types::PyType,
-    Py, PyClass, PyMethodDefType, PyResult, PyTypeInfo, Python,
+    Py, PyClass, PyGetterDef, PyMethodDefType, PyResult, PySetterDef, PyTypeInfo, Python,
 };
 use std::{
+    borrow::Cow,
     collections::HashMap,
     convert::TryInto,
     ffi::{CStr, CString},
@@ -16,7 +21,13 @@ use std::{
     ptr,
 };
 
-pub(crate) fn create_type_object<T>(py: Python<'_>) -> PyResult<Py<PyType>>
+pub(crate) struct PyClassTypeObject {
+    pub type_object: Py<PyType>,
+    #[allow(dead_code)] // This is purely a cache that must live as long as the type object
+    getset_destructors: Vec<GetSetDefDestructor>,
+}
+
+pub(crate) fn create_type_object<T>(py: Python<'_>) -> PyResult<PyClassTypeObject>
 where
     T: PyClass,
 {
@@ -40,7 +51,7 @@ type PyTypeBuilderCleanup = Box<dyn Fn(&PyTypeBuilder, *mut ffi::PyTypeObject)>;
 struct PyTypeBuilder {
     slots: Vec<ffi::PyType_Slot>,
     method_defs: Vec<ffi::PyMethodDef>,
-    property_defs_map: HashMap<&'static str, ffi::PyGetSetDef>,
+    getset_builders: HashMap<&'static str, GetSetDefBuilder>,
     /// Used to patch the type objects for the things there's no
     /// PyType_FromSpec API for... there's no reason this should work,
     /// except for that it does and we have tests.
@@ -111,28 +122,18 @@ impl PyTypeBuilder {
     }
 
     fn pymethod_def(&mut self, def: &PyMethodDefType) {
-        const PY_GET_SET_DEF_INIT: ffi::PyGetSetDef = ffi::PyGetSetDef {
-            name: ptr::null_mut(),
-            get: None,
-            set: None,
-            doc: ptr::null(),
-            closure: ptr::null_mut(),
-        };
-
         match def {
             PyMethodDefType::Getter(getter) => {
-                getter.copy_to(
-                    self.property_defs_map
+                self.getset_builders
                     .entry(getter.name)
-                        .or_insert(PY_GET_SET_DEF_INIT),
-                );
+                    .or_default()
+                    .add_getter(getter);
             }
             PyMethodDefType::Setter(setter) => {
-                setter.copy_to(
-                    self.property_defs_map
+                self.getset_builders
                     .entry(setter.name)
-                        .or_insert(PY_GET_SET_DEF_INIT),
-                );
+                    .or_default()
+                    .add_setter(setter);
             }
             PyMethodDefType::Method(def)
             | PyMethodDefType::Class(def)
@@ -147,22 +148,30 @@ impl PyTypeBuilder {
         }
     }
 
-    fn finalize_methods_and_properties(&mut self) {
-        let method_defs = std::mem::take(&mut self.method_defs);
+    fn finalize_methods_and_properties(&mut self) -> PyResult<Vec<GetSetDefDestructor>> {
+        let method_defs: Vec<pyo3_ffi::PyMethodDef> = std::mem::take(&mut self.method_defs);
         // Safety: Py_tp_methods expects a raw vec of PyMethodDef
         unsafe { self.push_raw_vec_slot(ffi::Py_tp_methods, method_defs) };
 
-        let property_defs = std::mem::take(&mut self.property_defs_map);
-        // TODO: use into_values when on MSRV Rust >= 1.54
+        let mut getset_destructors = Vec::with_capacity(self.getset_builders.len());
+
         #[allow(unused_mut)]
-        let mut property_defs: Vec<_> = property_defs.into_iter().map(|(_, value)| value).collect();
+        let mut property_defs: Vec<_> = self
+            .getset_builders
+            .iter()
+            .map(|(name, builder)| {
+                let (def, destructor) = builder.as_get_set_def(name)?;
+                getset_destructors.push(destructor);
+                Ok(def)
+            })
+            .collect::<PyResult<_>>()?;
 
         // PyPy doesn't automatically add __dict__ getter / setter.
         // PyObject_GenericGetDict not in the limited API until Python 3.10.
         if self.has_dict {
             #[cfg(not(any(PyPy, all(Py_LIMITED_API, not(Py_3_10)))))]
             property_defs.push(ffi::PyGetSetDef {
-                name: "__dict__\0".as_ptr() as *mut c_char,
+                name: "__dict__\0".as_ptr().cast(),
                 get: Some(ffi::PyObject_GenericGetDict),
                 set: Some(ffi::PyObject_GenericSetDict),
                 doc: ptr::null(),
@@ -200,6 +209,8 @@ impl PyTypeBuilder {
                 )
             }
         }
+
+        Ok(getset_destructors)
     }
 
     fn set_is_basetype(mut self, is_basetype: bool) -> Self {
@@ -324,12 +335,12 @@ impl PyTypeBuilder {
         name: &'static str,
         module_name: Option<&'static str>,
         basicsize: usize,
-    ) -> PyResult<Py<PyType>> {
+    ) -> PyResult<PyClassTypeObject> {
         // `c_ulong` and `c_uint` have the same size
         // on some platforms (like windows)
         #![allow(clippy::useless_conversion)]
 
-        self.finalize_methods_and_properties();
+        let getset_destructors = self.finalize_methods_and_properties()?;
 
         if !self.has_new {
             // Safety: This is the correct slot type for Py_tp_new
@@ -380,7 +391,10 @@ impl PyTypeBuilder {
             cleanup(&self, type_object.as_ref(py).as_type_ptr());
         }
 
-        Ok(type_object)
+        Ok(PyClassTypeObject {
+            type_object,
+            getset_destructors,
+        })
     }
 }
 
@@ -399,9 +413,152 @@ unsafe extern "C" fn no_constructor_defined(
     _args: *mut ffi::PyObject,
     _kwds: *mut ffi::PyObject,
 ) -> *mut ffi::PyObject {
-    crate::impl_::trampoline::trampoline_inner(|_| {
+    trampoline_inner(|_| {
         Err(crate::exceptions::PyTypeError::new_err(
             "No constructor defined",
         ))
     })
 }
+
+#[derive(Default)]
+struct GetSetDefBuilder {
+    doc: Option<&'static str>,
+    getter: Option<Getter>,
+    setter: Option<Setter>,
+}
+
+impl GetSetDefBuilder {
+    fn add_getter(&mut self, getter: &PyGetterDef) {
+        // TODO: be smarter about merging getter and setter docs
+        if self.doc.is_none() {
+            self.doc = Some(getter.doc);
+        }
+        // TODO: return an error if getter already defined?
+        self.getter = Some(getter.meth.0)
+    }
+
+    fn add_setter(&mut self, setter: &PySetterDef) {
+        // TODO: be smarter about merging getter and setter docs
+        if self.doc.is_none() {
+            self.doc = Some(setter.doc);
+        }
+        // TODO: return an error if setter already defined?
+        self.setter = Some(setter.meth.0)
+    }
+
+    fn as_get_set_def(
+        &self,
+        name: &'static str,
+    ) -> PyResult<(ffi::PyGetSetDef, GetSetDefDestructor)> {
+        let name = get_name(name)?;
+        let doc = self.doc.map(get_doc).transpose()?;
+
+        let getset_type = match (self.getter, self.setter) {
+            (Some(getter), None) => GetSetDefType::Getter(getter),
+            (None, Some(setter)) => GetSetDefType::Setter(setter),
+            (Some(getter), Some(setter)) => {
+                GetSetDefType::GetterAndSetter(Box::new(GetterAndSetter { getter, setter }))
+            }
+            (None, None) => {
+                unreachable!("GetSetDefBuilder expected to always have either getter or setter")
+            }
+        };
+
+        let getset_def = getset_type.create_py_get_set_def(&name, doc.as_deref());
+        let destructor = GetSetDefDestructor {
+            name,
+            doc,
+            closure: getset_type,
+        };
+        Ok((getset_def, destructor))
+    }
+}
+
+#[allow(dead_code)] // a stack of fields which are purely to cache until dropped
+struct GetSetDefDestructor {
+    name: Cow<'static, CStr>,
+    doc: Option<Cow<'static, CStr>>,
+    closure: GetSetDefType,
+}
+
+/// Possible forms of property - either a getter, setter, or both
+enum GetSetDefType {
+    Getter(Getter),
+    Setter(Setter),
+    // The box is here so that the `GetterAndSetter` has a stable
+    // memory address even if the `GetSetDefType` enum is moved
+    GetterAndSetter(Box<GetterAndSetter>),
+}
+
+pub(crate) struct GetterAndSetter {
+    getter: Getter,
+    setter: Setter,
+}
+
+impl GetSetDefType {
+    /// Fills a PyGetSetDef structure
+    /// It is only valid for as long as this GetSetDefType remains alive,
+    /// as well as name and doc members
+    pub(crate) fn create_py_get_set_def(
+        &self,
+        name: &CStr,
+        doc: Option<&CStr>,
+    ) -> ffi::PyGetSetDef {
+        let (get, set, closure): (Option<ffi::getter>, Option<ffi::setter>, *mut c_void) =
+            match self {
+                &Self::Getter(closure) => {
+                    unsafe extern "C" fn getter(
+                        slf: *mut ffi::PyObject,
+                        closure: *mut c_void,
+                    ) -> *mut ffi::PyObject {
+                        // Safety: PyO3 sets the closure when constructing the ffi getter so this cast should always be valid
+                        let getter: Getter = std::mem::transmute(closure);
+                        trampoline_inner(|py| getter(py, slf))
+                    }
+                    (Some(getter), None, closure as Getter as _)
+                }
+                &Self::Setter(closure) => {
+                    unsafe extern "C" fn setter(
+                        slf: *mut ffi::PyObject,
+                        value: *mut ffi::PyObject,
+                        closure: *mut c_void,
+                    ) -> c_int {
+                        // Safety: PyO3 sets the closure when constructing the ffi setter so this cast should always be valid
+                        let setter: Setter = std::mem::transmute(closure);
+                        trampoline_inner(|py| setter(py, slf, value))
+                    }
+                    (None, Some(setter), closure as Setter as _)
+                }
+                Self::GetterAndSetter(closure) => {
+                    unsafe extern "C" fn getset_getter(
+                        slf: *mut ffi::PyObject,
+                        closure: *mut c_void,
+                    ) -> *mut ffi::PyObject {
+                        let getset: &GetterAndSetter = &*(closure as *const GetterAndSetter);
+                        trampoline_inner(|py| (getset.getter)(py, slf))
+                    }
+
+                    unsafe extern "C" fn getset_setter(
+                        slf: *mut ffi::PyObject,
+                        value: *mut ffi::PyObject,
+                        closure: *mut c_void,
+                    ) -> c_int {
+                        let getset: &GetterAndSetter = &*(closure as *const GetterAndSetter);
+                        trampoline_inner(|py| (getset.setter)(py, slf, value))
+                    }
+                    (
+                        Some(getset_getter),
+                        Some(getset_setter),
+                        closure.as_ref() as *const GetterAndSetter as _,
+                    )
+                }
+            };
+        ffi::PyGetSetDef {
+            name: name.as_ptr(),
+            doc: doc.map_or(ptr::null(), CStr::as_ptr),
+            get,
+            set,
+            closure,
+        }
+    }
+}