Conversation
Signed-off-by: Nico Burns <nico@nicoburns.com>
| phantom: PhantomData<Static>, | ||
| } | ||
|
|
||
| // This isn't really correct as the Atoms can technically take up space. But I guess it's ok |
There was a problem hiding this comment.
We have the ability to use the internals of this crate now, so I would prefer to implement a real measurement here.
There was a problem hiding this comment.
The fundamental problem here is not access to internals, it's shared ownership. For a type like Atom (or Arc, Rc) that where ownership of the allocation is shared you risk overcounting if you implement MallocSizeOf properly.
Having said that, the solution to this for Arc is to not implement it and use the ignore_malloc_size_of attribute. So perhaps that is also the best solution here.
There was a problem hiding this comment.
There is a mechanism for Arc that avoids counting the same memory location twice. Perhaps we can reuse that here.
There was a problem hiding this comment.
So there is the ConditionalMallocSizeOf trait which keeps track of which pointers have been seen and then ignores duplicates (we'd need to do the tracking ourselves). However, I think this would be tricky for Atom as Atom's are shared across more than just style, so we'd need to keep global track of all pointers (shared between the memory measurement of stylo and the memory measurement of other components like parsing, script, layout etc). Furthermore, which component the memory counted for would be determined by which components memory we measured first.
I think it might make more sense just to ignore Atoms when measuring style, layout, script, etc. And count them as a separate category. This is effectively how it works for Arc too: you manually decide on an owner for each instance and then only count that instance. And I think it makes sense for the owner of Atom's to be the Atom set itself?
Also worth mentioning that Gecko have a separate Atom type for which MallocStyleOf returns 0. So we'd have to negotiate with Gecko if we want our Atom type to work differently to ensure they're not double-counting memory.
As such, my feeling is that the most viable strategies are:
- Implement this trait as in this PR and accept its a kludge.
- OR add ignore attributes to usage of Atom in stylo
jdm
left a comment
There was a problem hiding this comment.
Accepting the kludge seems fine to me.
Now that malloc_size_of is published to crates.io we are moving the trait implementations of MallocSizeOf and related traits into the crates that define the types. This will avoid the situation where depending on
malloc_size_ofpulls in a large number of dependencies.This PR adds the implementation of
MallocSizeOfto thestring-cachecrate.