Include line tables in compiler profile by workingjubilee · Pull Request #123337 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation32 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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 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.

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

⚠️ Warning ⚠️

These commits modify submodules.

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

labels

Apr 1, 2024

Some changes occurred in src/tools/cargo

cc @ehuss

This PR modifies src/bootstrap/defaults.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

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 debug-logging we already had plus debuginfo-level = 1).

Yeah; if you set debug = true, all the ignore-debug tests are... ignored. The situation used to be significantly worse than it is now, for example this PR that Ralf put up which passed CI (because all the CI jobs set debug = true) and passed the test suite when he ran it, but would have failed in bors: #122629 (comment)

I've since been able to remove a number of the ignore-debug annotations because some of the standard library debug assertions now branch on compiler magic instead of cfg!(debug_assertions). But I do not think we will ever get rid of all of them.

For that reason, I set

debug = false
debuginfo-level = 1
debug-logging = true

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

debug-assertions = true
debug-assertions-std = false

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 debug = true and

[rust] debug = false debuginfo-level = 1 debug-logging = true

Stage 1 build of rustc on my laptop:

Without debug = true: 170s, build directory has 5.9 GiB
With debug = true: 218s, build directory has 8.2 GiB

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).

Stage 1 build of rustc on my laptop:

Without debug = true: 170s, build directory has 5.9 GiB

With debug = true: 218s, build directory has 8.2 GiB

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.

It seems quite bad to me tbh 😃

Yeah I would still not enable this by default 😆 I just thought that the effect was even worse.

Stage 1 build of rustc on my laptop:

Without debug = true: 170s, build directory has 5.9 GiB

With debug = true: 218s, build directory has 8.2 GiB

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.

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:

Someone builds the compiler
They run a profiling thing that takes 2-10 hours
They find out the information from that is total gibberish because they don't have debuginfo

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.

Stage 1 build of rustc on my laptop:

Without debug = true: 170s, build directory has 5.9 GiB

With debug = true: 218s, build directory has 8.2 GiB

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 build directory and performed python3 x.py build library to get these numbers. I'm using downloaded LLVM, perhaps you build it yourself? Otherwise the build directory shouldn't be that big, IIRC.

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 build dir quite often 😁 Anyway, increasing the debuginfo slightly by default might be a good idea, we also do enable frame pointers by default after all, for beter profiling. But debug = true just seems wasteful, I don't think that a lot of contributors that depend on defaults are actually debugging the compiler with a step-by-step debugger. Line numbers/tables should be enough for everybody™.

debug = true (at least according to config.example.toml) sets debuginfo to level 1 meaning line tables, and also enables debug assertions and debug logging. I think those are all quite plausible things for a compiler user to set. The Cargo.toml debug = true would indeed be excessive.

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 PR modifies src/bootstrap/defaults.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This change needs to be added to the change tracker.

r? compiler (as this mostly affects the compiler team)

rustbot added the T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

label

Apr 2, 2024

Debuginfo can use line-tables-only instead of 1 (which should be smaller?) when #123364 be resolved.

Debuginfo can use line-tables-only instead of 1 (which should be smaller?) when #123364 be resolved.

With #123364 merged:

debuginfo-level=1: 12m 30s

$ du -s build/x86_64-unknown-linux-gnu/stage0-rustc
2678720 build/x86_64-unknown-linux-gnu/stage0-rustc

debuginfo-level="line-tables-only": 11m 09s

$ du -s build/x86_64-unknown-linux-gnu/stage0-rustc 
2179256 build/x86_64-unknown-linux-gnu/stage0-rustc

Time can be wrong, bc of cold\warm disk cache, VM performance, etc.

callgrind\dhat works fine, displaying lines.

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

workingjubilee changed the title~~Default rust.debug = true for compiler contribs~~ Set rust.debuginfo-level = "line-tables-only" for compiler contribs

Apr 25, 2024

workingjubilee changed the title~~Set rust.debuginfo-level = "line-tables-only" for compiler contribs~~ Set rust.debuginfo-level = "line-tables-only" for compiler profile

Apr 25, 2024

workingjubilee changed the title~~Set rust.debuginfo-level = "line-tables-only" for compiler profile~~ Set debuginfo-level = "line-tables-only" for compiler profile

Apr 25, 2024

workingjubilee changed the title~~Set debuginfo-level = "line-tables-only" for compiler profile~~ Include line tables in compiler profile

Apr 25, 2024

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.

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?)? 🤔

📌 Commit 887151a has been approved by fmease

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

May 16, 2024

This was referenced

May 16, 2024

Finished benchmarking commit (bf8801d): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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.

mean	range	count
Regressions ❌ (primary)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.9%	[-3.9%, -3.9%]	1
All ❌✅ (primary)	2.5%	[2.5%, 2.5%]	1

Cycles

Results (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.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.2%, 2.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 679.795s -> 679.188s (-0.09%)
Artifact size: 316.18 MiB -> 316.03 MiB (-0.05%)

bherrera pushed a commit to misttech/integration that referenced this pull request

Oct 16, 2024

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

Labels

merged-by-bors

This PR was explicitly merged by bors.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.