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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Remove inner Box from PyObject #671

wants to merge 5 commits into from

Conversation

OddCoincidence
Copy link
Contributor

@OddCoincidence OddCoincidence commented Mar 13, 2019 •
edited
Loading

(WIP: will need to incorporate changes from #670 and fixup Debug / Display impls)

This removes the Box inside PyObject, so that a PyRef / PyObjectRef only needs to follow a single pointer (just like cpython).

windelbouwman
windelbouwman reviewed Mar 13, 2019
View reviewed changes
vm/src/pyobject.rs
@@ -695,10 +695,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.)

@OddCoincidence
Copy link
Contributor Author

@coolreader18 it looks like something in wasm isn't getting along with this, could you take a look?

error[E0277]: the size for values of type `dyn rustpython_vm::pyobject::PyObjectPayload` cannot be known at compilation time
  --> wasm/lib/src/convert.rs:70:39
   |
70 |             let weak_py_obj = wasm_vm.push_held_rc(py_obj).unwrap();
   |                                       ^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: within `rustpython_vm::pyobject::PyObject<dyn rustpython_vm::pyobject::PyObjectPayload>`, the trait `std::marker::Sized` is not implemented for `dyn rustpython_vm::pyobject::PyObjectPayload`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: required because it appears within the type `rustpython_vm::pyobject::PyObject<dyn rustpython_vm::pyobject::PyObjectPayload>`

@coolreader18
Copy link
Member

Ah, you can just put : ?Sized on trait HeldRcInner {} at the top of vm_class.rs.

@OddCoincidence
Copy link
Contributor Author

Yeah, I already tried that - the problem is that now PyObjectRef is a fat pointer (because dyn PyObjectPayload is unsized) so it cannot be made into a dyn HeldRcInner because that would need a "super-fat pointer" (size and vtable), which doesn't exist.

@coolreader18
Copy link
Member

Hmm, you could probably just change held_rcs to be a Vec of PyObjectRefs then. If we needed to use it for something else other than PyObjects we could figure it out once we get there.

@coolreader18 coolreader18 mentioned this pull request Mar 17, 2019
Open
@coolreader18
Copy link
Member

Are you still working on this?

@OddCoincidence
Copy link
Contributor Author

Yeah, but merging in master caused an infinite loop in linearise_mro that I haven't had time to debug. I'll get to it this weekend hopefully.

@OddCoincidence
Copy link
Contributor Author

Redone in #671

@OddCoincidence OddCoincidence deleted the joey/unboxed-payload branch April 9, 2019 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@windelbouwman windelbouwman windelbouwman left review comments

@coolreader18 coolreader18 coolreader18 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@OddCoincidence @coolreader18 @windelbouwman