Derive the allocator used by tools from rustc's allocator - #160372
Conversation
|
cc @rust-lang/clippy
cc @rust-lang/miri |
|
r? @folkertdev rustbot has assigned @folkertdev. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
|
||
| use std::process::ExitCode; | ||
|
|
||
| rustc_driver::override_c_allocator_in_binary!(); |
There was a problem hiding this comment.
| rustc_driver::override_c_allocator_in_binary!(); | |
| // Use the same allocator that rustc would use. | |
| rustc_driver::override_c_allocator_in_binary!(); |
also here
| // A note about jemalloc: rustc uses jemalloc when built for CI and | ||
| // distribution. The obvious way to do this is with the `#[global_allocator]` | ||
| // mechanism. However, for complicated reasons (see | ||
| // https://github.com/rust-lang/rust/pull/81782#issuecomment-784438001 for some | ||
| // details) that mechanism doesn't work here. Also, we'd like to use a | ||
| // consistent allocator across the rustc <-> llvm boundary, and | ||
| // `#[global_allocator]` wouldn't provide that. | ||
| // | ||
| // Instead, we use a lower-level mechanism, namely the |
There was a problem hiding this comment.
it seems a bit unfortunate to throw away these comments?
There was a problem hiding this comment.
They're outdated / wrong. We can use #[global_allocator] and I intend to in follow up work.
There was a problem hiding this comment.
The point about providing a consistent allocator for rustc and LLVM seems to still be true?
Cc @bjorn3
There was a problem hiding this comment.
They're outdated / wrong. We can use
#[global_allocator]and I intend to in follow up work.
Ok. I remember at ~3 attempts to do this in the past and they always ended up with "no, actually global_allocator doesn't work", so I'm curious to see what changed :)
There was a problem hiding this comment.
It would have been working since #122362 (unless I'm forgetting something).
The point about providing a consistent allocator for rustc and LLVM seems to still be true?
We want a consistent C allocator generally (for soundness), not just between rustc <-> LLVM. We also want a single allocator to keep memory usage down.
There was a problem hiding this comment.
I definitely tried using #[global_allocator] after #122362, but didn't have much success. Let's try again :)
There was a problem hiding this comment.
If you already override the C allocator, overriding #[global_allocator] shouldn't have any perf effect.
#[global_allocator] in rustc_driver should work now that we statically link libstd. But you did still need to link jemalloc from the main executable I think for the C allocator to be overridden.
| @@ -0,0 +1,22 @@ | |||
| /// This macro overrides the C allocator in final binaries by linking jemalloc with the override feature enabled. | |||
There was a problem hiding this comment.
| /// This macro overrides the C allocator in final binaries by linking jemalloc with the override feature enabled. | |
| /// This macro overrides the C allocator in final binaries by linking jemalloc with the `override_allocator_on_supported_platforms` feature enabled. |
The comments on #[global_allocator] can go here maybe, to motivate why this exists even though we have #[global_allocator].
|
Reminder, once the PR becomes ready for a review, use |
There was a problem hiding this comment.
I guess that in general this change makes sense, because historically we used a different allocator by the tools mostly by accident, rather than explicitly, and we were losing some performance because of that.
Why is it required to override the allocator on Windows, btw?
7c6fc1b to
79796b8
Compare
Windows doesn't have an official mechanism to override a symbol for the entire program, so instead we need to override it in each dylib / DLL and the binary (at link time) for programs which make use of LLVM / rustc_driver. |
| @@ -0,0 +1,22 @@ | |||
| /// This macro overrides the C allocator in final binaries by linking jemalloc with the override feature enabled. | |||
There was a problem hiding this comment.
| /// This macro overrides the C allocator in final binaries by linking jemalloc with the override feature enabled. | |
| /// This macro overrides the C allocator (i.e., `malloc`) in final binaries by linking jemalloc with the override feature enabled. | |
| /// `malloc` is used by `alloc::System` on Unix targets but not Windows targets. |
|
r? Kobzol |
|
|
|
@bors try @rust-timer queue profiles=Check,Debug,Opt,Doc,DocJson,Clippy |
|
Error occurred while parsing comment: Cannot parse profiles: Invalid profile: DocJson. Valid values are: check, debug, opt, doc, doc-json, clippy |
This comment has been minimized.
This comment has been minimized.
Derive the allocator used by tools from rustc's allocator
|
@rust-timer queue profiles=check,debug,opt,doc,doc-json,clippy |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (52500f2): comparison URL. Overall result: no relevant changes - BENCHMARK(S) FAILEDBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression ❗ ❗ ❗ ❗ ❗
❗ ❗ ❗ ❗ ❗ Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary 0.6%, secondary -0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.1%, secondary -0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 491.018s -> 491.012s (-0.00%) |
|
Oops, my bad, second try. @bors try jobs=dist-x86_64-linux @rust-timer queue profiles=check,debug,opt,doc,doc-json,clippy |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Derive the allocator used by tools from rustc's allocator try-job: dist-x86_64-linux
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (4403287): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.6%, secondary -0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.4%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 491.018s -> 490.844s (-0.04%) |
|
Okay, perf. looks good, and I went through miri and Clippy and it doesn't seem like this should break them, as they weren't using the feature directly (it was only used by bootstrap). There are still some unresolved comments about modifying the code comments a bit. |
|
The job Click to see the possible cause of the failure (guessed by this bot)Important For more information how to resolve CI failures of this job, visit this link. |
|
I did rewrite the comments a bit. |
View all comments
This derives the allocator used by tools from rustc's allocator. This simplifies the feature handling a bit and would be a requirement for globally overriding the C allocator on Windows.
The overriding for rustdoc is moved from the library to the binary, which is more of a proper location given that the override must be linked into the binary.
The code in this PR is LLM written.