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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ jobs:
run: |
cargo build --no-default-features
cargo build
cargo build --features malloc_size_of
- uses: actions-rs/cargo@v1
with:
command: test
Expand Down
3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "string_cache"
version = "0.8.8" # Also update README.md when making a semver-breaking change
version = "0.8.9" # Also update README.md when making a semver-breaking change
authors = ["The Servo Project Developers"]
description = "A string interning library for Rust, developed as part of the Servo project."
license = "MIT OR Apache-2.0"
Expand All @@ -25,6 +25,7 @@ default = ["serde_support"]
[dependencies]
precomputed-hash = "0.1"
serde = { version = "1", optional = true }
malloc_size_of = { version = "0.1", default-features = false, optional = true }
phf_shared = "0.11"
new_debug_unreachable = "1.0.2"
parking_lot = "0.12"
Expand Down
9 changes: 9 additions & 0 deletions src/atom.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

// as it is possible to measure the size of the atom set separately/
#[cfg(feature = "malloc_size_of")]
impl<Static: StaticAtomSet> malloc_size_of::MallocSizeOf for Atom<Static> {
fn size_of(&self, _ops: &mut malloc_size_of::MallocSizeOfOps) -> usize {
0
}
}

// FIXME: bound removed from the struct definition before of this error for pack_static:
// "error[E0723]: trait bounds other than `Sized` on const fn parameters are unstable"
// https://github.com/rust-lang/rust/issues/57563
Expand Down
Loading