1409255 - Replace all uses of the `heapsize` crate with `malloc_size_of`

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Nicholas Nethercote [inactive]]

Servo currently uses the external `heapsize` crate, but Stylo/Gecko use the internal `malloc_size_of` crate. `malloc_size_of` is better -- it handles various cases that `heapsize` does not.

There are two possible ways to remove the divergence.

- Switch all of our uses of `heapsize` to `malloc_size_of`.

- Change `heapsize` to be more like `malloc_size_of`, and switch all of our uses of `malloc_size_of` to use the new `heapsize`.

I elected to do the former, primarily because I view this code as unstable and so not really appropriate as an external crate. Keeping it internal also makes it easier to update. However, it does mean that external crates cannot depend on `malloc_size_of`, which is a mild inconvenience but not a show-stopper.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Replace all uses of the `heapsize` crate with `malloc_size_of`

Servo currently uses `heapsize`, but Stylo/Gecko use `malloc_size_of`.
`malloc_size_of` is better -- it handles various cases that `heapsize` does not
-- so this patch changes Servo to use `malloc_size_of`.

This patch makes the following changes to the `malloc_size_of` crate.

- Adds `MallocSizeOf` trait implementations for numerous types, some built-in
  (e.g. `VecDeque`), some external and Servo-only (e.g. `string_cache`).

- Makes `enclosing_size_of_op` optional, because vanilla jemalloc doesn't
  support that operation.

- For `HashSet`/`HashMap`, falls back to a computed estimate when
  `enclosing_size_of_op` isn't available.

- Adds an extern "C" `malloc_size_of` function that does the actual heap
  measurement; this is based on the same functions from the `heapsize` crate.

This patch makes the following changes elsewhere.

- Converts all the uses of `heapsize` to instead use `malloc_size_of`.

- Disables the "heapsize"/"heap_size" feature for the external crates that
  provide it.

- Removes the `HeapSizeOf` implementation from `hashglobe`.

- Adds `ignore` annotations to a few `Rc`/`Arc`, because `malloc_size_of`
  doesn't derive those types, unlike `heapsize`.

MozReview-Commit-ID: 6qFkP4XVsN

Comment 2

[Simon Sapin (:SimonSapin)]

I strongly agree with the easier to update part. The approach of making heapsize an (optional) dependency of many crates like url causes it to be part of their public API. Then, updating to a semver-incompatible version of heapsize requires either a new incompatible version of url (with a cascading effect) or hacks to support multiple versions that don’t always play well with Cargo’s dependency resolution.

The downside of having trait impls for many external types in the crate defining the traits (malloc_size_of) is that we can’t use #[derive]. But that might be OK with the pattern shown by Felix in http://pnkfx.org/presentations/rustfest-zurich-2017.html#/4/10 :

  impl_malloc_size_of_for!(enum Option<T> { Some(T), None });

expanding to an impl like #[derive(MallocSizeOf)] would. This requires duplicating the type definition, but that’s much less repetition than manual impls. And we can build something that looks like a foo!() procedural macro on stable Rust with https://docs.rs/procedural-masquerade/

Comment 3

[Simon Sapin (:SimonSapin)]

Oh I didn’t realize you already had a patch when I started typing my previous comment. Never mind about that procedural macro, then. We can add it later if that proves useful.

Comment 4

[Nicholas Nethercote [inactive]]

https://github.com/servo/servo/pull/18938 is the Servo PR.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1409255 - Replace all uses of the `heapsize` crate with `malloc_size_of`. .

Review commit: https://reviewboard.mozilla.org/r/190582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190582/

Comment 6

[Simon Sapin (:SimonSapin)]

Comment on attachment 8919668 [details]
Bug 1409255 - Replace all uses of the `heapsize` crate with `malloc_size_of`. .

https://reviewboard.mozilla.org/r/190582/#review195824

The bot synchronizing the servo repostitory into autoland will do this automatically.

Comment 7

[Nicholas Nethercote [inactive]]

> The bot synchronizing the servo repostitory into autoland will do this
> automatically.

This has been done, and the Servo PR has been merged, so this bug can be closed.