Skip to content

build: Use unpacked split debuginfo and incremental build #23410

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

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

def-
Copy link
Contributor

@def- def- commented Nov 23, 2023

See https://doc.rust-lang.org/rustc/codegen-options/index.html#split-debuginfo

This should lead to:

  • slightly faster scratch build locally (4:12 -> 4:03)
  • faster release build in CI (~23 min -> ~17 min)
  • much faster incremental builds (cargo build && echo "//" >> src/environmentd/src/bin/environmentd/main.rs && time cargo build: 20 s -> 2.3 s)
  • smaller executable (2.7 GB -> 1.6 GB environmentd)
  • reduced disk usage (39 GB -> 32 GB target dir, but incremental makes it bloat again)
  • reduced memory usage during compilation (18.3 GB -> 17.4 GB from scratch, 1.8 GB -> 1.1 GB max incremental)

See for example https://www.productive-cpp.com/improving-cpp-builds-with-split-dwarf/ from a former colleague of mine, we had good experiences with split dwarf.

Does this matter for our release process or will the debuginfo still be uploaded correctly? Have to check with Brennan.

Also enable incremental build again, the previous problem seems to have been cargo clippy, see https://materializeinc.slack.com/archives/CMH6PG4CW/p1694572440379559 . Now that we enforce Rust 1.74 the issue seems to be fixed.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:

@def- def- changed the title build: Try unpacked split debuginfo build: Use unpacked split debuginfo and incremental build Nov 23, 2023
@def- def- marked this pull request as ready for review November 23, 2023 15:43
@benesch
Copy link
Contributor

benesch commented Nov 23, 2023

Unfortunately, last I looked, split-debuginfo = "unpacked" does not work as we need it to on Linux. Some thoughts on it here: https://materializeinc.slack.com/archives/C049UQ3RSF4/p1687471402709829

Yeah, turns out there have been two competing factions working on split debuginfo. The “DebugFission” project which was mostly confirmed with getting debuginfo out of intermediate object files to speed up linking when debugging (but not for distribution), and the Linux distros looking to make release distributions smaller by distributing debuginfo separately from the main package (but not really caring about compile times).

On Linux, setting split-debuginfo = "unpacked" produces DebugFission DWOs, which speeds up compile times but does not produce the distributable DWARF debug symbols that we upload to Polar Signals and S3—and, AFAIK, there's no way to turn those DWOs into that distributable package. The only way to get the distributable DWARF package is to build with split-debuginfo = "off" and then use objcopy to split the debuginfo out of the binary.

It seems ... maybe? ... reasonable to set split-debuginfo = "unpacked" in the dev profile, but unfortunately I think we need split-debuginfo = "off" in the release profile. But we should check that we still get backtraces when using ./mzcompose --dev run ... with unpacked debuginfo. I have a bad feeling that backtraces will break because the DWOs won't make it into the built images.

@benesch
Copy link
Contributor

benesch commented Nov 23, 2023

+1 on re-enabling incremental mode, though! If you want to split that out into its own PR I think we could merge immediately.

@def-
Copy link
Contributor Author

def- commented Nov 23, 2023

If dwp/dwo files are not supported by Polar Signals that's unfortunate. Probably mostly for local usage then.

But we should check that we still get backtraces when using ./mzcompose --dev run ... with unpacked debuginfo. I have a bad feeling that backtraces will break because the DWOs won't make it into the built images.

I'll verify.

@benesch
Copy link
Contributor

benesch commented Nov 23, 2023

If dwp/dwo files are not supported by Polar Signals that's unfortunate.

They're basically not supported as a distribution mechanism by anything. They're not usable with debuginfod either, for example. Just dug up one of the Rust discussions on this: rust-lang/rust#105991

def- added 2 commits November 23, 2023 18:22
See https://doc.rust-lang.org/rustc/codegen-options/index.html#split-debuginfo

This should lead to:
- faster scratch build (4:12 -> 4:03)
- smaller executable (2.7 GB -> 1.6 GB environmentd)
- reduced disk usage (39 GB -> 32 GB target dir)
- reduced memory usage during compilation (1.83 GB -> 0.08 GB max RSS in environmentd linking)

See for example https://www.productive-cpp.com/improving-cpp-builds-with-split-dwarf/
The previous problem seems to have been cargo clippy, see https://materializeinc.slack.com/archives/CMH6PG4CW/p1694572440379559

Now that we enforce Rust 1.74 the issue seems to be fixed.
@def- def- force-pushed the pr-split-debuginfo-unpacked branch from 7ab2730 to 6dc7afe Compare November 23, 2023 18:22
@def-
Copy link
Contributor Author

def- commented Nov 23, 2023

diff --git a/src/environmentd/src/bin/environmentd/main.rs b/src/environmentd/src/bin/environmentd/main.rs
index c6e2a0dcbc..80cfe3751c 100644
--- a/src/environmentd/src/bin/environmentd/main.rs
+++ b/src/environmentd/src/bin/environmentd/main.rs
@@ -621,6 +621,7 @@ fn aws_secrets_controller_key_alias(env_id: &EnvironmentId) -> String {
 }

 fn main() {
+    panic!("foo");
     let args = cli::parse_args(CliConfig {
         env_prefix: Some("MZ_"),
         enable_version_flag: true,
bin/mzcompose --find testdrive --dev run default

Seems to work:

testdrive-materialized-1     | thread 'main' panicked at src/environmentd/src/bin/environmentd/main.rs:624:5:
testdrive-materialized-1     | foo
testdrive-materialized-1     | stack backtrace:
testdrive-materialized-1     |    0: rust_begin_unwind
testdrive-materialized-1     |              at ./rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:597:5
testdrive-materialized-1     |    1: core::panicking::panic_fmt
testdrive-materialized-1     |              at ./rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:72:14
testdrive-materialized-1     |    2: environmentd::main
testdrive-materialized-1     |              at ./home/deen/git/materialize2/src/environmentd/src/bin/environmentd/main.rs:624:5
testdrive-materialized-1     |    3: <fn() as core::ops::function::FnOnce<()>>::call_once
testdrive-materialized-1     |              at ./rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/ops/function.rs:250:5
testdrive-materialized-1     | note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@def- def- merged commit 811ee27 into MaterializeInc:main Nov 23, 2023
@def- def- deleted the pr-split-debuginfo-unpacked branch November 23, 2023 22:05
@umanwizard
Copy link
Contributor

Even if we can't use it in release builds, decreasing the incremental debug build time by 10x is hugely valuable. Thanks for finding this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants