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

bootstrap: Include line numbers in debuginfo by default for library/compiler profile #104968

Closed
wants to merge 1 commit into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Nov 27, 2022

This only includes debuginfo for the relevant component, so for library and tool developers the compile time impact should be negligible. For compiler authors I think the improved backtraces are worth the slight compile time hit (roughly 10% slower).

  • Set the default to LineTablesOnly. This isn't enough debuginfo to run perf, but it's enough to
    get full backtraces.

  • Don't build debuginfo for build scripts and proc macros. This lowers the total disk space for stage0-rustc from 5.4 to 5.3 GB, or by 120 MB.

  • Fix a bug in deserialization where string values would never be deserialized

    For reasons I don't quite understand, using &str unconditionally failed:

    thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { inner: Error { inner: TomlError { message: "data did not match any variant of untagged enum StringOrInt", original: None, keys: [], span: None } } }', src/main.rs:15:49
    

@rustbot
Copy link
Collaborator

rustbot commented Nov 27, 2022

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2022
@Mark-Simulacrum
Copy link
Member

My recollection is that this increases the disk size pretty considerably as well, could you measure that while you get the timing estimate?

@jyn514
Copy link
Member Author

jyn514 commented Nov 28, 2022

before:

; x build --stage 0 compiler
Building stage0 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 8m 13s

; ls build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/{librustc_driver.so,rustc-main} -lh
-rwxrwxr-x 4 jnelson jnelson 115M Nov 28 13:20 build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/librustc_driver.so
-rwxrwxr-x 3 jnelson jnelson  17K Nov 28 13:20 build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/rustc-main

after:

; x build --stage 0 compiler
Building stage0 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized + debuginfo] target(s) in 10m 55s

; ls build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/{librustc_driver.so,rustc-main} -lh
-rwxrwxr-x 4 jnelson jnelson 529M Nov 28 13:36 build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/librustc_driver.so
-rwxrwxr-x 3 jnelson jnelson  22K Nov 28 13:36 build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/rustc-main

@jyn514
Copy link
Member Author

jyn514 commented Nov 28, 2022

I wonder if it makes sense to only do this for the tools and library profiles?

@saethlin
Copy link
Member

saethlin commented Nov 28, 2022

I think I have a system with a bit more parallelism. I see a much larger similar % slowdown, and turning on use-lld = true removes most of it.

I do not think this degree of slowdown is acceptable for a default, especially because evidence suggests it is mostly loaded into running the linker, which will always be done. If we could have use-lld = true on by default I think the regression might be tolerable.

I wonder if it makes sense to only do this for the tools and library profiles?

I think @thomcc previously commented that the benefit for library is dubious. I defer to more experienced library contributors there. I also think I spend a lot more time profiling Miri than anyone else, and the iteration speed on it is already rather brutal, so similarly I'm cautious about penalizing the workflows of others for my own convenience.

@jyn514
Copy link
Member Author

jyn514 commented Nov 28, 2022

I thought @thomcc was in favor of this change for library.

@Mark-Simulacrum
Copy link
Member

It's really unfortunate to increase the library by ~5x, to half a gigabyte(!), even aside from the potential wall-time slowdown.

I don't think we should do this by default at this time, it's too expensive a default. I do wonder whether the debuginfo really should be taking that much room -- naively, one might expect that a "line tables only" version should be able to be roughly on-par with what the source code (actually better!) itself takes up, whereas this is much bigger. Maybe we're including not just line table information but something more (e.g., types of some kind)?

cc @rust-lang/wg-debugging -- I wonder if folks here have thoughts on what we can do to give a nicer experience at lower cost, or whether we think debuginfo is the wrong way to go, etc.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2022
@michaelwoerister
Copy link
Member

The 5x size increase does seem fishy.

@bors
Copy link
Contributor

bors commented Feb 3, 2023

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

@jyn514
Copy link
Member Author

jyn514 commented Mar 18, 2023

It turns out the vast majority of the size (over 350 MB!) comes from the .debug_info, .debug_pubnames, and .debug_str sections, not the line info itself: https://rust-lang.zulipchat.com/#narrow/stream/317568-t-compiler.2Fwg-debugging/topic/investigating.20debuginfo.20size
Apparently there's tooling depending on those sections to exist 🤦 so we can't remove it: #64405

@jyn514
Copy link
Member Author

jyn514 commented Mar 18, 2023

#69074 also looks related.

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2023

This PR changes src/bootstrap/defaults/config.codegen.toml. If appropriate, please also update config.compiler.toml so the defaults are in sync.

This PR changes src/bootstrap/defaults/config.compiler.toml. If appropriate, please also update config.codegen.toml so the defaults are in sync.

@jyn514 jyn514 changed the title Include line numbers in debuginfo by default for developers bootstrap: Include line numbers in debuginfo by default for library/compiler profile Mar 18, 2023
@jyn514
Copy link
Member Author

jyn514 commented Mar 18, 2023

Ok, I added a new unstable flag that stops generating extra debuginfo when only line-level info is set, and enabled that and compression when building rustc itself. After that change, librustc_driver.so is 154 MB on my computer, which seems roughly comparable to the 131 MB used by the version without debuginfo. Do you think that's small enough we can land the change in defaults?

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 18, 2023
@jyn514
Copy link
Member Author

jyn514 commented Mar 18, 2023

ah, I just realized the reduced size won't take effect for most contributors until the new flag hits beta - I can split it into a separate PR so it lands without having to change the defaults at the same time.

@jyn514
Copy link
Member Author

jyn514 commented Mar 18, 2023

Wall time before:

Building compiler artifacts (stage1 -> stage2)
    Finished release [optimized] target(s) in 1m 58s
Assembling stage2 compiler
Build completed successfully in 0:01:59

and after:

Building compiler artifacts (stage1 -> stage2)
    Finished release [optimized + debuginfo] target(s) in 2m 13s
Assembling stage2 compiler
Build completed successfully in 0:02:13

@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2023
…oerister

Extend -Cdebuginfo with new options and named aliases

This is a rebase of rust-lang#83947, along with my best guess at what the new options mean. I tried to follow the LLVM source code to get a better idea but ran into quite a lot of trouble (https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/go-to-definition.20in.20src.2Fllvm-project.3F). The description for the original PR follows below.

Note that the changes in this PR have already been through FCP: rust-lang#83947 (comment)

Closes rust-lang#109311. Helps with rust-lang#104968.
r? `@michaelwoerister` cc `@cuviper`

---

The -Cdebuginfo=1 option was never line tables only and can't be due to backwards compatibility issues. This was clarified and an option for emitting line tables only was added. Additionally an option for emitting line info directives only was added, which is needed for some targets, i.e. nvptx. The debug info options should now behave similarly to clang's debug info options.

Fix rust-lang#60020
Fix rust-lang#64405
@bors
Copy link
Contributor

bors commented Apr 4, 2023

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

saethlin pushed a commit to saethlin/miri that referenced this pull request Apr 10, 2023
Extend -Cdebuginfo with new options and named aliases

This is a rebase of rust-lang/rust#83947, along with my best guess at what the new options mean. I tried to follow the LLVM source code to get a better idea but ran into quite a lot of trouble (https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/go-to-definition.20in.20src.2Fllvm-project.3F). The description for the original PR follows below.

Note that the changes in this PR have already been through FCP: rust-lang/rust#83947 (comment)

Closes rust-lang/rust#109311. Helps with rust-lang/rust#104968.
r? `@michaelwoerister` cc `@cuviper`

---

The -Cdebuginfo=1 option was never line tables only and can't be due to backwards compatibility issues. This was clarified and an option for emitting line tables only was added. Additionally an option for emitting line info directives only was added, which is needed for some targets, i.e. nvptx. The debug info options should now behave similarly to clang's debug info options.

Fix rust-lang/rust#60020
Fix rust-lang/rust#64405
@jyn514
Copy link
Member Author

jyn514 commented Jun 6, 2023

waiting on me to test #110221 (comment)

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 6, 2023
@jyn514
Copy link
Member Author

jyn514 commented Jun 6, 2023

I think a sense of what the impact is on the full build directory with and without debuginfo would help make a decision on whether this makes sense as a default (the PR contains numbers for librustc_driver only).

Without debuginfo:

; CARGOFLAGS=--timings x b --stage 0 compiler
Building compiler artifacts (stage0 -> stage1, x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 1m 07s
; usage build/host/stage0-rustc
3.4G    build/host/stage0-rustc
3.0G    build/host/stage0-rustc/x86_64-unknown-linux-gnu
355M    build/host/stage0-rustc/release
244K    build/host/stage0-rustc/cargo-timings

With debuginfo-level-rustc = 1:

; CARGOFLAGS=--timings x b --stage 0 compiler
Building compiler artifacts (stage0 -> stage1, x86_64-unknown-linux-gnu)
    Finished release [optimized + debuginfo] target(s) in 1m 13s
; usage build/host/stage0-rustc
4.4G    build/host/stage0-rustc
4.0G    build/host/stage0-rustc/x86_64-unknown-linux-gnu
401M    build/host/stage0-rustc/release
256K    build/host/stage0-rustc/cargo-timings

For comparison, build/host/stage0-std takes up 500 MB total in both cases.

Both of these were taken with the default settings from profile = compiler on b2b34bd.

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 6, 2023
@jyn514
Copy link
Member Author

jyn514 commented Jun 7, 2023

oh, i just realized we can cut down most of the added space in release by disabling debuginfo for build dependencies - that should save 50 mb or so.

@Mark-Simulacrum
Copy link
Member

r=me, unless you want to do more here.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2023
@jyn514
Copy link
Member Author

jyn514 commented Jun 24, 2023

This had a semantic conflict with #112528 - I've rebased and pushed a commit that fixes the conflict as well as not building debuginfo for build scripts.

@jyn514
Copy link
Member Author

jyn514 commented Jun 24, 2023

err it's not actually correct, it changed the default for debug = true instead of the per-profile default

- Set the default to LineTablesOnly. This isn't enough debuginfo to run perf, but it's enough to
get full backtraces.
- Don't build debuginfo for build scripts and proc macros. This lowers the total disk space for stage0-rustc from 5.4 to 5.3 GB, or by 120 MB.
- Fix a bug in deserialization where string values would never be deserialized

  For reasons I don't quite understand, using `&str` unconditionally failed:
    ```
    thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { inner: Error { inner: TomlError { message: "data did not match any variant of untagged enum StringOrInt", original: None, keys: [], span: None } } }', src/main.rs:15:49
    ```
@jyn514
Copy link
Member Author

jyn514 commented Jun 24, 2023

something here is broken and i don't have time to investigate it right now

; ls -lh build/host/stage0-rustc/x86_64-unknown-linux-gnu/release/librustc_driver.so 
-rwxrwxr-x 4 jyn 278M Jun 23 22:35 build/host/stage0-rustc/x86_64-unknown-linux-gnu/release/librustc_driver.so

this was significantly smaller last time i measured it and i don't know what changed.

@bors
Copy link
Contributor

bors commented Jul 2, 2023

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

@workingjubilee workingjubilee added the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Jul 30, 2023
@jyn514
Copy link
Member Author

jyn514 commented Aug 21, 2023

rust-lang/team#1051

@jyn514 jyn514 closed this Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants