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.