-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Include line tables in compiler profile #123337
Include line tables in compiler profile #123337
Conversation
r? @onur-ozkan rustbot has assigned @onur-ozkan. Use |
|
Some changes occurred in src/tools/cargo cc @ehuss This PR modifies If appropriate, please update |
huh odd. |
f542959
to
f43ad4c
Compare
Fixed the submodule thing, whoops. |
Hmm. Based on a remark from @saethlin it seems possible we might want to have a more nuanced setting here (the |
Yeah; if you set I've since been able to remove a number of the For that reason, I set
This makes sure I have symbols and access to logging, but can actually run the whole test suite. One could make an argument for setting
but I'm just a bit too lazy |
To be clear, I don't want to actually advocate for people to turn off the debug assertions I've worked so hard on, but also, they kind of break quite fundamental workflows because bootstrap doesn't know that codegen tests need to be run against the sysroot config we distribute, not whatever random config happens to be specified by the developer. |
Wouldn't this make the compilation of the compiler a lot slower by defaut, and also increase the size of artifacts on disk by (tens of?) GiBs? |
Hm. Is there a significant size/compiletime difference between simply [rust]
debug = false
debuginfo-level = 1
debug-logging = true |
Stage 1 build of
Ok, that's actually.. not that bad, I remember it being much worse (maybe we default to a lower debuginfo level now? or it only shows in stage2 builds). |
It seems quite bad to me tbh 😃 Activating a value by default that significantly increases build time and disk usage isn't very sensible, especially if it can be activated when desired or needed. |
Yeah I would still not enable this by default 😆 I just thought that the effect was even worse. |
This is a profile setting primarily for people who are already debugging the compiler, however. Anyways I think I will test a modification on the build settings @saethlin described. I think basic debuginfo for the compiler is important, because what happens is often:
That sure happened to me, at least. And for some contributors, merely "rebuild the compiler to get debuginfo" is not really an acceptable additional time overhead. |
I have two rustc checkouts, and their build directories are 35 GB and 26 GB. The 2.3 GB for debug symbols is not very significant compared to all the other causes of rampant disk use we have. Sure, this looks like extra disk use if you do a clean build, but I'd be a bit surprised if people are doing that. It's probably just in the noise for normal workflows. |
I removed the |
I strongly suspect that the build directories I have are large because they are accumulated build artifacts from swapping between branches and running the test suite. I'm not removing my build directory every time I switch branches or something like that. |
I have like 20 GiB of free space left on my laptop at any given time (don't ask me why), so I delete the |
|
Ah, now I remember that this has changed last year (?). Yeah, that woud explain why the compile time and disk hit is much more reasonable now. I remember it being excruciating a few years ago. |
This change needs to be added to the change tracker. r? compiler (as this mostly affects the compiler team) |
Debuginfo can use |
With #123364 merged:
Time can be wrong, bc of cold\warm disk cache, VM performance, etc. callgrind\dhat works fine, displaying lines. |
f43ad4c
to
3acc407
Compare
before: $ rm -rf build
$ ./x.py check
...time passes...
$ du -s build
7342136 build with this as my config.toml: # Includes one of the default files in src/bootstrap/defaults
profile = "compiler"
change-id = 123711 and this new variant, thanks to @klensy: $ rm -rf build
$ ./x.py check
...time passes...
$ du -s build
7511056 build |
rust.debug = true
for compiler contribsrust.debuginfo-level = "line-tables-only"
for compiler contribs
rust.debuginfo-level = "line-tables-only"
for compiler contribsrust.debuginfo-level = "line-tables-only"
for compiler profile
r? compiler |
rust.debuginfo-level = "line-tables-only"
for compiler profiledebuginfo-level = "line-tables-only"
for compiler profile
debuginfo-level = "line-tables-only"
for compiler profile
☔ The latest upstream changes (presumably #124726) made this pull request unmergeable. Please resolve the merge conflicts. |
3acc407
to
887151a
Compare
This profile has only undergone minimal tweaks since it was originally drafted. I asked a number of compiler contributors and they said they set rust.debug explicitly. This was even true for one contributor that set `rust.debug` = false! Almost everyone seems slightly surprised that `rust.debug = true` is not the default. However, adding full debuginfo at this level costs multiple gigabytes! We can still get much better debuginfo by setting "line-tables-only" at the cost of only 150~200 MB.
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.
Sounds very reasonable, thanks!
I wonder if there's any documentation that needs updating (rustc-dev-guide?)? 🤔
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (bf8801d): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 2.5%, secondary -3.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 2.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 679.795s -> 679.188s (-0.09%) |
Here are all the changes. I went through them one-by-one and confirmed that they should not be affecting us. In paritcular, we explicitly set rust.lld = false (because we want to use the lld that ships with clang), so the change in default does not affect us. There have been changes to x.py since you last updated: [INFO] New option `build.lldb` that will override the default lldb binary path used in debuginfo tests - PR Link rust-lang/rust#124501 [INFO] The compiler profile now defaults to rust.debuginfo-level = "line-tables-only" - PR Link rust-lang/rust#123337 [WARNING] `rust.lld` has a new default value of `true` on `x86_64-unknown-linux-gnu`. Starting at stage1, `rust-lld` will thus be this target's default linker. No config changes should be necessary. - PR Link rust-lang/rust#124129 [WARNING] Removed `dist.missing-tools` configuration as it was deprecated long time ago. - PR Link rust-lang/rust#125535 [WARNING] `llvm.lld` is enabled by default for the dist profile. If set to false, `lld` will not be included in the dist build. - PR Link rust-lang/rust#126701 [WARNING] `debug-logging` option has been removed from the default `tools` profile. - PR Link rust-lang/rust#127913 [INFO] the `wasm-component-ld` tool is now built as part of `build.extended` and can be a member of `build.tools` - PR Link rust-lang/rust#127866 [INFO] Removed android-ndk r25b support in favor of android-ndk r26d. - PR Link rust-lang/rust#120593 [WARNING] For tarball sources, default value for `rust.channel` will be taken from `src/ci/channel` file. - PR Link rust-lang/rust#125181 [INFO] New option `llvm.libzstd` to control whether llvm is built with zstd support. - PR Link rust-lang/rust#125642 [WARNING] ./x test --rustc-args was renamed to --compiletest-rustc-args as it only applies there. ./x miri --rustc-args was also removed. - PR Link rust-lang/rust#128841 [INFO] The `build.profiler` option now tries to use source code from `download-ci-llvm` if possible, instead of checking out the `src/llvm-project` submodule. - PR Link rust-lang/rust#129295 Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/recipes/+/1120078 Original-Revision: 27df37a30e50b14b9ffefc872b6997790f03d4ea GitOrigin-RevId: 341e222f002e36886b9960645b21faeaed633f90 Change-Id: Id1eb54a677a6f538bf7666d65b85d5fdba17ea42
This profile has only undergone minimal tweaks since it was originally drafted. I asked a number of compiler contributors and they said they set rust.debug explicitly. This was even true for one contributor that set
rust.debug = false
! Almost everyone seems slightly surprised thatrust.debug = true
is not the default.However, adding full debuginfo at this level costs multiple gigabytes! We can still get much better profiling and such by setting
rust.debuginfo-level = "line-tables-only"
at the cost of only 150~200 MB on the weight of a fresh build dir from./x.py check
.