-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Implement object.__sizeof__
#3421
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
Conversation
vm/src/builtins/dict.rs
Outdated
|
|
||
| #[pymethod(magic)] | ||
| fn sizeof(zelf: PyRef<Self>) -> usize { | ||
| zelf.dict().sizeof() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dict has data but dict_keys is just a view of dict. So it would be not the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could just be size_of::<Self> and could be implemented directly on DictView.
dict_keys.__sizeof__dict_keys.__sizeof__, dict_items.__sizeof__, dict_values.__sizeof__
|
@moreal is there anything more work for this PR? |
|
When I was working on this pull request, I realized there is no So I'll implement P.S. The reason I didn't any action for this pull request, is just I forgot. Thanks! |
dict_keys.__sizeof__, dict_items.__sizeof__, dict_values.__sizeof__object.__sizeof__
598326e to
40421ac
Compare
vm/src/builtins/object.rs
Outdated
| // TODO: CPython implementation has `PyVarObject.ob_size` field for each objects. | ||
| // Its default value seems 0 but it is changed by dynamically. | ||
| let object_size = 0; | ||
| size = object_size * isize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About these comments, I couldn't find an API to get PyObject's size yet. 😞
You can see the original implementation at https://github.com/python/cpython/blob/6d430ef5ab62158a200b94dff31b89524a9576bb/Objects/typeobject.c#L5481-L5494.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyTypeObject PyBaseObject_Type = {
PyVarObject_HEAD_INIT(&PyType_Type, 0)
"object",
...I think that second argument in PyVarObject_HEAD_INIT is ob_size and that PyVarObject_HEAD_INIT assigned to ob_base in each types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! It was helpful to analyze this issue again. 🙏🏻
|
Regarding AFAIU RustPython does not try to replicate this interface exactly. For example, in RustPython If you want, I can make a separate pull request, since this issue is already inactive since more than four months. |
|
I stopped to work on this pull request since I met the situation I couldn't find the way to get RustPython object's memory size. If not, I'll continue to look this pull request on this weekend. |
|
I merged #3728. If this PR has more enhancement, please keep go ahead. |
|
This comment is a note to record the progress to solve this issue. At current commit, when we run the below code, it seems like unnatural. # RustPython
>>>>> object.__sizeof__(1223456789812391231291231231231212312312312312312312321321321321312312321123123123199129391239219394923912949213021949302194942130123949203912430392402139210492139123012940219394923942395943856228368385)
32
# CPython
>>> object.__sizeof__(1223456789812391231291231231231212312312312312312312321321321321312312321123123123199129391239219394923912949213021949302194942130123949203912430392402139210492139123012940219394923942395943856228368385)
116When I build the CPython with debugging log in local environment and I ran it, the The
Then It will be the solution to introduce |
6e15bd6 to
86d4999
Compare
|
Does |
vm/src/builtins/int.rs
Outdated
| /// >>> int('0b100', base=0) | ||
| /// 4 | ||
| #[pyclass(module = false, name = "int")] | ||
| #[pyclass(module = false, name = "int", itemsize = 4)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we have a better way than adding manual itemsize to type definition.
vm/src/builtins/object.rs
Outdated
| fn sizeof(zelf: PyObjectRef) -> usize { | ||
| zelf.class().slots.basicsize | ||
| let mut res: usize = 0; | ||
| let isize: usize = zelf.class().slots.itemsize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let isize: usize = zelf.class().slots.itemsize; | |
| let itemsize: usize = zelf.class().slots.itemsize; |
to avoid reusing primitive type name isize
vm/src/object/core.rs
Outdated
|
|
||
| impl<T> PyInner<T> { | ||
| #[inline(always)] | ||
| pub fn ob_size(&self) -> usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In RustPython, we don't use the prefix ob_.
| pub fn ob_size(&self) -> usize { | |
| pub fn size(&self) -> usize { |
vm/src/object/core.rs
Outdated
| } | ||
|
|
||
| #[inline(always)] | ||
| pub fn set_ob_size(&self, size: usize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this being called?
I agree about this suggestion because it doesn't need to implement same struct model with CPython. I'll implement the idea again by referring to other traits. If I implement it, some methods or fields will be removed or replaced. (e.g. |
b05a34d to
73e90a0
Compare
|
I wanted RustPython's implementations return same value with CPython implementation. I'll close this pull request but it may be because of my passion and my code skill. |
|
oh.. I thought this is going to the right way. what was not fitting? |
This pull request may close #356.
This pull request implements
object.__sizeof__fully, except for setting all types'ob_size. It introducesPyVarObject.ob_sizefield andtp_itemsizeslot. Also it provides a way to setupitemsizeslot withpyclassmacro.ob_sizefield is used to calculate variable object's size withitemsize(ob_size * itemsize + basicsize).Because this pull request modified the core part like (
PyInnerandPyObjectRef) and I felt the next work can be more bigger, I cut this part and request a review for this pull request.Related codes:
object.__sizeof__impl: https://github.com/python/cpython/blob/e5d8dbdd304935dbd0631ee9605efb501332f792/Objects/typeobject.c#L5520-L5533PyVarObject: https://github.com/python/cpython/blob/fa2b8b75eb2b8a0193d587e02b488a73579118fc/Include/object.h#L111-L114The next work will be to call
set_ob_size.