Skip to content
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

Merged

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Apr 1, 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 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.

@rustbot
Copy link
Collaborator

rustbot commented Apr 1, 2024

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

@rustbot
Copy link
Collaborator

rustbot commented Apr 1, 2024

⚠️ Warning ⚠️

  • These commits modify submodules.

@rustbot 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
@rustbot
Copy link
Collaborator

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

@workingjubilee
Copy link
Member Author

huh odd.

@workingjubilee workingjubilee force-pushed the debug-compiler-profile-expectations branch from f542959 to f43ad4c Compare April 1, 2024 20:16
@workingjubilee
Copy link
Member Author

Fixed the submodule thing, whoops.

@workingjubilee
Copy link
Member Author

workingjubilee commented Apr 1, 2024

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

@saethlin
Copy link
Member

saethlin commented Apr 1, 2024

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

@saethlin
Copy link
Member

saethlin commented Apr 1, 2024

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.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 2, 2024

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?

@workingjubilee
Copy link
Member Author

Hm. Is there a significant size/compiletime difference between simply debug = true and

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

@Kobzol
Copy link
Contributor

Kobzol commented Apr 2, 2024

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

@onur-ozkan
Copy link
Member

onur-ozkan commented Apr 2, 2024

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.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 2, 2024

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.

@workingjubilee
Copy link
Member Author

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.

@saethlin
Copy link
Member

saethlin commented Apr 2, 2024

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.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 2, 2024

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.

@saethlin
Copy link
Member

saethlin commented Apr 2, 2024

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.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 2, 2024

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

@saethlin
Copy link
Member

saethlin commented Apr 2, 2024

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.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 2, 2024

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.

@onur-ozkan
Copy link
Member

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 rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 2, 2024
@rustbot rustbot assigned cjgillot and unassigned onur-ozkan Apr 2, 2024
@klensy
Copy link
Contributor

klensy commented Apr 3, 2024

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

@klensy
Copy link
Contributor

klensy commented Apr 20, 2024

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.

@workingjubilee workingjubilee force-pushed the debug-compiler-profile-expectations branch from f43ad4c to 3acc407 Compare April 25, 2024 00:53
@workingjubilee
Copy link
Member Author

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 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 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
Copy link
Member Author

r? compiler

@rustbot rustbot assigned fmease and unassigned cjgillot Apr 25, 2024
@workingjubilee 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 workingjubilee changed the title Set debuginfo-level = "line-tables-only" for compiler profile Include line tables in compiler profile Apr 25, 2024
@bors
Copy link
Contributor

bors commented May 5, 2024

☔ The latest upstream changes (presumably #124726) made this pull request unmergeable. Please resolve the merge conflicts.

@workingjubilee workingjubilee force-pushed the debug-compiler-profile-expectations branch from 3acc407 to 887151a Compare May 16, 2024 00:37
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.
Copy link
Member

@fmease fmease left a 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?)? 🤔

@fmease
Copy link
Member

fmease commented May 16, 2024

@bors r+

@bors
Copy link
Contributor

bors commented May 16, 2024

📌 Commit 887151a has been approved by fmease

It is now in the queue for this repository.

@bors 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
@bors
Copy link
Contributor

bors commented May 16, 2024

⌛ Testing commit 887151a with merge bf8801d...

@bors
Copy link
Contributor

bors commented May 16, 2024

☀️ Test successful - checks-actions
Approved by: fmease
Pushing bf8801d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 16, 2024
@bors bors merged commit bf8801d into rust-lang:master May 16, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 16, 2024
@rust-timer
Copy link
Collaborator

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

@workingjubilee workingjubilee deleted the debug-compiler-profile-expectations branch May 16, 2024 17:43
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants