Skip to content
This repository has been archived by the owner on Jul 10, 2023. It is now read-only.

Remove jemalloc dependency. #84

Closed
wants to merge 1 commit into from
Closed

Remove jemalloc dependency. #84

wants to merge 1 commit into from

Conversation

jdm
Copy link
Member

@jdm jdm commented Jul 3, 2017

Add optional jemalloc feature to expose jemalloc-based heap measurement
API that can be passed as the argument to heap_size_of and
heap_size_of_children.

Fixes #80.


This change is Reviewable

@nnethercote
Copy link

Thanks, @jdm! Looks pretty good. My only concern is about names. Given that the type is called HeapSizeOfFn, I would prefer if the argument name for the passed-in function is heap_size_of instead of heap_size. That will require renaming the existing heap_size_of as something else like do_heap_size_of. (That's what I did with the MallocSizeOf in Servo: malloc_size_of and do_malloc_size_of.)

The whole heap_size_of/do_heap_size_of split is unfortunate. I wish it wasn't necessary. In fact, I can't even remember why it is necessary. In Gecko the caller is always responsible for null-checking first. Maybe having the no-allocation check within the function makes the auto-deriving simpler?

@jdm
Copy link
Member Author

jdm commented Jul 3, 2017

Yeah, encoding the null-checking logic (which is more complicated than just a null pointer comparison) in every caller would not be a good choice.

Add optional jemalloc feature to expose jemalloc-based heap measurement
API that can be passed as the argument to heap_size_of and
heap_size_of_children.
@bors-servo
Copy link

☔ The latest upstream changes (presumably #85) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
Copy link

@jdm, is this ready to land? It would be helpful to get Servo updated to use this new API.

@SimonSapin
Copy link
Member

If we’re making breaking changes anyway, let’s also remove the incorrect HeapSizeOf for Arc<T> and HeapSizeOf for Vec<Rc<T>> impls. CC #37

@nnethercote
Copy link

Hmm, I'm having second thoughts about this. This patch makes HeapSizeOf more like Stylo's MallocSizeOf, which is good. But MallocSizeOf is a work in progress. E.g. I'm right now working on some additional stuff that would allow Rcs and Arcs to be measured. Once that design is settled, we could update HeapSizeOf to match, and then eventually replace MallocSizeOf with HeapSizeOf.

Sound reasonable, @jdm? Apologies for my change of heart, but I think it will save work in the long run.

@jdm
Copy link
Member Author

jdm commented Jul 19, 2017

Sure.

@jdm jdm closed this Jul 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants