From 0b3cf7a55eaeec4102bfb6d9a96cd052026865c3 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Mon, 17 May 2021 00:31:05 +0100 Subject: [PATCH] pyclass: refactor initialization, call native type dealloc --- CHANGELOG.md | 2 + guide/src/class.md | 12 ++- pyo3-macros-backend/src/pyclass.rs | 42 +++++--- src/class/impl_.rs | 135 ++++++++++++++++++++++++- src/ffi/datetime.rs | 1 - src/freelist.rs | 115 --------------------- src/impl_.rs | 1 + src/impl_/freelist.rs | 66 ++++++++++++ src/lib.rs | 1 - src/pycell.rs | 157 ++++++++++++----------------- src/pyclass.rs | 155 ++++------------------------ src/pyclass_init.rs | 115 ++++++++++++++++++--- src/pyclass_slots.rs | 8 ++ src/type_object.rs | 38 +++++-- tests/test_inheritance.rs | 17 ++++ 15 files changed, 485 insertions(+), 380 deletions(-) delete mode 100644 src/freelist.rs create mode 100644 src/impl_/freelist.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index f871c26e603..3a755a943aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Remove `__doc__` from module's `__all__`. [#1509](https://github.com/PyO3/pyo3/pull/1509) - Remove `PYO3_CROSS_INCLUDE_DIR` environment variable and the associated C header parsing functionality. [#1521](https://github.com/PyO3/pyo3/pull/1521) - Remove `raw_pycfunction!` macro. [#1619](https://github.com/PyO3/pyo3/pull/1619) +- Remove `PyClassAlloc` trait. [#1657](https://github.com/PyO3/pyo3/pull/1657) ### Fixed - Remove FFI definition `PyCFunction_ClearFreeList` for Python 3.9 and later. [#1425](https://github.com/PyO3/pyo3/pull/1425) @@ -80,6 +81,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Fix unneccessary rebuilds when cycling between `cargo check` and `cargo clippy` in a Python virtualenv. [#1557](https://github.com/PyO3/pyo3/pull/1557) - Fix segfault when dereferencing `ffi::PyDateTimeAPI` without the GIL. [#1563](https://github.com/PyO3/pyo3/pull/1563) - Fix memory leak when converting to u128 and i128. [#1638](https://github.com/PyO3/pyo3/pull/1638) +- Fix `#[pyclass(extends=PyDict)]` leaking the dict contents on drop. [#1657](https://github.com/PyO3/pyo3/pull/1657) ## [0.13.2] - 2021-02-12 ### Packaging diff --git a/guide/src/class.md b/guide/src/class.md index 269db256d3d..c0bc650a4d3 100644 --- a/guide/src/class.md +++ b/guide/src/class.md @@ -720,8 +720,6 @@ struct MyClass { debug: bool, } -impl pyo3::pyclass::PyClassAlloc for MyClass {} - unsafe impl pyo3::PyTypeInfo for MyClass { type AsRefTarget = PyCell; @@ -774,6 +772,16 @@ impl pyo3::class::impl_::PyClassImpl for MyClass { let collector = PyClassImplCollector::::new(); collector.new_impl() } + fn get_alloc() -> Option { + use pyo3::class::impl_::*; + let collector = PyClassImplCollector::::new(); + collector.alloc_impl() + } + fn get_free() -> Option { + use pyo3::class::impl_::*; + let collector = PyClassImplCollector::::new(); + collector.free_impl() + } fn get_call() -> Option { use pyo3::class::impl_::*; let collector = PyClassImplCollector::::new(); diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index e6a979eb881..074d5571369 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -311,29 +311,37 @@ fn impl_class( ) -> syn::Result { let cls_name = get_class_python_name(cls, attr).to_string(); - let alloc = { - if let Some(freelist) = &attr.freelist { + let alloc = attr.freelist.as_ref().map(|freelist| { quote! { - impl pyo3::freelist::PyClassWithFreeList for #cls { + impl pyo3::class::impl_::PyClassWithFreeList for #cls { #[inline] - fn get_free_list(_py: pyo3::Python) -> &mut pyo3::freelist::FreeList<*mut pyo3::ffi::PyObject> { - static mut FREELIST: *mut pyo3::freelist::FreeList<*mut pyo3::ffi::PyObject> = 0 as *mut _; + fn get_free_list(_py: pyo3::Python) -> &mut pyo3::impl_::freelist::FreeList<*mut pyo3::ffi::PyObject> { + static mut FREELIST: *mut pyo3::impl_::freelist::FreeList<*mut pyo3::ffi::PyObject> = 0 as *mut _; unsafe { if FREELIST.is_null() { FREELIST = Box::into_raw(Box::new( - pyo3::freelist::FreeList::with_capacity(#freelist))); + pyo3::impl_::freelist::FreeList::with_capacity(#freelist))); } &mut *FREELIST } } } + + impl pyo3::class::impl_::PyClassAllocImpl<#cls> for pyo3::class::impl_::PyClassImplCollector<#cls> { + #[inline] + fn alloc_impl(self) -> Option { + Some(pyo3::class::impl_::alloc_with_freelist::<#cls>) + } + } + + impl pyo3::class::impl_::PyClassFreeImpl<#cls> for pyo3::class::impl_::PyClassImplCollector<#cls> { + #[inline] + fn free_impl(self) -> Option { + Some(pyo3::class::impl_::free_with_freelist::<#cls>) + } + } } - } else { - quote! { - impl pyo3::pyclass::PyClassAlloc for #cls {} - } - } - }; + }); let descriptors = impl_descriptors(cls, field_options)?; @@ -482,6 +490,16 @@ fn impl_class( let collector = PyClassImplCollector::::new(); collector.new_impl() } + fn get_alloc() -> Option { + use pyo3::class::impl_::*; + let collector = PyClassImplCollector::::new(); + collector.alloc_impl() + } + fn get_free() -> Option { + use pyo3::class::impl_::*; + let collector = PyClassImplCollector::::new(); + collector.free_impl() + } fn get_call() -> Option { use pyo3::class::impl_::*; let collector = PyClassImplCollector::::new(); diff --git a/src/class/impl_.rs b/src/class/impl_.rs index c5552123e5e..9b747c96aa9 100644 --- a/src/class/impl_.rs +++ b/src/class/impl_.rs @@ -2,12 +2,13 @@ use crate::{ ffi, + impl_::freelist::FreeList, pycell::PyCellLayout, pyclass_init::PyObjectInit, type_object::{PyLayout, PyTypeObject}, - PyClass, PyMethodDefType, PyNativeType, PyTypeInfo, + PyCell, PyClass, PyMethodDefType, PyNativeType, PyTypeInfo, Python, }; -use std::{marker::PhantomData, thread}; +use std::{marker::PhantomData, os::raw::c_void, thread}; /// This type is used as a "dummy" type on which dtolnay specializations are /// applied to apply implementations from `#[pymethods]` & `#[pyproto]` @@ -72,6 +73,12 @@ pub trait PyClassImpl: Sized { fn get_call() -> Option { None } + fn get_alloc() -> Option { + None + } + fn get_free() -> Option { + None + } fn for_each_proto_slot(_visitor: &mut dyn FnMut(&[ffi::PyType_Slot])) {} fn get_buffer() -> Option<&'static PyBufferProcs> { None @@ -100,6 +107,104 @@ impl PyClassCallImpl for &'_ PyClassImplCollector { } } +pub trait PyClassAllocImpl { + fn alloc_impl(self) -> Option; +} + +impl PyClassAllocImpl for &'_ PyClassImplCollector { + fn alloc_impl(self) -> Option { + None + } +} + +pub trait PyClassFreeImpl { + fn free_impl(self) -> Option; +} + +impl PyClassFreeImpl for &'_ PyClassImplCollector { + fn free_impl(self) -> Option { + None + } +} + +/// Implements a freelist. +/// +/// Do not implement this trait manually. Instead, use `#[pyclass(freelist = N)]` +/// on a Rust struct to implement it. +pub trait PyClassWithFreeList: PyClass { + fn get_free_list(py: Python) -> &mut FreeList<*mut ffi::PyObject>; +} + +/// Implementation of tp_alloc for `freelist` classes. +pub unsafe extern "C" fn alloc_with_freelist( + subtype: *mut ffi::PyTypeObject, + nitems: ffi::Py_ssize_t, +) -> *mut ffi::PyObject { + let py = Python::assume_gil_acquired(); + + #[cfg(not(Py_3_8))] + bpo_35810_workaround(py, subtype); + + let self_type = T::type_object_raw(py); + // If this type is a variable type or the subtype is not equal to this type, we cannot use the + // freelist + if nitems == 0 && subtype == self_type { + if let Some(obj) = T::get_free_list(py).pop() { + ffi::PyObject_Init(obj, subtype); + return obj as _; + } + } + + ffi::PyType_GenericAlloc(subtype, nitems) +} + +/// Implementation of tp_free for `freelist` classes. +#[allow(clippy::collapsible_if)] // for if cfg! +pub unsafe extern "C" fn free_with_freelist(obj: *mut c_void) { + let obj = obj as *mut ffi::PyObject; + debug_assert_eq!( + T::type_object_raw(Python::assume_gil_acquired()), + ffi::Py_TYPE(obj) + ); + if let Some(obj) = T::get_free_list(Python::assume_gil_acquired()).insert(obj) { + let ty = ffi::Py_TYPE(obj); + + // Deduce appropriate inverse of PyType_GenericAlloc + let free = if ffi::PyType_IS_GC(ty) != 0 { + ffi::PyObject_GC_Del + } else { + ffi::PyObject_Free + }; + free(obj as *mut c_void); + + if cfg!(Py_3_8) { + if ffi::PyType_HasFeature(ty, ffi::Py_TPFLAGS_HEAPTYPE) != 0 { + ffi::Py_DECREF(ty as *mut ffi::PyObject); + } + } + } +} + +/// Workaround for Python issue 35810; no longer necessary in Python 3.8 +#[inline] +#[cfg(not(Py_3_8))] +unsafe fn bpo_35810_workaround(_py: Python, ty: *mut ffi::PyTypeObject) { + #[cfg(Py_LIMITED_API)] + { + // Must check version at runtime for abi3 wheels - they could run against a higher version + // than the build config suggests. + use crate::once_cell::GILOnceCell; + static IS_PYTHON_3_8: GILOnceCell = GILOnceCell::new(); + + if *IS_PYTHON_3_8.get_or_init(_py, || _py.version_info() >= (3, 8)) { + // No fix needed - the wheel is running on a sufficiently new interpreter. + return; + } + } + + ffi::Py_INCREF(ty as *mut ffi::PyObject); +} + // General methods implementation: either dtolnay specialization trait or inventory if // multiple-pymethods feature is enabled. @@ -216,6 +321,7 @@ pub struct ThreadCheckerStub(PhantomData); impl PyClassThreadChecker for ThreadCheckerStub { fn ensure(&self) {} + #[inline] fn new() -> Self { ThreadCheckerStub(PhantomData) } @@ -224,6 +330,7 @@ impl PyClassThreadChecker for ThreadCheckerStub { impl PyClassThreadChecker for ThreadCheckerStub { fn ensure(&self) {} + #[inline] fn new() -> Self { ThreadCheckerStub(PhantomData) } @@ -279,8 +386,30 @@ pub trait PyClassBaseType: Sized { impl PyClassBaseType for T { type Dict = T::Dict; type WeakRef = T::WeakRef; - type LayoutAsBase = crate::pycell::PyCellInner; + type LayoutAsBase = crate::pycell::PyCell; type BaseNativeType = T::BaseNativeType; type ThreadChecker = T::ThreadChecker; type Initializer = crate::pyclass_init::PyClassInitializer; } + +// Default new implementation + +pub(crate) unsafe extern "C" fn fallback_new( + _subtype: *mut ffi::PyTypeObject, + _args: *mut ffi::PyObject, + _kwds: *mut ffi::PyObject, +) -> *mut ffi::PyObject { + crate::callback_body!(py, { + Err::<(), _>(crate::exceptions::PyTypeError::new_err( + "No constructor defined", + )) + }) +} + +pub(crate) unsafe extern "C" fn tp_dealloc(obj: *mut ffi::PyObject) { + crate::callback_body!(py, { + // Safety: Python will only call tp_dealloc when no references to the object remain. + let cell: &mut PyCell = &mut *(obj as *mut _); + cell.tp_dealloc(py); + }) +} diff --git a/src/ffi/datetime.rs b/src/ffi/datetime.rs index 5f4915df4b5..749286d82d2 100644 --- a/src/ffi/datetime.rs +++ b/src/ffi/datetime.rs @@ -650,7 +650,6 @@ mod tests { fn test_date_fromtimestamp() { Python::with_gil(|py| { let args: Py = (100,).into_py(py); - dbg!(args.as_ref(py)); unsafe { PyDateTime_IMPORT() }; let dt: &PyAny = unsafe { py.from_owned_ptr(PyDate_FromTimestamp(args.as_ptr())) }; py_run!( diff --git a/src/freelist.rs b/src/freelist.rs deleted file mode 100644 index b728a87219d..00000000000 --- a/src/freelist.rs +++ /dev/null @@ -1,115 +0,0 @@ -// Copyright (c) 2017-present PyO3 Project and Contributors - -//! Support for [free allocation lists][1]. -//! -//! This can improve performance for types that are often created and deleted in quick succession. -//! -//! Rather than implementing this manually, -//! implement it by annotating a struct with `#[pyclass(freelist = N)]`, -//! where `N` is the size of the freelist. -//! -//! [1]: https://en.wikipedia.org/wiki/Free_list - -use crate::class::impl_::PyClassImpl; -use crate::pyclass::{get_type_free, tp_free_fallback, PyClassAlloc}; -use crate::type_object::{PyLayout, PyTypeInfo}; -use crate::{ffi, AsPyPointer, FromPyPointer, PyAny, Python}; -use std::mem; -use std::os::raw::c_void; - -/// Implements a freelist. -/// -/// Do not implement this trait manually. Instead, use `#[pyclass(freelist = N)]` -/// on a Rust struct to implement it. -pub trait PyClassWithFreeList { - fn get_free_list(py: Python) -> &mut FreeList<*mut ffi::PyObject>; -} - -/// Represents a slot of a [`FreeList`]. -pub enum Slot { - Empty, - Filled(T), -} - -pub struct FreeList { - entries: Vec>, - split: usize, - capacity: usize, -} - -impl FreeList { - /// Creates a new `FreeList` instance with specified capacity. - pub fn with_capacity(capacity: usize) -> FreeList { - let entries = (0..capacity).map(|_| Slot::Empty).collect::>(); - - FreeList { - entries, - split: 0, - capacity, - } - } - - /// Pops the first non empty item. - pub fn pop(&mut self) -> Option { - let idx = self.split; - if idx == 0 { - None - } else { - match mem::replace(&mut self.entries[idx - 1], Slot::Empty) { - Slot::Filled(v) => { - self.split = idx - 1; - Some(v) - } - _ => panic!("FreeList is corrupt"), - } - } - } - - /// Inserts a value into the list. Returns `None` if the `FreeList` is full. - pub fn insert(&mut self, val: T) -> Option { - let next = self.split + 1; - if next < self.capacity { - self.entries[self.split] = Slot::Filled(val); - self.split = next; - None - } else { - Some(val) - } - } -} - -impl PyClassAlloc for T -where - T: PyTypeInfo + PyClassImpl + PyClassWithFreeList, -{ - unsafe fn new(py: Python, subtype: *mut ffi::PyTypeObject) -> *mut Self::Layout { - // if subtype is not equal to this type, we cannot use the freelist - if subtype == Self::type_object_raw(py) { - if let Some(obj) = ::get_free_list(py).pop() { - ffi::PyObject_Init(obj, subtype); - #[cfg(not(Py_3_8))] - crate::pyclass::bpo_35810_workaround(py, subtype); - return obj as _; - } - } - crate::pyclass::default_new::(py, subtype) as _ - } - - #[allow(clippy::collapsible_if)] // for if cfg! - unsafe fn dealloc(py: Python, self_: *mut Self::Layout) { - (*self_).py_drop(py); - let obj = PyAny::from_borrowed_ptr_or_panic(py, self_ as _); - - if let Some(obj) = ::get_free_list(py).insert(obj.as_ptr()) { - let ty = ffi::Py_TYPE(obj); - let free = get_type_free(ty).unwrap_or_else(|| tp_free_fallback(ty)); - free(obj as *mut c_void); - - if cfg!(Py_3_8) { - if ffi::PyType_HasFeature(ty, ffi::Py_TPFLAGS_HEAPTYPE) != 0 { - ffi::Py_DECREF(ty as *mut ffi::PyObject); - } - } - } - } -} diff --git a/src/impl_.rs b/src/impl_.rs index f8ef67919e9..a4bd78ff5fb 100644 --- a/src/impl_.rs +++ b/src/impl_.rs @@ -3,3 +3,4 @@ //! any time without documentation in the CHANGELOG and without breaking semver guarantees. pub mod deprecations; +pub mod freelist; diff --git a/src/impl_/freelist.rs b/src/impl_/freelist.rs new file mode 100644 index 00000000000..a8d926c3247 --- /dev/null +++ b/src/impl_/freelist.rs @@ -0,0 +1,66 @@ +// Copyright (c) 2017-present PyO3 Project and Contributors + +//! Support for [free allocation lists][1]. +//! +//! This can improve performance for types that are often created and deleted in quick succession. +//! +//! Rather than implementing this manually, +//! implement it by annotating a struct with `#[pyclass(freelist = N)]`, +//! where `N` is the size of the freelist. +//! +//! [1]: https://en.wikipedia.org/wiki/Free_list + +use std::mem; + +/// Represents a slot of a [`FreeList`]. +pub enum Slot { + Empty, + Filled(T), +} + +pub struct FreeList { + entries: Vec>, + split: usize, + capacity: usize, +} + +impl FreeList { + /// Creates a new `FreeList` instance with specified capacity. + pub fn with_capacity(capacity: usize) -> FreeList { + let entries = (0..capacity).map(|_| Slot::Empty).collect::>(); + + FreeList { + entries, + split: 0, + capacity, + } + } + + /// Pops the first non empty item. + pub fn pop(&mut self) -> Option { + let idx = self.split; + if idx == 0 { + None + } else { + match mem::replace(&mut self.entries[idx - 1], Slot::Empty) { + Slot::Filled(v) => { + self.split = idx - 1; + Some(v) + } + _ => panic!("FreeList is corrupt"), + } + } + } + + /// Inserts a value into the list. Returns `None` if the `FreeList` is full. + pub fn insert(&mut self, val: T) -> Option { + let next = self.split + 1; + if next < self.capacity { + self.entries[self.split] = Slot::Filled(val); + self.split = next; + None + } else { + Some(val) + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 1bde92c3318..a9373f6b405 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -277,7 +277,6 @@ pub mod derive_utils; mod err; pub mod exceptions; pub mod ffi; -pub mod freelist; mod gil; pub mod impl_; mod instance; diff --git a/src/pycell.rs b/src/pycell.rs index 9a5c40e5cbc..398cc9e5794 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -1,5 +1,4 @@ //! Includes `PyCell` implementation. -use crate::conversion::{AsPyPointer, FromPyPointer, ToPyObject}; use crate::exceptions::PyRuntimeError; use crate::pyclass::PyClass; use crate::pyclass_init::PyClassInitializer; @@ -7,6 +6,12 @@ use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; use crate::type_object::{PyLayout, PySizedLayout}; use crate::types::PyAny; use crate::{class::impl_::PyClassBaseType, class::impl_::PyClassThreadChecker}; +use crate::{ + conversion::{AsPyPointer, FromPyPointer, ToPyObject}, + ffi::PyBaseObject_Type, + type_object::get_tp_free, + PyTypeInfo, +}; use crate::{ffi, IntoPy, PyErr, PyNativeType, PyObject, PyResult, Python}; use std::cell::{Cell, UnsafeCell}; use std::fmt; @@ -22,49 +27,7 @@ pub struct PyCellBase { borrow_flag: Cell, } -unsafe impl PyLayout for PyCellBase -where - U: PySizedLayout, -{ - const IS_NATIVE_TYPE: bool = true; -} - -/// Inner type of `PyCell` without dict slots and reference counter. -/// This struct has two usages: -/// 1. As an inner type of `PyRef` and `PyRefMut`. -/// 2. When `#[pyclass(extends=Base)]` is specified, `PyCellInner` is used as a base layout. -#[doc(hidden)] -#[repr(C)] -pub struct PyCellInner { - ob_base: ::LayoutAsBase, - value: ManuallyDrop>, -} - -impl AsPyPointer for PyCellInner { - fn as_ptr(&self) -> *mut ffi::PyObject { - (self as *const _) as *mut _ - } -} - -unsafe impl PyLayout for PyCellInner { - const IS_NATIVE_TYPE: bool = false; - fn py_init(&mut self, value: T) { - self.value = ManuallyDrop::new(UnsafeCell::new(value)); - } - unsafe fn py_drop(&mut self, py: Python) { - ManuallyDrop::drop(&mut self.value); - self.ob_base.py_drop(py); - } -} - -// These impls ensures `PyCellInner` can be a base type. -impl PySizedLayout for PyCellInner {} - -impl PyCellInner { - fn get_ptr(&self) -> *mut T { - self.value.get() - } -} +unsafe impl PyLayout for PyCellBase where U: PySizedLayout {} /// `PyCell` is the container type for [`PyClass`](../pyclass/trait.PyClass.html). /// @@ -133,12 +96,18 @@ impl PyCellInner { /// ``` #[repr(C)] pub struct PyCell { - inner: PyCellInner, - thread_checker: T::ThreadChecker, + ob_base: ::LayoutAsBase, + contents: PyCellContents, +} + +#[repr(C)] +pub(crate) struct PyCellContents { + pub(crate) value: ManuallyDrop>, + pub(crate) thread_checker: T::ThreadChecker, // DO NOT CHANGE THE ORDER OF THESE FIELDS WITHOUT CHANGING PyCell::dict_offset() // AND PyCell::weakref_offset() - dict: T::Dict, - weakref: T::WeakRef, + pub(crate) dict: T::Dict, + pub(crate) weakref: T::WeakRef, } impl PyCell { @@ -228,13 +197,12 @@ impl PyCell { /// }); /// ``` pub fn try_borrow(&self) -> Result, PyBorrowError> { - self.thread_checker.ensure(); - let flag = self.inner.get_borrow_flag(); + let flag = self.get_borrow_flag(); if flag == BorrowFlag::HAS_MUTABLE_BORROW { Err(PyBorrowError { _private: () }) } else { - self.inner.set_borrow_flag(flag.increment()); - Ok(PyRef { inner: &self.inner }) + self.set_borrow_flag(flag.increment()); + Ok(PyRef { inner: self }) } } @@ -260,12 +228,11 @@ impl PyCell { /// }); /// ``` pub fn try_borrow_mut(&self) -> Result, PyBorrowMutError> { - self.thread_checker.ensure(); - if self.inner.get_borrow_flag() != BorrowFlag::UNUSED { + if self.get_borrow_flag() != BorrowFlag::UNUSED { Err(PyBorrowMutError { _private: () }) } else { - self.inner.set_borrow_flag(BorrowFlag::HAS_MUTABLE_BORROW); - Ok(PyRefMut { inner: &self.inner }) + self.set_borrow_flag(BorrowFlag::HAS_MUTABLE_BORROW); + Ok(PyRefMut { inner: self }) } } @@ -299,11 +266,10 @@ impl PyCell { /// }); /// ``` pub unsafe fn try_borrow_unguarded(&self) -> Result<&T, PyBorrowError> { - self.thread_checker.ensure(); - if self.inner.get_borrow_flag() == BorrowFlag::HAS_MUTABLE_BORROW { + if self.get_borrow_flag() == BorrowFlag::HAS_MUTABLE_BORROW { Err(PyBorrowError { _private: () }) } else { - Ok(&*self.inner.value.get()) + Ok(&*self.contents.value.get()) } } @@ -338,41 +304,17 @@ impl PyCell { std::mem::swap(&mut *self.borrow_mut(), &mut *other.borrow_mut()) } - /// Allocates a new PyCell given a type object `subtype`. Used by our `tp_new` implementation. - pub(crate) unsafe fn internal_new( - py: Python, - subtype: *mut ffi::PyTypeObject, - ) -> PyResult<*mut Self> { - let base = T::new(py, subtype); - if base.is_null() { - return Err(PyErr::fetch(py)); - } - let base = base as *mut PyCellBase; - (*base).borrow_flag = Cell::new(BorrowFlag::UNUSED); - let self_ = base as *mut Self; - (*self_).dict = T::Dict::new(); - (*self_).weakref = T::WeakRef::new(); - (*self_).thread_checker = T::ThreadChecker::new(); - Ok(self_) + fn get_ptr(&self) -> *mut T { + self.contents.value.get() } } -unsafe impl PyLayout for PyCell { - const IS_NATIVE_TYPE: bool = false; - fn py_init(&mut self, value: T) { - self.inner.value = ManuallyDrop::new(UnsafeCell::new(value)); - } - unsafe fn py_drop(&mut self, py: Python) { - ManuallyDrop::drop(&mut self.inner.value); - self.dict.clear_dict(py); - self.weakref.clear_weakrefs(self.as_ptr(), py); - self.inner.ob_base.py_drop(py); - } -} +unsafe impl PyLayout for PyCell {} +impl PySizedLayout for PyCell {} impl AsPyPointer for PyCell { fn as_ptr(&self) -> *mut ffi::PyObject { - self.inner.as_ptr() + (self as *const _) as *mut _ } } @@ -453,7 +395,7 @@ impl fmt::Debug for PyCell { /// # }); /// ``` pub struct PyRef<'p, T: PyClass> { - inner: &'p PyCellInner, + inner: &'p PyCell, } impl<'p, T: PyClass> PyRef<'p, T> { @@ -570,7 +512,7 @@ impl fmt::Debug for PyRef<'_, T> { /// /// See the [`PyCell`](struct.PyCell.html) and [`PyRef`](struct.PyRef.html) documentations for more. pub struct PyRefMut<'p, T: PyClass> { - inner: &'p PyCellInner, + inner: &'p PyCell, } impl<'p, T: PyClass> PyRefMut<'p, T> { @@ -669,7 +611,7 @@ impl fmt::Debug for PyRefMut<'_, T> { pub struct BorrowFlag(usize); impl BorrowFlag { - const UNUSED: BorrowFlag = BorrowFlag(0); + pub(crate) const UNUSED: BorrowFlag = BorrowFlag(0); const HAS_MUTABLE_BORROW: BorrowFlag = BorrowFlag(usize::max_value()); const fn increment(self) -> Self { Self(self.0 + 1) @@ -733,11 +675,14 @@ impl From for PyErr { pub trait PyCellLayout: PyLayout { fn get_borrow_flag(&self) -> BorrowFlag; fn set_borrow_flag(&self, flag: BorrowFlag); + /// Implementation of tp_dealloc. Do not attempt to use &self after calling this! + unsafe fn tp_dealloc(&mut self, py: Python); } impl PyCellLayout for PyCellBase where U: PySizedLayout, + T: PyTypeInfo, { fn get_borrow_flag(&self) -> BorrowFlag { self.borrow_flag.get() @@ -745,16 +690,44 @@ where fn set_borrow_flag(&self, flag: BorrowFlag) { self.borrow_flag.set(flag) } + unsafe fn tp_dealloc(&mut self, py: Python) { + let obj: *mut ffi::PyObject = self as *mut _ as *mut _; + + // For `#[pyclass]` types which inherit from PyAny, we can just call tp_free + if T::type_object_raw(py) == &mut PyBaseObject_Type { + return get_tp_free(ffi::Py_TYPE(obj))(obj as _); + } + + // More complex native types (e.g. `extends=PyDict`) require calling the base's dealloc. + #[cfg(not(Py_LIMITED_API))] + { + if let Some(dealloc) = (*T::type_object_raw(py)).tp_dealloc { + dealloc(obj as _); + } else { + get_tp_free(ffi::Py_TYPE(obj))(obj as _); + } + } + + #[cfg(Py_LIMITED_API)] + unreachable!("subclassing native types is not possible with the `abi3` feature"); + } } -impl PyCellLayout for PyCellInner +impl PyCellLayout for PyCell where ::LayoutAsBase: PyCellLayout, { fn get_borrow_flag(&self) -> BorrowFlag { + self.contents.thread_checker.ensure(); self.ob_base.get_borrow_flag() } fn set_borrow_flag(&self, flag: BorrowFlag) { self.ob_base.set_borrow_flag(flag) } + unsafe fn tp_dealloc(&mut self, py: Python) { + ManuallyDrop::drop(&mut self.contents.value); + self.contents.dict.clear_dict(py); + self.contents.weakref.clear_weakrefs(self.as_ptr(), py); + self.ob_base.tp_dealloc(py); + } } diff --git a/src/pyclass.rs b/src/pyclass.rs index b988e8a51e1..c09092727f9 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -1,135 +1,16 @@ //! `PyClass` and related traits. -use crate::class::methods::PyMethodDefType; -use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; -use crate::type_object::PyLayout; -use crate::{class::impl_::PyClassBaseType, class::impl_::PyClassImpl}; -use crate::{ffi, PyCell, PyErr, PyNativeType, PyResult, PyTypeInfo, Python}; -use std::convert::TryInto; -use std::ffi::CString; -use std::os::raw::{c_char, c_int, c_uint, c_void}; -use std::{mem, ptr}; - -#[inline] -unsafe fn get_type_alloc(tp: *mut ffi::PyTypeObject) -> Option { - mem::transmute(ffi::PyType_GetSlot(tp, ffi::Py_tp_alloc)) -} - -#[inline] -pub(crate) unsafe fn get_type_free(tp: *mut ffi::PyTypeObject) -> Option { - mem::transmute(ffi::PyType_GetSlot(tp, ffi::Py_tp_free)) -} - -/// Workaround for Python issue 35810; no longer necessary in Python 3.8 -#[inline] -#[cfg(not(Py_3_8))] -pub(crate) unsafe fn bpo_35810_workaround(_py: Python, ty: *mut ffi::PyTypeObject) { - #[cfg(Py_LIMITED_API)] - { - // Must check version at runtime for abi3 wheels - they could run against a higher version - // than the build config suggests. - use crate::once_cell::GILOnceCell; - static IS_PYTHON_3_8: GILOnceCell = GILOnceCell::new(); - - if *IS_PYTHON_3_8.get_or_init(_py, || _py.version_info() >= (3, 8)) { - // No fix needed - the wheel is running on a sufficiently new interpreter. - return; - } - } - - ffi::Py_INCREF(ty as *mut ffi::PyObject); -} - -#[inline] -pub(crate) unsafe fn default_new( - py: Python, - subtype: *mut ffi::PyTypeObject, -) -> *mut ffi::PyObject { - // if the class derives native types(e.g., PyDict), call special new - if T::IS_SUBCLASS && ::LayoutAsBase::IS_NATIVE_TYPE { - #[cfg(not(Py_LIMITED_API))] - { - let base_tp = T::BaseType::type_object_raw(py); - if let Some(base_new) = (*base_tp).tp_new { - return base_new(subtype, ptr::null_mut(), ptr::null_mut()); - } - } - #[cfg(Py_LIMITED_API)] - { - // Silence unused parameter warning. - let _ = py; - unreachable!("Subclassing native types isn't support in limited API mode"); - } - } - - let alloc = get_type_alloc(subtype).unwrap_or(ffi::PyType_GenericAlloc); - - #[cfg(not(Py_3_8))] - bpo_35810_workaround(py, subtype); - - alloc(subtype, 0) -} - -/// This trait enables custom `tp_new`/`tp_dealloc` implementations for `T: PyClass`. -pub trait PyClassAlloc: PyTypeInfo + PyClassImpl { - /// Allocate the actual field for `#[pyclass]`. - /// - /// # Safety - /// This function must return a valid pointer to the Python heap. - unsafe fn new(py: Python, subtype: *mut ffi::PyTypeObject) -> *mut Self::Layout { - default_new::(py, subtype) as _ - } - - /// Deallocate `#[pyclass]` on the Python heap. - /// - /// # Safety - /// `self_` must be a valid pointer to the Python heap. - #[allow(clippy::collapsible_if)] // for if cfg! - unsafe fn dealloc(py: Python, self_: *mut Self::Layout) { - (*self_).py_drop(py); - let obj = self_ as *mut ffi::PyObject; - - let ty = ffi::Py_TYPE(obj); - let free = get_type_free(ty).unwrap_or_else(|| tp_free_fallback(ty)); - free(obj as *mut c_void); - - if cfg!(Py_3_8) { - if ffi::PyType_HasFeature(ty, ffi::Py_TPFLAGS_HEAPTYPE) != 0 { - ffi::Py_DECREF(ty as *mut ffi::PyObject); - } - } - } -} - -// Default new implementation - -unsafe extern "C" fn fallback_new( - _subtype: *mut ffi::PyTypeObject, - _args: *mut ffi::PyObject, - _kwds: *mut ffi::PyObject, -) -> *mut ffi::PyObject { - crate::callback_body!(py, { - Err::<(), _>(crate::exceptions::PyTypeError::new_err( - "No constructor defined", - )) - }) -} - -unsafe extern "C" fn tp_dealloc(obj: *mut ffi::PyObject) -where - T: PyClassAlloc, -{ - let pool = crate::GILPool::new(); - let py = pool.python(); - ::dealloc(py, (obj as *mut T::Layout) as _) -} - -pub(crate) unsafe fn tp_free_fallback(ty: *mut ffi::PyTypeObject) -> ffi::freefunc { - if ffi::PyType_IS_GC(ty) != 0 { - ffi::PyObject_GC_Del - } else { - ffi::PyObject_Free - } -} +use crate::{ + class::impl_::{fallback_new, tp_dealloc, PyClassImpl}, + ffi, + pyclass_slots::{PyClassDict, PyClassWeakRef}, + PyCell, PyErr, PyMethodDefType, PyNativeType, PyResult, PyTypeInfo, Python, +}; +use std::{ + convert::TryInto, + ffi::CString, + os::raw::{c_char, c_int, c_uint, c_void}, + ptr, +}; /// If `PyClass` is implemented for `T`, then we can use `T` in the Python world, /// via `PyCell`. @@ -137,7 +18,7 @@ pub(crate) unsafe fn tp_free_fallback(ty: *mut ffi::PyTypeObject) -> ffi::freefu /// The `#[pyclass]` attribute automatically implements this trait for your Rust struct, /// so you don't have to use this trait directly. pub trait PyClass: - PyTypeInfo> + Sized + PyClassAlloc + PyClassImpl> + PyTypeInfo> + PyClassImpl> { /// Specify this class has `#[pyclass(dict)]` or not. type Dict: PyClassDict; @@ -188,12 +69,20 @@ where let mut slots = TypeSlots::default(); slots.push(ffi::Py_tp_base, T::BaseType::type_object_raw(py) as _); - slots.push(ffi::Py_tp_dealloc, tp_dealloc:: as _); if let Some(doc) = tp_doc::()? { slots.push(ffi::Py_tp_doc, doc); } slots.push(ffi::Py_tp_new, T::get_new().unwrap_or(fallback_new) as _); + slots.push(ffi::Py_tp_dealloc, tp_dealloc:: as _); + + if let Some(alloc) = T::get_alloc() { + slots.push(ffi::Py_tp_alloc, alloc as _); + } + if let Some(free) = T::get_free() { + slots.push(ffi::Py_tp_free, free as _); + } + if let Some(call_meth) = T::get_call() { slots.push(ffi::Py_tp_call, call_meth as _); } diff --git a/src/pyclass_init.rs b/src/pyclass_init.rs index b56197305a9..5ba9a230a5a 100644 --- a/src/pyclass_init.rs +++ b/src/pyclass_init.rs @@ -1,15 +1,29 @@ //! Initialization utilities for `#[pyclass]`. -use crate::type_object::{PyLayout, PyTypeInfo}; +use crate::class::impl_::PyClassThreadChecker; +use crate::pyclass_slots::{PyClassDict, PyClassWeakRef}; use crate::{callback::IntoPyCallbackOutput, class::impl_::PyClassBaseType}; -use crate::{PyCell, PyClass, PyResult, Python}; -use std::marker::PhantomData; +use crate::{PyCell, PyClass, PyErr, PyResult, Python, ffi}; +use crate::{ + ffi::PyTypeObject, + pycell::{BorrowFlag, PyCellContents}, + type_object::{get_tp_alloc, PyTypeInfo}, +}; +use std::{ + cell::{Cell, UnsafeCell}, + marker::PhantomData, + mem::{ManuallyDrop, MaybeUninit}, +}; /// Initializer for Python types. /// /// This trait is intended to use internally for distinguishing `#[pyclass]` and /// Python native types. pub trait PyObjectInit: Sized { - fn init_class>(self, layout: &mut L); + unsafe fn into_new_object( + self, + py: Python, + subtype: *mut PyTypeObject, + ) -> PyResult<*mut ffi::PyObject>; private_decl! {} } @@ -17,7 +31,46 @@ pub trait PyObjectInit: Sized { pub struct PyNativeTypeInitializer(PhantomData); impl PyObjectInit for PyNativeTypeInitializer { - fn init_class>(self, _layout: &mut L) {} + unsafe fn into_new_object( + self, + py: Python, + subtype: *mut PyTypeObject, + ) -> PyResult<*mut ffi::PyObject> { + let type_object = T::type_object_raw(py); + + // HACK (due to FIXME below): PyBaseObject_Type's tp_new isn't happy with NULL arguments + if type_object == (&ffi::PyBaseObject_Type as *const _ as *mut _) { + let alloc = get_tp_alloc(subtype).unwrap_or(ffi::PyType_GenericAlloc); + let obj = alloc(subtype, 0); + return if obj.is_null() { + Err(PyErr::fetch(py)) + } else { + Ok(obj) + }; + } + + #[cfg(Py_LIMITED_API)] + unreachable!("subclassing native types is not possible with the `abi3` feature"); + + #[cfg(not(Py_LIMITED_API))] + { + match (*type_object).tp_new { + // FIXME: Call __new__ with actual arguments + Some(newfunc) => { + let obj = newfunc(subtype, std::ptr::null_mut(), std::ptr::null_mut()); + if obj.is_null() { + Err(PyErr::fetch(py)) + } else { + Ok(obj) + } + } + None => Err(crate::exceptions::PyTypeError::new_err( + "base type without tp_new", + )), + } + } + } + private_impl! {} } @@ -142,22 +195,52 @@ impl PyClassInitializer { where T: PyClass, { - let cell = PyCell::internal_new(py, subtype)?; - self.init_class(&mut *cell); - Ok(cell) + self.into_new_object(py, subtype).map(|obj| obj as _) } } impl PyObjectInit for PyClassInitializer { - fn init_class>(self, layout: &mut L) { + unsafe fn into_new_object( + self, + py: Python, + subtype: *mut PyTypeObject, + ) -> PyResult<*mut ffi::PyObject> { + /// Layout of a PyCellBase after base new has been called, but borrow flag has not yet been initialized. + struct PartiallyInitializedPyCellBase { + _ob_base: T, + borrow_flag: MaybeUninit>, + } + + /// Layout of a PyCell after base new has been called, but contents have not yet been written. + struct PartiallyInitializedPyCell { + _ob_base: ::LayoutAsBase, + contents: MaybeUninit>, + } + let Self { init, super_init } = self; - // Safety: A valid PyLayout must contain the base layout as the first entry, so casting L to - // T::BaseType::LayoutAsBase is ok. - super_init.init_class(unsafe { - &mut *(layout as *mut _ as *mut ::LayoutAsBase) - }); - layout.py_init(init); + let obj = super_init.into_new_object(py, subtype)?; + + // FIXME: Only need to initialize borrow flag once per whole hierarchy + let base: *mut PartiallyInitializedPyCellBase = obj as _; + std::ptr::write( + (*base).borrow_flag.as_mut_ptr(), + Cell::new(BorrowFlag::UNUSED), + ); + + // FIXME: Initialize borrow flag if necessary?? + let cell: *mut PartiallyInitializedPyCell = obj as _; + std::ptr::write( + (*cell).contents.as_mut_ptr(), + PyCellContents { + value: ManuallyDrop::new(UnsafeCell::new(init)), + thread_checker: T::ThreadChecker::new(), + dict: T::Dict::new(), + weakref: T::WeakRef::new(), + }, + ); + Ok(obj) } + private_impl! {} } @@ -166,6 +249,7 @@ where T: PyClass, T::BaseType: PyClassBaseType>, { + #[inline] fn from(value: T) -> PyClassInitializer { Self::new(value, PyNativeTypeInitializer(PhantomData)) } @@ -190,6 +274,7 @@ where T: PyClass, U: Into>, { + #[inline] fn convert(self, _py: Python) -> PyResult> { Ok(self.into()) } diff --git a/src/pyclass_slots.rs b/src/pyclass_slots.rs index 5b182102afc..74b6ef81fc7 100644 --- a/src/pyclass_slots.rs +++ b/src/pyclass_slots.rs @@ -6,6 +6,7 @@ use crate::{ffi, Python}; pub trait PyClassDict { const IS_DUMMY: bool = true; fn new() -> Self; + #[inline] fn clear_dict(&mut self, _py: Python) {} private_decl! {} } @@ -14,6 +15,7 @@ pub trait PyClassDict { pub trait PyClassWeakRef { const IS_DUMMY: bool = true; fn new() -> Self; + #[inline] unsafe fn clear_weakrefs(&mut self, _obj: *mut ffi::PyObject, _py: Python) {} private_decl! {} } @@ -23,6 +25,7 @@ pub struct PyClassDummySlot; impl PyClassDict for PyClassDummySlot { private_impl! {} + #[inline] fn new() -> Self { PyClassDummySlot } @@ -30,6 +33,7 @@ impl PyClassDict for PyClassDummySlot { impl PyClassWeakRef for PyClassDummySlot { private_impl! {} + #[inline] fn new() -> Self { PyClassDummySlot } @@ -44,9 +48,11 @@ pub struct PyClassDictSlot(*mut ffi::PyObject); impl PyClassDict for PyClassDictSlot { private_impl! {} const IS_DUMMY: bool = false; + #[inline] fn new() -> Self { Self(std::ptr::null_mut()) } + #[inline] fn clear_dict(&mut self, _py: Python) { if !self.0.is_null() { unsafe { ffi::PyDict_Clear(self.0) } @@ -63,9 +69,11 @@ pub struct PyClassWeakRefSlot(*mut ffi::PyObject); impl PyClassWeakRef for PyClassWeakRefSlot { private_impl! {} const IS_DUMMY: bool = false; + #[inline] fn new() -> Self { Self(std::ptr::null_mut()) } + #[inline] unsafe fn clear_weakrefs(&mut self, obj: *mut ffi::PyObject, _py: Python) { if !self.0.is_null() { ffi::PyObject_ClearWeakRefs(obj) diff --git a/src/type_object.rs b/src/type_object.rs index 84b6096d488..2282d29d8f8 100644 --- a/src/type_object.rs +++ b/src/type_object.rs @@ -3,7 +3,8 @@ use crate::internal_tricks::extract_cstr_or_leak_cstring; use crate::once_cell::GILOnceCell; -use crate::pyclass::{create_type_object, PyClass}; +use crate::pyclass::create_type_object; +use crate::pyclass::PyClass; use crate::types::{PyAny, PyType}; use crate::{conversion::IntoPyPointer, PyMethodDefType}; use crate::{ffi, AsPyPointer, PyErr, PyNativeType, PyObject, PyResult, Python}; @@ -15,11 +16,7 @@ use std::thread::{self, ThreadId}; /// is of `PyAny`. /// /// This trait is intended to be used internally. -pub unsafe trait PyLayout { - const IS_NATIVE_TYPE: bool = true; - fn py_init(&mut self, _value: T) {} - unsafe fn py_drop(&mut self, _py: Python) {} -} +pub unsafe trait PyLayout {} /// `T: PySizedLayout` represents `T` is not a instance of /// [`PyVarObject`](https://docs.python.org/3.8/c-api/structures.html?highlight=pyvarobject#c.PyVarObject). @@ -200,3 +197,32 @@ fn initialize_tp_dict( // This is necessary for making static `LazyStaticType`s unsafe impl Sync for LazyStaticType {} + +#[inline] +pub(crate) unsafe fn get_tp_alloc(tp: *mut ffi::PyTypeObject) -> Option { + #[cfg(not(Py_LIMITED_API))] + { + (*tp).tp_alloc + } + + #[cfg(Py_LIMITED_API)] + { + let ptr = ffi::PyType_GetSlot(tp, ffi::Py_tp_alloc); + std::mem::transmute(ptr) + } +} + +#[inline] +pub(crate) unsafe fn get_tp_free(tp: *mut ffi::PyTypeObject) -> ffi::freefunc { + #[cfg(not(Py_LIMITED_API))] + { + (*tp).tp_free.unwrap() + } + + #[cfg(Py_LIMITED_API)] + { + let ptr = ffi::PyType_GetSlot(tp, ffi::Py_tp_free); + debug_assert_ne!(ptr, std::ptr::null_mut()); + std::mem::transmute(ptr) + } +} diff --git a/tests/test_inheritance.rs b/tests/test_inheritance.rs index 1e8a7fbb61d..01b124b8be9 100644 --- a/tests/test_inheritance.rs +++ b/tests/test_inheritance.rs @@ -210,6 +210,23 @@ mod inheriting_native_type { ); } + #[test] + fn inherit_dict_drop() { + Python::with_gil(|py| { + let dict_sub = pyo3::Py::new(py, DictWithName::new()).unwrap(); + assert_eq!(dict_sub.get_refcnt(py), 1); + + let item = py.eval("object()", None, None).unwrap(); + assert_eq!(item.get_refcnt(), 1); + + dict_sub.as_ref(py).set_item("foo", item).unwrap(); + assert_eq!(item.get_refcnt(), 2); + + drop(dict_sub); + assert_eq!(item.get_refcnt(), 1); + }) + } + #[pyclass(extends=PyException)] struct CustomException { #[pyo3(get)]