Skip to content
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

implement get_size::GetSize for SmallVec, so we can know how much memory we're using? #331

Closed
amandasaurus opened this issue Jan 13, 2024 · 9 comments

Comments

@amandasaurus
Copy link
Contributor

I have been using smallvec successfully, and it's been a great memory saver. I also use the get_size library to report how much (heap) memory my data structures use. This doesn't work for SmallVec's, or data stucutres which include them.

Is it possible to add support to GetSize to smallvec? Maybe behind a feature flag? get_size allows one to implement it for external types, do you have any tips on how I could implement GetSize for a SmallVec?

@mbrubeck
Copy link
Collaborator

Is it possible to add support to GetSize to smallvec? Maybe behind a feature flag?

Sure! The implementation should be pretty straightforward, similar to this template that is used for collections like Vec.

For SmallVec, the get_heap_size method should start with 0 when the buffer is “inline,” or the buffer size (capacity × stack size of each item) when it is “spilled.” Then it should iterate through the vector and add the heap size of each element.

@amandasaurus
Copy link
Contributor Author

amandasaurus commented Jan 15, 2024 via email

@mbrubeck
Copy link
Collaborator

Is it possible to add an impl for GetSize to small_vec directly, hidden behind a feature flag?

Yes, please feel free to submit a pull request. You may want to do this on the v1 branch if you want to use it right away.

@mbrubeck
Copy link
Collaborator

Some more details:

  1. Start by forking this repository and checking out the v1 branch.
  2. Add get-size as a dependency in Cargo.toml with optional = true.
  3. Add an impl<A: Array> get_size::GetSize for SmallVec<A> { ... } behind a feature = "get_size" gate, similar to this one for feature = "serde".

Let me know if you have any questions along the way!

amandasaurus added a commit to amandasaurus/rust-smallvec that referenced this issue Jan 16, 2024
Shows memory usage of the struct

cf. issue servo#331
@amandasaurus
Copy link
Contributor Author

amandasaurus commented Jan 16, 2024 via email

@mbrubeck
Copy link
Collaborator

How come changing the size of the array doesn't linearly affect stack size

This is expected. SmallVec has a minimum stack size because it must be able store a length, capacity, and pointer, for the “spilled” case where it is basically a (usize, usize, *const T). On a 64-bit platform, this takes up (3 * 64 bits) = (3 * 8 bytes) = 24 bytes.

And because of alignment, the size can only grow in multiples of 64 bits (8 bytes).

But if a SmallVec is inline, then the heap size should be zero, right? 🤔 Have I done something wrong?

This code:

        for v in self.iter() {
            total += GetSize::get_size(v);
        }

should call get_heap_size(v) instead of get_size(v).

amandasaurus added a commit to amandasaurus/rust-smallvec that referenced this issue Jan 17, 2024
Shows memory usage of the struct

cf. issue servo#331
amandasaurus added a commit to amandasaurus/rust-smallvec that referenced this issue Jan 17, 2024
Shows memory usage of the struct

cf. issue servo#331
@amandasaurus
Copy link
Contributor Author

oh wow! I've made the changes you recommended, and from the tests this is exactly the sort of numbers I expect:

    let mut a: SmallVec<[i32; 2]> = smallvec![];
    assert!(!a.spilled());
    assert_eq!(a.get_size(), 24);
    assert_eq!(a.get_heap_size(), 0);

    a.push(0);
    assert_eq!(a.len(), 1);
    assert!(!a.spilled());
    assert_eq!(a.get_size(), 24);
    assert_eq!(a.get_heap_size(), 0);

    a.push(1);
    assert_eq!(a.len(), 2);
    assert!(!a.spilled());
    assert_eq!(a.get_size(), 24);
    assert_eq!(a.get_heap_size(), 0);

    a.push(2);
    assert_eq!(a.len(), 3);
    assert!(a.spilled());
    assert_eq!(a.get_size(), 40);
    assert_eq!(a.get_heap_size(), 16);

    a.push(3);
    assert_eq!(a.len(), 4);
    assert!(a.spilled());
    assert_eq!(a.get_size(), 40);
    assert_eq!(a.get_heap_size(), 16);

    a.push(4);
    assert_eq!(a.len(), 5);
    assert!(a.spilled());
    assert_eq!(a.get_size(), 56);
    assert_eq!(a.get_heap_size(), 32);

The total size grows a little bit, and the heap size stays 0 while it's not spilled. 🙂

Would you be willing to merge this feature into the main codebase?

@mbrubeck
Copy link
Collaborator

Would you be willing to merge this feature into the main codebase?

Yes, absolutely!

amandasaurus added a commit to amandasaurus/rust-smallvec that referenced this issue Jan 17, 2024
Shows memory usage of the struct

cf. issue servo#331
amandasaurus added a commit to amandasaurus/rust-smallvec that referenced this issue Jan 18, 2024
Shows memory usage of the struct

cf. issue servo#331
amandasaurus added a commit to amandasaurus/rust-smallvec that referenced this issue Jan 18, 2024
Shows memory usage of the struct

cf. issue servo#331
github-merge-queue bot pushed a commit that referenced this issue Jan 18, 2024
Shows memory usage of the struct

cf. issue #331
mbrubeck pushed a commit that referenced this issue Jan 18, 2024
Shows memory usage of the struct

cf. issue #331
@mbrubeck
Copy link
Collaborator

Implemented in #335, #336.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants