-
Notifications
You must be signed in to change notification settings - Fork 14k
Simplify jemalloc setup
#146627
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
Simplify jemalloc setup
#146627
Conversation
This comment has been minimized.
This comment has been minimized.
15a8fbc to
551e285
Compare
|
Also, in the second commit, I fixed |
Simplify `jemalloc` setup try-job: `aarch64-gnu` try-job: `dist-aarch64-linux` try-job: `dist-x86_64-musl` try-job: `dist-x86_64-apple` try-job: `dist-aarch64-apple`
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy The Miri subtree was changed cc @rust-lang/miri These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
de2de0b to
2c0d266
Compare
|
☔ The latest upstream changes (presumably #148412) made this pull request unmergeable. Please resolve the merge conflicts. |
2c0d266 to
aa3fd28
Compare
This comment has been minimized.
This comment has been minimized.
| use rustc_middle::ty::TyCtxt; | ||
| use rustc_session::config::{ErrorOutputType, RustcOptGroup, make_crate_type_option}; | ||
| use rustc_session::{EarlyDiagCtxt, getopts}; | ||
| /// See docs in https://github.com/rust-lang/rust/blob/HEAD/compiler/rustc/src/main.rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you also link to this PR here? It's much easier for someone to accidentally change something in rustc and this reference to go stale, while if you link to this PR the reasoning always stays clear.
r=me after that
@rustbot author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps pin the reference to a commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with:
/// See docs in https://github.com/rust-lang/rust/blob/HEAD/compiler/rustc/src/main.rs
/// and https://github.com/rust-lang/rust/pull/146627 for why we need this `use` statement.
(There wasn't really a commit I could pin it to while changing the contents in the same commit).
|
Error: Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
Rollup merge of #146627 - madsmtm:jemalloc-simplify, r=jdonszelmann Simplify `jemalloc` setup In the past, `#[used]` had to appear in the top-level crate to have a consistent effect on the linker. This has been fixed a while ago for ELF with the introduction of the `symbols.o` file in #95604, and more recently for Mach-O in #133832, which means that libraries can now implement the required workarounds themselves. This allows moving these `#[used]` declarations out of our `main.rs`. Specifically, I have moved them into `tikv-jemalloc-sys` where they belong in tikv/jemallocator#109 and done the same for `mimalloc` in purpleprotocol/mimalloc_rust#146 (in case we want to experiment with switching to that one day). Test with: ```sh ./x build library src/tools/rustdoc src/tools/clippy --set rust.jemalloc=true # macOS lldb -- ./build/host/stage1/bin/rustc -vV (lldb) b _rjem_je_zone_register (lldb) run # Should breakpoint, this means that the allocator was properly linked # Linux lldb -- ./build/host/stage1/bin/rustc -vV (lldb) b malloc (lldb) run # Should breakpoint, inspect that the `malloc` symbol comes from the `rustc` binary and not from `libc` ``` try-job: `aarch64-gnu` try-job: `dist-aarch64-linux` try-job: `dist-x86_64-musl` try-job: `dist-x86_64-apple` try-job: `dist-aarch64-apple`
Rollup of 16 pull requests Successful merges: - rust-lang/rust#146627 (Simplify `jemalloc` setup) - rust-lang/rust#147753 (Suggest add bounding value for RangeTo) - rust-lang/rust#147832 (rustdoc: Don't pass `RenderOptions` to `DocContext`) - rust-lang/rust#147974 (Improve diagnostics for buffer reuse with borrowed references) - rust-lang/rust#148080 ([rustdoc] Fix invalid jump to def macro link generation) - rust-lang/rust#148465 (Adjust spans into the `for` loops context before creating the new desugaring spans.) - rust-lang/rust#148500 (Update git index before running diff-index) - rust-lang/rust#148531 (rustc_target: introduce Abi, Env, Os) - rust-lang/rust#148536 (cmse: add test for `async` and `const` functions) - rust-lang/rust#148770 (implement `feature(c_variadic_naked_functions)`) - rust-lang/rust#148780 (fix filecheck typos in tests) - rust-lang/rust#148819 (Remove specialized warning for removed target) - rust-lang/rust#148830 (miri subtree update) - rust-lang/rust#148833 (Update rustbook dependencies) - rust-lang/rust#148834 (fix(rustdoc): Color doctest errors) - rust-lang/rust#148841 (Remove more `#[must_use]` from portable-simd) r? `@ghost` `@rustbot` modify labels: rollup
|
Bors, this has already been merged. @bors r- retry |
Regressions in doc benchmarks across the board; this seems unintended? |
|
Maybe rustdoc accidentally is not using jemalloc any more? |
|
Some allocation changes have definitely happened there: I would suggest perf. testing any PR that touches things like memory allocation, for the next time 😅 I will post a revert, so that we can figure out what happened here. |
Revert "Rollup merge of #146627 - madsmtm:jemalloc-simplify, r=jdonszelmann"
Could you tell me how you arrived at that table? I'm unfamiliar with Linux perf tooling.
Yeah, sorry about that, I should've done that! |
Yeah, sorry. If you open a benchmark row from the comparison table, it shows "Command for profiling locally", which you can copy paste in a checkout of https://github.com/rust-lang/rustc-perf to do a Cachegrind diff before/after the PR. |
Revert "Rollup merge of #146627 - madsmtm:jemalloc-simplify, r=jdonszelmann" This reverts commit 5dc3c19, reversing changes made to 11339a0. Reverts #146627 due to a [perf regression](#148851 (comment)). r? `@Zalathar`
Revert "Rollup merge of #146627 - madsmtm:jemalloc-simplify, r=jdonszelmann" This reverts commit 5dc3c19417d0fbfd979c5a1ecd8bebe2d8d9110e, reversing changes made to 11339a0ef5ed586bb7ea4f85a9b7287880caac3a. Reverts rust-lang/rust#146627 due to a [perf regression](rust-lang/rust#148851 (comment)). r? `@Zalathar`
Simplify `jemalloc` setup (without perf regression) Reland #146627 after fixing [the performance regression](#148851 (comment)) that caused it to be reverted in #148896. This avoids 65f0b7a (second commit in the initial PR), and adds a comment explaining why `extern crate` is needed here instead of `use` (we need to load `tikv_jemalloc_sys` from the sysroot because of rust-lang/cc-rs#1613). r? Kobzol
Simplify `jemalloc` setup (without perf regression) Reland #146627 after fixing [the performance regression](#148851 (comment)) that caused it to be reverted in #148896. This avoids 65f0b7a (second commit in the initial PR), and adds a comment explaining why `extern crate` is needed here instead of `use` (we need to load `tikv_jemalloc_sys` from the sysroot because of rust-lang/cc-rs#1613). r? Kobzol
Simplify `jemalloc` setup (without perf regression) Reland rust-lang/rust#146627 after fixing [the performance regression](rust-lang/rust#148851 (comment)) that caused it to be reverted in rust-lang/rust#148896. This avoids 65f0b7a (second commit in the initial PR), and adds a comment explaining why `extern crate` is needed here instead of `use` (we need to load `tikv_jemalloc_sys` from the sysroot because of rust-lang/cc-rs#1613). r? Kobzol
Simplify `jemalloc` setup (without perf regression) Reland rust-lang/rust#146627 after fixing [the performance regression](rust-lang/rust#148851 (comment)) that caused it to be reverted in rust-lang/rust#148896. This avoids 65f0b7a (second commit in the initial PR), and adds a comment explaining why `extern crate` is needed here instead of `use` (we need to load `tikv_jemalloc_sys` from the sysroot because of rust-lang/cc-rs#1613). r? Kobzol
miri: use `tikv-jemalloc-sys` from sysroot This allows LTO to work when compiling jemalloc (which is currently broken due to rust-lang/cc-rs#1613), which should yield a small performance boost, and makes Miri's behaviour here match Clippy and Rustdoc. Follow-up to rust-lang#148925 / rust-lang#146627 after discussion in rust-lang#148925 (review). r? RalfJung
Rollup merge of #149252 - madsmtm:miri-jemalloc, r=RalfJung miri: use `tikv-jemalloc-sys` from sysroot This allows LTO to work when compiling jemalloc (which is currently broken due to rust-lang/cc-rs#1613), which should yield a small performance boost, and makes Miri's behaviour here match Clippy and Rustdoc. Follow-up to #148925 / #146627 after discussion in #148925 (review). r? RalfJung
miri: use `tikv-jemalloc-sys` from sysroot This allows LTO to work when compiling jemalloc (which is currently broken due to rust-lang/cc-rs#1613), which should yield a small performance boost, and makes Miri's behaviour here match Clippy and Rustdoc. Follow-up to rust-lang/rust#148925 / rust-lang/rust#146627 after discussion in rust-lang/rust#148925 (review). r? RalfJung
In the past,
#[used]had to appear in the top-level crate to have a consistent effect on the linker. This has been fixed a while ago for ELF with the introduction of thesymbols.ofile in #95604, and more recently for Mach-O in #133832, which means that libraries can now implement the required workarounds themselves. This allows moving these#[used]declarations out of ourmain.rs.Specifically, I have moved them into
tikv-jemalloc-syswhere they belong in tikv/jemallocator#109 and done the same formimallocin purpleprotocol/mimalloc_rust#146 (in case we want to experiment with switching to that one day).Test with:
try-job:
aarch64-gnutry-job:
dist-aarch64-linuxtry-job:
dist-x86_64-musltry-job:
dist-x86_64-appletry-job:
dist-aarch64-apple