From 229a9c8578f88be9f249cd27f20f2a959b7561ac Mon Sep 17 00:00:00 2001 From: Noa <33094578+coolreader18@users.noreply.github.com> Date: Tue, 9 Nov 2021 16:04:02 -0600 Subject: [PATCH] Make PyRef wrap a NonNull> This makes debuggers (or rust-gdb, at least) more pleasant to use, since you don't have to manually cast `PyRef.obj.ptr as `*const PyObjectView` Also get rid of PyGenericObject, since it's vestigial --- derive/src/pyclass.rs | 2 +- vm/src/builtins/object.rs | 6 +- vm/src/pyobject.rs | 5 +- vm/src/pyobjectrc.rs | 119 ++++++++++++++++++--------------- vm/src/vm.rs | 2 +- wasm/lib/src/browser_module.rs | 6 +- 6 files changed, 75 insertions(+), 65 deletions(-) diff --git a/derive/src/pyclass.rs b/derive/src/pyclass.rs index 81a7a6aff..b148e951a 100644 --- a/derive/src/pyclass.rs +++ b/derive/src/pyclass.rs @@ -667,7 +667,7 @@ impl ToTokens for GetSetNursery { #( #cfgs )* class.set_str_attr( #name, - ::rustpython_vm::PyGenericObject::new( + ::rustpython_vm::PyRef::new_ref( ::rustpython_vm::builtins::PyGetSet::new(#name.into(), class.clone()) .with_get(&Self::#getter) #setter #deleter, diff --git a/vm/src/builtins/object.rs b/vm/src/builtins/object.rs index 8467bcd10..bbd64e54e 100644 --- a/vm/src/builtins/object.rs +++ b/vm/src/builtins/object.rs @@ -2,8 +2,8 @@ use super::{PyDict, PyDictRef, PyList, PyStr, PyStrRef, PyType, PyTypeRef}; use crate::common::hash::PyHash; use crate::{ function::FuncArgs, types::PyComparisonOp, utils::Either, IdProtocol, ItemProtocol, - PyArithmeticValue, PyAttributes, PyClassImpl, PyComparisonValue, PyContext, PyGenericObject, - PyObject, PyObjectRef, PyResult, PyValue, TypeProtocol, VirtualMachine, + PyArithmeticValue, PyAttributes, PyClassImpl, PyComparisonValue, PyContext, PyObject, + PyObjectRef, PyResult, PyValue, TypeProtocol, VirtualMachine, }; /// object() @@ -34,7 +34,7 @@ impl PyBaseObject { } else { Some(vm.ctx.new_dict()) }; - Ok(PyGenericObject::new(PyBaseObject, cls, dict)) + Ok(crate::PyRef::new_ref(PyBaseObject, cls, dict).into()) } #[pyslot] diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 82d29f862..e6f0abd5e 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -4,8 +4,7 @@ use crate::common::{ static_cell, }; pub use crate::pyobjectrc::{ - PyGenericObject, PyObject, PyObjectRef, PyObjectView, PyObjectWeak, PyObjectWrap, PyRef, - PyWeakRef, + PyObject, PyObjectRef, PyObjectView, PyObjectWeak, PyObjectWrap, PyRef, PyWeakRef, }; use crate::{ builtins::{ @@ -334,7 +333,7 @@ impl PyContext { class.slots.flags.contains(PyTypeFlags::HAS_DICT), dict.is_some() ); - PyGenericObject::new(object::PyBaseObject, class, dict) + PyRef::new_ref(object::PyBaseObject, class, dict).into() } } diff --git a/vm/src/pyobjectrc.rs b/vm/src/pyobjectrc.rs index bdaa0bf67..91271af9f 100644 --- a/vm/src/pyobjectrc.rs +++ b/vm/src/pyobjectrc.rs @@ -15,7 +15,7 @@ use std::mem::ManuallyDrop; use std::ops::Deref; use std::ptr::{self, NonNull}; -// so, PyObjectRef is basically equivalent to `PyRc>`, except it's +// so, PyObjectRef is basically equivalent to `PyRc>`, except it's // only one pointer in width rather than 2. We do that by manually creating a vtable, and putting // a &'static reference to it inside the `PyRc` rather than adjacent to it, like trait objects do. // This can lead to faster code since there's just less data to pass around, as well as because of @@ -32,18 +32,18 @@ use std::ptr::{self, NonNull}; // There has to be padding in the space between the 2 fields. But, if that field is a trait object // (like `dyn PyObjectPayload`) we don't *know* how much padding there is between the `payload` // field and the previous field. So, Rust has to consult the vtable to know the exact offset of -// `payload` in `PyGenericObject`, which has a huge performance impact when *every +// `payload` in `PyInner`, which has a huge performance impact when *every // single payload access* requires a vtable lookup. Thankfully, we're able to avoid that because of // the way we use PyObjectRef, in that whenever we want to access the payload we (almost) always // access it from a generic function. So, rather than doing // // - check vtable for payload offset -// - get offset in PyGenericObject struct +// - get offset in PyInner struct // - call as_any() method of PyObjectPayload // - call downcast_ref() method of Any // we can just do // - check vtable that typeid matches -// - pointer cast directly to *const PyGenericObject +// - pointer cast directly to *const PyInner // // and at that point the compiler can know the offset of `payload` for us because **we've given it a // concrete type to work with before we ever access the `payload` field** @@ -395,18 +395,9 @@ impl InstanceDict { } } -/// This is an actual python object. It consists of a `typ` which is the -/// python class, and carries some rust payload optionally. This rust -/// payload can be a rust float or rust int in case of float and int objects. -#[repr(transparent)] -pub struct PyGenericObject { - inner: PyInner, -} - -impl PyGenericObject { - #[allow(clippy::new_ret_no_self)] - pub fn new(payload: T, typ: PyTypeRef, dict: Option) -> PyObjectRef { - let inner = PyInner { +impl PyInner { + fn new(payload: T, typ: PyTypeRef, dict: Option) -> Box { + Box::new(PyInner { refcount: RefCount::new(), typeid: TypeId::of::(), vtable: PyObjVTable::of::(), @@ -414,8 +405,7 @@ impl PyGenericObject { dict: dict.map(InstanceDict::new), weaklist: WeakRefList::new(), payload, - }; - PyObjectRef::new(PyGenericObject { inner }) + }) } } @@ -501,13 +491,6 @@ impl PyObjectRef { } } - fn new(value: PyGenericObject) -> Self { - let inner: *const PyInner = Box::into_raw(Box::new(value.inner)); - Self { - ptr: unsafe { NonNull::new_unchecked(inner as *mut PyObject) }, - } - } - /// Attempt to downcast this reference to a subclass. /// /// If the downcast fails, the original ref is returned in as `Err` so @@ -712,12 +695,9 @@ impl PyObject { pub fn as_raw(&self) -> *const PyObject { self } -} -impl PyObjectRef { - #[inline(never)] - #[cold] - fn drop_slow(&mut self) { + #[inline] + fn drop_slow_inner(&self) -> Result<(), ()> { // CPython-compatible drop implementation if let Some(slot_del) = self.class().mro_find_map(|cls| cls.slots.del.load()) { let ret = crate::vm::thread::with_vm(self, |vm| { @@ -731,7 +711,7 @@ impl PyObjectRef { // the decref right above set refcount back to 0 Some(true) => {} // we've been resurrected by __del__ - Some(false) => return, + Some(false) => return Err(()), None => { warn!("couldn't run __del__ method for object") } @@ -741,8 +721,20 @@ impl PyObjectRef { wrl.clear(); } - let drop_dealloc = self.0.vtable.drop_dealloc; - unsafe { drop_dealloc(self.ptr.as_ptr()) } + Ok(()) + } + + /// Can only be called when refcount has dropped to zero. `ptr` must be valid + #[inline(never)] + #[cold] + unsafe fn drop_slow(ptr: NonNull) { + if let Err(()) = ptr.as_ref().drop_slow_inner() { + // abort drop for whatever reason + return; + } + let drop_dealloc = ptr.as_ref().0.vtable.drop_dealloc; + // call drop only when there are no references in scope - stacked borrows stuff + drop_dealloc(ptr.as_ptr()) } } @@ -805,7 +797,7 @@ impl PyObjectWeak { impl Drop for PyObjectRef { fn drop(&mut self) { if self.0.refcount.dec() { - self.drop_slow() + unsafe { PyObject::drop_slow(self.ptr) } } } } @@ -870,9 +862,9 @@ impl ToOwned for PyObjectView { #[inline(always)] fn to_owned(&self) -> Self::Owned { + self.0.refcount.inc(); PyRef { - obj: self.as_object().to_owned(), - _marker: PhantomData, + ptr: NonNull::from(self), } } } @@ -911,9 +903,14 @@ impl fmt::Debug for PyObjectView { /// where a reference to the same object must be returned. #[repr(transparent)] pub struct PyRef { - // invariant: this obj must always have payload of type T - obj: PyObjectRef, - _marker: PhantomData, + ptr: NonNull>, +} + +cfg_if::cfg_if! { + if #[cfg(feature = "threading")] { + unsafe impl Send for PyRef {} + unsafe impl Sync for PyRef {} + } } impl fmt::Debug for PyRef { @@ -922,6 +919,15 @@ impl fmt::Debug for PyRef { } } +impl Drop for PyRef { + #[inline] + fn drop(&mut self) { + if self.0.refcount.dec() { + unsafe { PyObject::drop_slow(self.ptr.cast::()) } + } + } +} + impl Clone for PyRef { #[inline(always)] fn clone(&self) -> Self { @@ -932,25 +938,26 @@ impl Clone for PyRef { impl PyRef { unsafe fn from_raw(raw: *const PyObjectView) -> Self { Self { - obj: PyObjectRef::from_raw(raw as _), - _marker: PhantomData, + ptr: NonNull::new_unchecked(raw as *mut _), } } /// Safety: payload type of `obj` must be `T` + #[inline] unsafe fn from_obj_unchecked(obj: PyObjectRef) -> Self { debug_assert!(obj.payload_is::()); + let obj = ManuallyDrop::new(obj); Self { - obj, - _marker: PhantomData, + ptr: obj.ptr.cast(), } } - // ideally we'd be able to define this in pyobject.rs, but method visibility rules are weird + #[inline] pub fn new_ref(payload: T, typ: crate::builtins::PyTypeRef, dict: Option) -> Self { - let obj = PyGenericObject::new(payload, typ, dict); - // SAFETY: we just created the object from a payload of type T - unsafe { Self::from_obj_unchecked(obj) } + let inner = Box::into_raw(PyInner::new(payload, typ, dict)); + Self { + ptr: unsafe { NonNull::new_unchecked(inner.cast::>()) }, + } } } @@ -958,8 +965,10 @@ impl PyObjectWrap for PyRef where T: PyObjectPayload, { + #[inline] fn into_object(self) -> PyObjectRef { - self.obj + let me = ManuallyDrop::new(self); + PyObjectRef { ptr: me.ptr.cast() } } } @@ -967,8 +976,9 @@ impl AsRef for PyRef where T: PyObjectPayload, { + #[inline(always)] fn as_ref(&self) -> &PyObject { - &self.obj + (**self).as_object() } } @@ -987,8 +997,9 @@ where { type Target = PyObjectView; + #[inline(always)] fn deref(&self) -> &PyObjectView { - unsafe { &*(&*self.obj as *const PyObject as *const PyObjectView) } + unsafe { self.ptr.as_ref() } } } @@ -1000,10 +1011,10 @@ pub struct PyWeakRef { impl PyWeakRef { pub fn upgrade(&self) -> Option> { - self.weak.upgrade().map(|obj| PyRef { - obj, - _marker: PhantomData, - }) + self.weak + .upgrade() + // SAFETY: PyWeakRef was always created from a PyRef, so the object is T + .map(|obj| unsafe { PyRef::from_obj_unchecked(obj) }) } } diff --git a/vm/src/vm.rs b/vm/src/vm.rs index b097fed30..49b2583f7 100644 --- a/vm/src/vm.rs +++ b/vm/src/vm.rs @@ -1936,7 +1936,7 @@ impl<'vm> ReprGuard<'vm> { let mut guards = vm.repr_guards.borrow_mut(); // Should this be a flag on the obj itself? putting it in a global variable for now until it - // decided the form of the PyGenericObject. https://github.com/RustPython/RustPython/issues/371 + // decided the form of PyObject. https://github.com/RustPython/RustPython/issues/371 let id = obj.get_id(); if guards.contains(&id) { return None; diff --git a/wasm/lib/src/browser_module.rs b/wasm/lib/src/browser_module.rs index 49996bd0f..2a4f1ae62 100644 --- a/wasm/lib/src/browser_module.rs +++ b/wasm/lib/src/browser_module.rs @@ -10,7 +10,7 @@ mod _browser { builtins::{PyDictRef, PyStrRef}, function::{ArgCallable, IntoPyObject, OptionalArg}, import::import_file, - PyClassImpl, PyGenericObject, PyObjectRef, PyResult, PyValue, VirtualMachine, + PyClassImpl, PyObjectRef, PyRef, PyResult, PyValue, VirtualMachine, }; use wasm_bindgen::{prelude::*, JsCast}; use wasm_bindgen_futures::JsFuture; @@ -179,8 +179,8 @@ mod _browser { } #[pyattr] - fn document(vm: &VirtualMachine) -> PyObjectRef { - PyGenericObject::new( + fn document(vm: &VirtualMachine) -> PyRef { + PyRef::new_ref( Document { doc: window().document().expect("Document missing from window"), },