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

Bug 1409255 - Replace all uses of the heapsize crate with malloc_size_of #18920

Closed
wants to merge 1 commit into from

Conversation

SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Oct 17, 2017

Patch from Nicholas Nethercote on https://bugzilla.mozilla.org/show_bug.cgi?id=1409255


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.


This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: ports/geckolib/glue.rs, components/style/values/generics/flex.rs, components/style/stylesheet_set.rs, components/style/rule_tree/mod.rs, components/style/values/specified/length.rs and 86 more
  • @jgraham: components/webdriver_server/Cargo.toml
  • @canaltinova: components/style/values/generics/flex.rs, components/style/stylesheet_set.rs, components/style/rule_tree/mod.rs, components/style/values/specified/length.rs, components/style/stylesheets/origin.rs and 83 more
  • @emilio: ports/geckolib/glue.rs, components/script/dom/webglframebuffer.rs, components/style/values/generics/flex.rs, components/style/stylesheet_set.rs, components/style/rule_tree/mod.rs and 97 more
  • @fitzgen: components/script/dom/request.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/webglframebuffer.rs, components/script/dom/crypto.rs, components/devtools_traits/Cargo.toml and 127 more
  • @KiChjang: components/script/dom/request.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/webglframebuffer.rs, components/script/dom/crypto.rs, components/script/dom/vrdisplaycapabilities.rs and 136 more
  • @asajeffrey: components/webdriver_server/Cargo.toml
  • @paulrouget: components/servo_arc/Cargo.toml, components/servo_arc/lib.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 17, 2017
@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@jdm
Copy link
Member

jdm commented Oct 17, 2017

./components/malloc_size_of/lib.rs:153: Line is longer than 120 characters

@jdm jdm added the S-fails-tidy `./mach test-tidy` reported errors. label Oct 17, 2017
@SimonSapin
Copy link
Member Author

CC @nnethercote

I split the long line to fix tidy.

Nit: size_of_is_0! out of context sounds like std::mem::size_of(). Should that macro be renamed malloc_size_of_is_0!?

@bors-servo try


Reviewed 269 of 269 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


components/malloc_size_of/lib.rs, line 345 at r1 (raw file):

    fn shallow_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
        if let Some(elem) = self.get(0) {
            // This assumes that element 0 is the start of the heap block.

Element 0 is not necessarily at the start. VecDeque is a ring buffer internally.


components/script/dom/eventtarget.rs, line 121 at r1 (raw file):

impl MallocSizeOf for EventListenerType {
    fn size_of(&self, _ops: &mut MallocSizeOfOps) -> usize {
        // FIXME: Rc<T> isn't MallocSizeOf and we can't ignore it due to #6870 and #6871

This doesn’t apply anymore, does it? Shouldn’t this be a #[derive] with ignore attributes where appropriate?


Comments from Reviewable

@bors-servo
Copy link
Contributor

⌛ Trying commit 4c7e747 with merge 39d0c34...

bors-servo pushed a commit that referenced this pull request Oct 17, 2017
Bug 1409255 - Replace all uses of the heapsize crate with malloc_size_of

Patch from Nicholas Nethercote on https://bugzilla.mozilla.org/show_bug.cgi?id=1409255

----

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`.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18920)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #18924) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Oct 17, 2017
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #18704) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Oct 17, 2017
@SimonSapin
Copy link
Member Author

Looks like (in AppVeyor) this is missing some imports on Windows:

error[E0425]: cannot find function `GetProcessHeap` in this scope
   --> C:\projects\servo\components\malloc_size_of\lib.rs:163:16
    |
163 |     let heap = GetProcessHeap();
    |                ^^^^^^^^^^^^^^ not found in this scope
error[E0425]: cannot find function `HeapValidate` in this scope
   --> C:\projects\servo\components\malloc_size_of\lib.rs:165:8
    |
165 |     if HeapValidate(heap, 0, ptr) == 0 {
    |        ^^^^^^^^^^^^ not found in this scope
error[E0425]: cannot find function `HeapSize` in this scope
   --> C:\projects\servo\components\malloc_size_of\lib.rs:169:5
    |
169 |     HeapSize(heap, 0, ptr) as usize
    |     ^^^^^^^^ not found in this scope
error: aborting due to 3 previous errors
error: Could not compile `malloc_size_of`.

heapsize has this dependency:

[target.'cfg(windows)'.dependencies]
kernel32-sys = "0.2.1"

@nnethercote
Copy link
Contributor

Nit: size_of_is_0! out of context sounds like std::mem::size_of(). Should that macro be renamed malloc_size_of_is_0!?

I filed #18932 to fix that.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #18932) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Oct 18, 2017
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`.
@nnethercote
Copy link
Contributor

#18938 has the updated version of this change.

@SimonSapin
Copy link
Member Author

Re-opening temporarily to get Reviewable to show a diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-review There is new code that needs to be reviewed. S-fails-tidy `./mach test-tidy` reported errors. S-needs-rebase There are merge conflict errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants