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

parity-util-mem: optimize MallocSizeOf for flat collections #398

Merged
merged 4 commits into from
Jun 17, 2020

Conversation

ordian
Copy link
Member

@ordian ordian commented Jun 12, 2020

During investigation of slowness in MemoryDB::size_of, I've noticed that we iterate over collections when calculating size even if values have size_of of 0, e.g. for HashMap<u32, u8>.

To avoid iterations in this case, a new static method fn size_of_is_zero() is added to MallocSizeOf.
I've considered another approach with a marker trait, but it didn't work due to the lack of specialization.

I've noticed that the compiler is smart enough to optimize this for Vec's already, but not for other collections: https://rust.godbolt.org/z/2jNo4c.

@ordian ordian requested a review from cheme June 12, 2020 11:34
Copy link
Collaborator

@cheme cheme left a comment

Choose a reason for hiding this comment

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

I did not think of doing it this way, but it LGTM.
A possible variation would be to do :
fn constant_size() -> Option<usize>, which would allow other optimizations if the collection got a efficient method to return its size (usually).

@ordian
Copy link
Member Author

ordian commented Jun 12, 2020

A possible variation would be to do :
fn constant_size() -> Option, which would allow other optimizations if the collection got a efficient method to return its size (usually).

I've thought about this as well, but unless estimate-heapsize feature is used, there is no way to know if a type actually allocates a fixed amount of bytes (even with Box<[u8; 32]> you never know it's 32 bytes with the allocator, that's why our tests have >= and not ==).

@ordian
Copy link
Member Author

ordian commented Jun 17, 2020

fn constant_size() -> Option, which would allow other optimizations if the collection got a efficient method to return its size (usually).

Implemented this because why not :)

@cheme
Copy link
Collaborator

cheme commented Jun 17, 2020

just thought it could be an associated constant (locking trait object use case, but I am not sure if using mallocsizeof as trait object could be useful).

@ordian
Copy link
Member Author

ordian commented Jun 17, 2020

just thought it could be an associated constant (locking trait object use case, but I am not sure if using mallocsizeof as trait object could be useful).

Is there any benefit of doing that? Both cases are optimized at compile time in release mode, so if there is no benefit, I'd prefer to keep as is and keep the ability to create Box<dyn MallocSizeOf> or &dyn MallocSizeOf.

@cheme
Copy link
Collaborator

cheme commented Jun 17, 2020

Not that much, gives stronger guaranties, so we are sure there will be no bad implementation of the method.

@ordian
Copy link
Member Author

ordian commented Jun 17, 2020

Not that much, gives stronger guaranties, so we are sure there will be no bad implementation of the method.

not sure I follow, could you elaborate how can no bad impl be guaranteed with the assoc const?

@cheme
Copy link
Collaborator

cheme commented Jun 17, 2020

The associated constant strictly needs to be a constant.
On the other hand when implementing the function from a trait we do not need to return a constant value, someone can do anything (querying global mutable variable, calling ffi, rand...).
So using the associative constant is more restrictive.

@ordian
Copy link
Member Author

ordian commented Jun 17, 2020

The associated constant strictly needs to be a constant.
On the other hand when implementing the function from a trait we do not need to return a constant value, someone can do anything (querying global mutable variable, calling ffi, rand...).
So using the associative constant is more restrictive.

I see, that's a contrived example though, an easier way to shoot oneself in the foot is to use Some(usize::MAX). And I don't expect constant_size to be implemented at all, only via malloc_size_is_0! macro.

@ordian ordian merged commit f3023c9 into master Jun 17, 2020
@ordian ordian deleted the ao-size-of-is-zero branch June 17, 2020 21:00
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

Successfully merging this pull request may close these issues.

None yet

3 participants