Skip to content

Remove inner Box from PyObject #671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions vm/src/obj/objtype.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,10 +339,10 @@ pub fn new(
let mros = bases.into_iter().map(|x| _mro(&x)).collect();
let mro = linearise_mro(mros).unwrap();
Ok(PyObject {
payload: Box::new(PyClass {
payload: PyClass {
name: String::from(name),
mro,
}),
},
dict: Some(RefCell::new(dict)),
typ,
}
Expand Down
4 changes: 2 additions & 2 deletions vm/src/obj/objweakref.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use crate::obj::objtype::PyClassRef;
use crate::pyobject::PyValue;
use crate::pyobject::{PyContext, PyObject, PyObjectRef, PyRef, PyResult};
use crate::pyobject::{PyContext, PyObject, PyObjectPayload, PyObjectRef, PyRef, PyResult};
use crate::vm::VirtualMachine;

use std::rc::{Rc, Weak};

#[derive(Debug)]
pub struct PyWeak {
referent: Weak<PyObject>,
referent: Weak<PyObject<dyn PyObjectPayload>>,
}

impl PyWeak {
Expand Down
55 changes: 27 additions & 28 deletions vm/src/pyobject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ Basically reference counting, but then done by rust.
/// this reference counting is accounted for by this type. Use the `.clone()`
/// method to create a new reference and increment the amount of references
/// to the python object by 1.
pub type PyObjectRef = Rc<PyObject>;
pub type PyObjectRef = Rc<PyObject<dyn PyObjectPayload>>;

/// Use this type for function which return a python object or and exception.
/// Both the python object and the python exception are `PyObjectRef` types
Expand All @@ -82,7 +82,7 @@ pub type PyResult<T = PyObjectRef> = Result<T, PyObjectRef>; // A valid value, o
/// faster, unordered, and only supports strings as keys.
pub type PyAttributes = HashMap<String, PyObjectRef>;

impl fmt::Display for PyObject {
impl fmt::Display for PyObject<dyn PyObjectPayload> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
use self::TypeProtocol;
if let Some(PyClass { ref name, .. }) = self.payload::<PyClass>() {
Expand Down Expand Up @@ -187,25 +187,25 @@ fn init_type_hierarchy() -> (PyObjectRef, PyObjectRef) {
let object_type = PyObject {
typ: mem::uninitialized(), // !
dict: Some(RefCell::new(PyAttributes::new())),
payload: Box::new(PyClass {
payload: PyClass {
name: String::from("object"),
mro: vec![],
}),
},
}
.into_ref();

let type_type = PyObject {
typ: mem::uninitialized(), // !
dict: Some(RefCell::new(PyAttributes::new())),
payload: Box::new(PyClass {
payload: PyClass {
name: String::from("type"),
mro: vec![FromPyObjectRef::from_pyobj(&object_type)],
}),
},
}
.into_ref();

let object_type_ptr = PyObjectRef::into_raw(object_type.clone()) as *mut PyObject;
let type_type_ptr = PyObjectRef::into_raw(type_type.clone()) as *mut PyObject;
let object_type_ptr = PyObjectRef::into_raw(object_type.clone()) as *mut PyObject<PyClass>;
let type_type_ptr = PyObjectRef::into_raw(type_type.clone()) as *mut PyObject<PyClass>;
ptr::write(&mut (*object_type_ptr).typ, type_type.clone());
ptr::write(&mut (*type_type_ptr).typ, type_type.clone());

Expand Down Expand Up @@ -627,7 +627,7 @@ impl PyContext {
PyObject {
typ: class,
dict: Some(RefCell::new(dict)),
payload: Box::new(objobject::PyInstance),
payload: objobject::PyInstance,
}
.into_ref()
}
Expand Down Expand Up @@ -690,10 +690,10 @@ impl Default for PyContext {
/// 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.
pub struct PyObject {
pub struct PyObject<T: ?Sized> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the effect of this template on the code size? Did you check this? This is of interest to the wasm build, since it may be intended for web.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There aren't really any methods on PyObject, so making it generic won't duplicate much code; it's just a container.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could throw some #[inline]s on PyObject::new and PyObject::into_ref but those are the only ones that are really generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This increases the binary size by about 2.4% for me, which seems acceptable.

(A side note - moving to the new args style should decrease the binary size because there will only be one monomorphization per distinct function signature, whereas arg_check! will generate type checking code for each one.)

pub typ: PyObjectRef,
pub dict: Option<RefCell<PyAttributes>>, // __dict__ member
pub payload: Box<dyn PyObjectPayload>,
pub payload: T,
}

/// A reference to a Python object.
Expand All @@ -706,7 +706,7 @@ pub struct PyObject {
/// situations (such as when implementing in-place methods such as `__iadd__`)
/// where a reference to the same object must be returned.
#[derive(Clone, Debug)]
pub struct PyRef<T> {
pub struct PyRef<T: ?Sized> {
// invariant: this obj must always have payload of type T
obj: PyObjectRef,
_payload: PhantomData<T>,
Expand Down Expand Up @@ -749,21 +749,15 @@ impl<T: PyValue> PyRef<T> {
}
}

impl<T> Deref for PyRef<T>
where
T: PyValue,
{
impl<T: PyValue> Deref for PyRef<T> {
type Target = T;

fn deref(&self) -> &T {
self.obj.payload().expect("unexpected payload for type")
}
}

impl<T> TryFromObject for PyRef<T>
where
T: PyValue,
{
impl<T: PyValue> TryFromObject for PyRef<T> {
fn try_from_object(vm: &mut VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
if objtype::isinstance(&obj, &T::class(vm)) {
Ok(PyRef {
Expand All @@ -788,7 +782,7 @@ impl<T> IntoPyObject for PyRef<T> {
}
}

impl<T> fmt::Display for PyRef<T> {
impl<T: ?Sized> fmt::Display for PyRef<T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
self.obj.fmt(f)
}
Expand All @@ -806,7 +800,10 @@ pub trait IdProtocol {

impl IdProtocol for PyObjectRef {
fn get_id(&self) -> usize {
&*self as &PyObject as *const PyObject as usize
// This is kinda ridiculous...
//
// Rc -> (fat) shared ref -> (fat) pointer -> (thin) pointer -> usize
&*self as &_ as *const _ as *const PyObject<()> as usize
}
}

Expand All @@ -830,7 +827,7 @@ impl TypeProtocol for PyObjectRef {
}
}

impl TypeProtocol for PyObject {
impl<T: ?Sized> TypeProtocol for PyObject<T> {
fn type_ref(&self) -> &PyObjectRef {
&self.typ
}
Expand Down Expand Up @@ -969,9 +966,9 @@ impl BufferProtocol for PyObjectRef {
}
}

impl fmt::Debug for PyObject {
impl<T: ?Sized + fmt::Debug> fmt::Debug for PyObject<T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "[PyObj {:?}]", self.payload)
write!(f, "[PyObj {:?}]", &self.payload)
}
}

Expand Down Expand Up @@ -1553,12 +1550,12 @@ impl PyValue for PyIteratorValue {
}
}

impl PyObject {
pub fn new<T: PyObjectPayload>(payload: T, typ: PyObjectRef) -> PyObjectRef {
impl<T: PyObjectPayload> PyObject<T> {
pub fn new(payload: T, typ: PyObjectRef) -> PyObjectRef {
PyObject {
typ,
dict: Some(RefCell::new(PyAttributes::new())),
payload: Box::new(payload),
payload,
}
.into_ref()
}
Expand All @@ -1567,7 +1564,9 @@ impl PyObject {
pub fn into_ref(self) -> PyObjectRef {
Rc::new(self)
}
}

impl PyObject<dyn PyObjectPayload> {
#[inline]
pub fn payload<T: PyValue>(&self) -> Option<&T> {
self.payload.as_any().downcast_ref()
Expand Down
4 changes: 2 additions & 2 deletions vm/src/vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ impl VirtualMachine {
}

// TODO: is it safe to just invoke __call__ otherwise?
trace!("invoke __call__ for: {:?}", func_ref.payload);
trace!("invoke __call__ for: {:?}", &func_ref.payload);
self.call_method(&func_ref, "__call__", args)
}

Expand Down Expand Up @@ -554,7 +554,7 @@ impl VirtualMachine {
let cls = obj.typ();
match cls.get_attr(method_name) {
Some(method) => self.call_get_descriptor(method, obj.clone()),
None => Err(self.new_type_error(format!("{} has no method {:?}", obj, method_name))),
None => Err(self.new_type_error(format!("{} has no method {:?}", &obj, method_name))),
}
}

Expand Down
14 changes: 5 additions & 9 deletions wasm/lib/src/vm_class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,20 @@ use js_sys::{Object, Reflect, SyntaxError, TypeError};
use rustpython_vm::{
compile,
frame::{NameProtocol, Scope},
pyobject::{PyContext, PyFuncArgs, PyObjectRef, PyResult},
pyobject::{PyContext, PyFuncArgs, PyObject, PyObjectPayload, PyObjectRef, PyResult},
VirtualMachine,
};
use std::cell::RefCell;
use std::collections::HashMap;
use std::rc::{Rc, Weak};
use wasm_bindgen::{prelude::*, JsCast};

pub trait HeldRcInner {}

impl<T> HeldRcInner for T {}

pub(crate) struct StoredVirtualMachine {
pub vm: VirtualMachine,
pub scope: Scope,
/// you can put a Rc in here, keep it as a Weak, and it'll be held only for
/// as long as the StoredVM is alive
held_rcs: Vec<Rc<dyn HeldRcInner>>,
held_rcs: Vec<PyObjectRef>,
}

impl StoredVirtualMachine {
Expand Down Expand Up @@ -222,10 +218,10 @@ impl WASMVirtualMachine {
STORED_VMS.with(|cell| cell.borrow().contains_key(&self.id))
}

pub(crate) fn push_held_rc<T: HeldRcInner + 'static>(
pub(crate) fn push_held_rc(
&self,
rc: Rc<T>,
) -> Result<Weak<T>, JsValue> {
rc: PyObjectRef,
) -> Result<Weak<PyObject<dyn PyObjectPayload + 'static>>, JsValue> {
self.with(|stored_vm| {
let weak = Rc::downgrade(&rc);
stored_vm.held_rcs.push(rc);
Expand Down