-
Notifications
You must be signed in to change notification settings - Fork 78
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 MallocSizeOf
for Atom
#289
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nico Burns <[email protected]>
@@ -82,6 +82,15 @@ pub struct Atom<Static> { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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_of
pulls in a large number of dependencies.This PR adds the implementation of
MallocSizeOf
to thestring-cache
crate.