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

Implement MallocSizeOf for Atom #289

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Implement MallocSizeOf for Atom #289

wants to merge 1 commit into from

Conversation

nicoburns
Copy link

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 the string-cache crate.

@@ -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
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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

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.

3 participants