A compiling copy of the Servo HeapSizeOf trait #1
Conversation
(technically works on stable, for crates that want that, though it actually relies on jemalloc, so it's not really stable)
|
I'll set bors up for this repo. Please don't manually merge anything. |
|
Danke. |
|
Bors is online. Carry on! |
| // platforms `JEMALLOC_USABLE_SIZE_CONST` is `const` and on some it is empty. But in practice | ||
| // this function doesn't modify the contents of the block that `ptr` points to, so we use | ||
| // `*const c_void` here. | ||
| fn je_malloc_usable_size(ptr: *const c_void) -> size_t; |
There was a problem hiding this comment.
@nnethercote Could std::heap::usable_size(size: usize, align: usize) -> usize be used instead of this? It’s based on je_nallocx. (Don’t block this PR on this, it could be a follow-up.)
There was a problem hiding this comment.
How would that work? je_malloc_usable_size takes a pointer and returns a size. usable_size takes a size (and an alignment) and returns a size.
There was a problem hiding this comment.
Everywhere a pointer is given to heap_size_of(), a size from std::mem::size_of or Vec::capacity (and alignment from std::mem::align_of) could be given instead. Would that be equivalent?
There was a problem hiding this comment.
I see. I strongly recommend against that.
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting discusses this some (see "Two ways to measure"). In short, measuring actual heap blocks is better because it's more verifiable:
- Size computations are error-prone.
- Size computations are often more complex.
- If you call
je_malloc_usable_sizeon something that's not a heap pointer, you'll crash. Which is a good thing, because it's clear you've done something wrong.
For the first two points, the HashMap code lower in this patch is a good example. It uses self.capacity() * (size_of::<V>() + size_of::<K>()) to compute the size, which is more complex than calling je_malloc_usable_size, and it's also unclear if it's accurate -- are hash values also stored? Is there any padding between the K and V values? Etc.
(In fact, there's a similar sort of comment just 20 lines lower down.)
There was a problem hiding this comment.
I see, thanks.
(The HashMap impl is indeed not accurate: servo/servo#6908 , but we can’t access the pointer without making assumptions about the struct layout of private fields, which seems bad.)
There was a problem hiding this comment.
Yes. For HeapSizeOf to be used ubiquitously, it'll eventually need to become part of Rust, as the comment above LinkedList2 says.
|
@bors-servo: r+ |
|
📌 Commit f45e9bd has been approved by |
A compiling copy of the Servo HeapSizeOf trait (technically works on stable, for crates that want that, though it actually relies on jemalloc, so it's not really stable)
|
☀️ Test successful - travis |
(technically works on stable, for crates that want that, though it
actually relies on jemalloc, so it's not really stable)