-
Notifications
You must be signed in to change notification settings - Fork 65
starknet_api: using internal implementation of sizeof #8002
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
starknet_api: using internal implementation of sizeof #8002
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
8c969a7
to
a5a2717
Compare
a5a2717
to
697cc49
Compare
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.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5)
crates/sizeof/sizeof_internal/src/lib.rs
line 56 at r1 (raw file):
impl<T: SizeOf> SizeOf for Vec<T> { fn dynamic_size(&self) -> usize { self.iter().map(|x| x.size_bytes()).sum::<usize>()
Shouldn't it be also base on the capacity like in String
?
using iter()
counts only the used length and not the actual allocated memory.
Consider:
Suggestion:
let used = self.iter().map(|x| x.size_bytes()).sum::<usize>();
let excess = (self.capacity() - self.len()) * std::mem::size_of::<T>();
used + excess
crates/sizeof/sizeof_internal/src/lib.rs
line 85 at r1 (raw file):
vec![1_u8, 2_u8, 3_u8].size_bytes(), std::mem::size_of::<Vec<u8>>() + std::mem::size_of::<u8>() * 3 );
Can you test a bit more complicated types, for example:
Vec<String>
, Arc<Vec<String>>
?
Also to test a vec that has capacity > len
, you can use Vec::with_capacity()
697cc49
to
5b5a6d3
Compare
Artifacts upload workflows: |
Benchmark movements: No major performance changes detected. |
5b5a6d3
to
4cb9d45
Compare
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.
Made the said changes:
- using capacity for vec
- added tests for complex types
In addition, corrected an error: only Box, Rc, and Arc implement Deref, so they have a corresponding implementation for the trait.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @dafnamatsry)
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.
Reviewed 2 of 3 files at r2, all commit messages.
Reviewable status: 3 of 4 files reviewed, 5 unresolved discussions (waiting on @alonh5 and @victorkstarkware)
crates/sizeof/sizeof_internal/src/lib.rs
line 21 at r2 (raw file):
/// itself. Currently, this trait is only implemented for the following `Deref` types: /// `Box<T>, Rc<T>, Arc<T>`. If your `Deref` type is not on this list, refrain from using /// this method.
Suggestion:
/// `WARNING`: It is `DANGEROUS` to use this function on types that implement `Deref`, since
/// `Deref coercion` will neglect counting the stack size taken by the original type
/// itself. Currently, this trait is only implemented for the following `Deref` types:
/// `Box<T>, Rc<T>, Arc<T>`. If your `Deref` type is not on this list, refrain from using
/// this method.
crates/sizeof/sizeof_internal/src/lib.rs
line 30 at r2 (raw file):
/// itself. Currently, this trait is only implemented for the following `Deref` types: /// `Box<T>, Rc<T>, Arc<T>`. If your `Deref` type is not on this list, refrain from using /// this method.
Suggestion:
/// `WARNING`: It is `DANGEROUS` to use this function on types that implement `Deref`, since
/// `Deref coercion` will neglect counting the stack size taken by the original type
/// itself. Currently, this trait is only implemented for the following `Deref` types:
/// `Box<T>, Rc<T>, Arc<T>`. If your `Deref` type is not on this list, refrain from using
/// this method.
crates/sizeof/sizeof_internal/src/lib.rs
line 141 at r2 (raw file):
+ vec_string[1].size_bytes() + (vec_string.capacity() - vec_string.len()) * std::mem::size_of::<String>() + std::mem::size_of::<Vec<String>>()
Consider extracting this to a helper function vec_string_size(...)
, as it repeats itself many time in the tests.
Code quote:
vec_string[0].size_bytes()
+ vec_string[1].size_bytes()
+ (vec_string.capacity() - vec_string.len()) * std::mem::size_of::<String>()
+ std::mem::size_of::<Vec<String>>()
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.
Reviewable status: 3 of 4 files reviewed, 5 unresolved discussions (waiting on @alonh5 and @dafnamatsry)
crates/sizeof/sizeof_internal/src/lib.rs
line 21 at r2 (raw file):
/// itself. Currently, this trait is only implemented for the following `Deref` types: /// `Box<T>, Rc<T>, Arc<T>`. If your `Deref` type is not on this list, refrain from using /// this method.
these spaces are the output of rust_fmt.sh, so i keep them as is
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.
Reviewable status: 3 of 4 files reviewed, 5 unresolved discussions (waiting on @alonh5 and @dafnamatsry)
crates/sizeof/sizeof_internal/src/lib.rs
line 56 at r1 (raw file):
Previously, dafnamatsry wrote…
Shouldn't it be also base on the capacity like in
String
?
usingiter()
counts only the used length and not the actual allocated memory.Consider:
fixed
crates/sizeof/sizeof_internal/src/lib.rs
line 85 at r1 (raw file):
Previously, dafnamatsry wrote…
Can you test a bit more complicated types, for example:
Vec<String>
,Arc<Vec<String>>
?Also to test a vec that has
capacity > len
, you can useVec::with_capacity()
fixed
crates/sizeof/sizeof_internal/src/lib.rs
line 30 at r2 (raw file):
/// itself. Currently, this trait is only implemented for the following `Deref` types: /// `Box<T>, Rc<T>, Arc<T>`. If your `Deref` type is not on this list, refrain from using /// this method.
this is the result of rust_fmt.sh so i keep as is
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.
Reviewable status: 3 of 4 files reviewed, 5 unresolved discussions (waiting on @alonh5 and @dafnamatsry)
crates/sizeof/sizeof_internal/src/lib.rs
line 141 at r2 (raw file):
Previously, dafnamatsry wrote…
Consider extracting this to a helper function
vec_string_size(...)
, as it repeats itself many time in the tests.
i prefer to keep as is, to emphasize the difference between capacity and len, more readable for someone only reading the test not needing to look into another helper function
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.
Reviewed 1 of 3 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @alonh5)
TL;DR
Added a new
sizeof_internal
crate to compute the size of types in memory.What changed?
sizeof_internal
with aSizeOf
trait that provides two methods:dynamic_size()
: returns the heap size of a type in bytessize_bytes()
: returns the total size (stack + heap) of a type in bytesString
,Vec<T>
, and pointer types likeBox<T>
,Rc<T>
, etc.default_primitive_sizeof!
anddefault_pointer_sizeof!
to simplify trait implementationFelt
How to test?
Run the tests in the crate:
cargo test -p sizeof_internal
The tests verify size calculations for primitive types, strings, vectors, and Felt values.
Why make this change?
This should replace the
size-of
crate currently in use down the line, as it uses code that will be deprecated in Rust 1.88. This specific PR includes just the trait definition and implementations for primitive types.