Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upRemove the `alloc_jemalloc` crate #55238
Conversation
rust-highfive
assigned
estebank
Oct 21, 2018
This comment has been minimized.
This comment has been minimized.
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
label
Oct 21, 2018
alexcrichton
referenced this pull request
Oct 21, 2018
Closed
Switch the default global allocator to System, remove alloc_jemalloc, use jemallocator in rustc #36963
This comment has been minimized.
This comment has been minimized.
|
cc @sfackler |
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
Woo! There is one pretty significant difference between jemalloc 4 and 5. The new version depends on a background thread to clear out allocation caches, and the background thread defaults to off. Without it, we saw a very large increase in heap memory on a production service until we enabled background threads. See jemalloc/jemalloc#1128 for some background. While this doesn't matter for rustc at all, it will probably have an impact on long running services. We may want to fold the switch to the system allocator as the default into this change as well? |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
|
|
SimonSapin
referenced this pull request
Oct 21, 2018
Closed
liballoc_system without #[global_allocator] uses jemalloc #45966
This comment has been minimized.
This comment has been minimized.
As far as I understand, that switch is already part of this PR. |
alexcrichton
force-pushed the
alexcrichton:rm-jemalloc
branch
from
7094bb6
to
94cb105
Oct 21, 2018
This comment has been minimized.
This comment has been minimized.
|
@sfackler ah yeah as @SimonSapin mentioned this PR is doing the switch as well as the update. Despite it looking like we enable the background thread by default in |
This comment has been minimized.
This comment has been minimized.
|
Those options compile in support for the background thread but it's still disabled at runtime by default. I don't think it would have much of an effect on rustc memory usage - my understanding is that it's used to clean up caches on idle threads and that active threads clean themselves up periodically when touching the allocator. There are a bunch of settings you can poke at to control how aggressively jemalloc caches if you want: http://jemalloc.net/jemalloc.3.html |
This comment has been minimized.
This comment has been minimized.
|
@bors r+ |
This comment was marked as outdated.
This comment was marked as outdated.
|
|
bors
added
S-waiting-on-bors
and removed
S-waiting-on-review
labels
Oct 21, 2018
This comment was marked as outdated.
This comment was marked as outdated.
bors
added a commit
that referenced
this pull request
Oct 21, 2018
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as outdated.
This comment was marked as outdated.
|
|
bors
added
S-waiting-on-review
and removed
S-waiting-on-bors
labels
Oct 21, 2018
kennytm
added
S-waiting-on-author
and removed
S-waiting-on-review
labels
Oct 22, 2018
gnzlbg
reviewed
Oct 23, 2018
| // enable this by default on other platforms, so other platforms aren't handled | ||
| // here yet. | ||
| #[cfg(feature = "jemalloc-sys")] | ||
| extern crate jemalloc_sys; |
This comment has been minimized.
This comment has been minimized.
gnzlbg
Oct 23, 2018
Contributor
I get that on these platforms global_allocator is not needed, but I still do wonder why this does not use it.
This comment has been minimized.
This comment has been minimized.
alexcrichton
Oct 24, 2018
Author
Member
The #[global_allocator] attribute is actually incompatible with this due to the way linkage works and such. I can go into details if necessary, but the answer to your question at least is "it's required by the way we build rustc"
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors: retry After many attempts locally I can't reproduce, so I'm curious if we can get another failure on bors... |
bors
added
S-waiting-on-bors
and removed
S-waiting-on-author
labels
Oct 24, 2018
bors
referenced this pull request
Nov 3, 2018
Merged
Support memcpy/memmove with differing src/dst alignment #55633
This was referenced Nov 3, 2018
alexcrichton
deleted the
alexcrichton:rm-jemalloc
branch
Nov 3, 2018
alexcrichton
added
the
relnotes
label
Nov 3, 2018
This comment has been minimized.
This comment has been minimized.
|
Just for clarity, does this change just affect |
gnzlbg
reviewed
Nov 3, 2018
| @@ -2464,6 +2504,11 @@ impl OngoingCodegen { | |||
| } | |||
| } | |||
|
|
|||
| // impl Drop for OngoingCodegen { | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Yes, |
This comment has been minimized.
This comment has been minimized.
|
@jonhoo this affects all crates, no crate will be compiled against jemalloc by default now. The compilers we distribute for OSX/Linux will, however, still enable jemalloc (but only the compiler itself) |
ehuss
referenced this pull request
Nov 4, 2018
Closed
Can't find "use-jemalloc" in config.toml. #227
symphorien
referenced this pull request
Nov 7, 2018
Merged
rustc: build with system llvm and jemalloc #49557
This comment has been minimized.
This comment has been minimized.
|
Here are the final perf results from the landing. Almost everything improved by at least 1%, and some were up to 15%. Pretty great! |
This comment has been minimized.
This comment has been minimized.
|
This is most likely from upgrading jemalloc from 4.5 to 5.1, isn’t it? |
This comment has been minimized.
This comment has been minimized.
|
Are there plans to do something about the memory usage regression? |
This comment has been minimized.
This comment has been minimized.
|
@lnicola the |
alexcrichton commentedOct 21, 2018
This commit removes the
alloc_jemalloccrate from the standard library and all related configuration. We will no longer be shipping this unstable crate. Rationale for this is provided on #36963 and the many linked issues, but I can inline rationale here if desired!We currently rely on jemalloc for increased perf in the Rust compiler, however. This perf run shows that if we switch to glibc 2.23's allocator that it's slower than jemalloc across many benchmarks. This perf run, however, shows that if we use
jemalloc-sysfrom crates.io then rustc actually gets faster across all benchmarks! (presumably because it has a more recent version of jemalloc than our submodule).As a result, it's expected that this doesn't regress any code (as it's just removing an unstable crate) and it should actually improve rustc performance because it updates jemalloc.
Closes #36963