Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upUse the jemallocator crate from crates.io instead of alloc_jemalloc #18836
Conversation
|
r? @nnethercote |
|
@bors-servo try |
|
|
|
|
| // Before we request the measurement of interest, we first send an "epoch" | ||
| // request. Without that jemalloc gives cached statistics(!) which can be | ||
| // highly inaccurate. | ||
| if let Err(_) = ::jemallocator::mallctl_set(b"epoch\0", 0_u64) { |
This comment has been minimized.
This comment has been minimized.
|
Just curious, what's the benefit of this? |
|
Sorry, I don't think I'm a good person to review this. I don't know much about all these Rust/jemalloc plumbing details. One thing I will note is that you've explained the "what" for this patch, but not the "why". Why is this change a good thing? |
|
Sorry, I should have said more in the original message. @nnethercote I asked you since you wrote the initial version of memory profiling. In particular it’s the modified As to why this change:
Now, I’ve just realized that this PR probably breaks the |
|
I know a lot about heapsize but nothing about jemalloc/Rust integration, The
In what way? Right now I'm working on removing heapsize usage in Servo in favour of malloc_size_of... |
Oh that’s right. Good. Then this PR should wait for that. |
This compiles a new copy of jemalloc in addition to the one brought by the Rust standard library, but: * Only one of those ends up being linked * The increase in compile times is small (20 seconds on my laptop) This commit also adds an use of `#[global_allocator]` to select `jemallocator` as the default. `extern crate alloc_jemalloc;` used to decide the default, but it hasn’t since the `#[global_allocator]` attribute was introduced. `mallctl_set(b"epoch\0", 0_u64)` ends up calling `mallctl` with null pointers for the "old" value and its length, which isn’t the same as what we were doing before (passing the same pointer to both "old" and "new" parameters). My understanding is that this works too, since the important part is passing a new value: http://jemalloc.net/jemalloc.3.html#epoch > epoch (uint64_t) rw > > If a value is passed in, refresh the data from which > the mallctl*() functions report values, and increment the epoch. > Return the current epoch. This is useful for detecting whether > another thread caused a refresh.
|
@bors-servo try (Do we run the profiling code on CI?) |
Use the jemallocator crate from crates.io instead of alloc_jemalloc This compiles a new copy of jemalloc in addition to the one brought by the Rust standard library, but: * Only one of those ends up being linked * The increase in compile times is small (20 seconds on my laptop) This commit also adds an use of `#[global_allocator]` to select `jemallocator` as the default. `extern crate alloc_jemalloc;` used to decide the default, but it hasn’t since the `#[global_allocator]` attribute was introduced. `mallctl_set(b"epoch\0", 0_u64)` ends up calling `mallctl` with null pointers for the "old" value and its length, which isn’t the same as what we were doing before (passing the same pointer to both "old" and "new" parameters). My understanding is that this works too, since the important part is passing a new value: http://jemalloc.net/jemalloc.3.html#epoch > epoch (uint64_t) rw > > If a value is passed in, refresh the data from which the mallctl*() functions report values, and increment the epoch. Return the current epoch. This is useful for detecting whether another thread caused a refresh. <!-- 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/18836) <!-- Reviewable:end -->
|
|
|
As expected: |
|
|
|
Replaced by #18944. |
SimonSapin commentedOct 11, 2017
•
edited
This compiles a new copy of jemalloc in addition to the one brought by the Rust standard library, but:
This commit also adds an use of
#[global_allocator]to selectjemallocatoras the default.extern crate alloc_jemalloc;used to decide the default, but it hasn’t since the#[global_allocator]attribute was introduced.mallctl_set(b"epoch\0", 0_u64)ends up callingmallctlwith null pointers for the "old" value and its length, which isn’t the same as what we were doing before (passing the same pointer to both "old" and "new" parameters). My understanding is that this works too, since the important part is passing a new value:http://jemalloc.net/jemalloc.3.html#epoch
This change is