Turn off debuginfo for build dependencies to improve compile times #10493

lqd · 2022-03-21T14:41:29Z

This PR is a draft towards the possible state of build-override defaults described in #10481, in order to improve compile times for build dependencies (mostly proc-macros and their dependencies benefit from this):

debuginfo is turned off by default (stripping and incremental are less impactful and can be done as improvements in future PRs, since they also require more analysis and work) to match this comment and zulip message
the new defaults are documented, and explain how to turn it back on when needed
when a build script fails, and backtraces are requested, a custom note mentions how to improve backtraces and links to the documentation

Opening as draft for feedback and guidance:

on the build script error message itself (it tries to only show when applicable, when backtraces are opted into, but I wonder if that interferes with how diagnostics are usually buffered). Update: the current approach and message look acceptable.
a bunch of tests rely on the fact that dependencies and build dependencies will not be rebuilt under the current defaults: now that the dev profile defaults and dev.build-override differ on whether debuginfo is turned on, these dependencies will be built twice. To leave the tests as is, either debuginfo could be turned off, or manually set to 2, which I've done in the "tmp: update tests relying on dev and build-deps reuse" commit. I am not sure it's the expected way to do this ? Update: the test changes look acceptable so I've turned the temporary commit into a permanent one.
I need some help for -Zscrape-examples: the feature seems to depend implicitly on a similar build reuse, with a mapping from for-host dependencies to regular dependencies. I'm not sure that this works correctly in all situations: it seems focused on feature flags, but reuse can also be different depending on other flags. In our case, debuginfo is now different, making some of the mapping memoization panic, causing the tests to fail. So I've temporarily disabled them in the "tmp: disable -Zscrape-examples unit tests" commit, and would need help to know what to do there. My expectation is that these panics would already happen today if someone manually opted into different debuginfo level in their build-override (or panic method, etc) but have not tested it (it seems sensible in a context where only the default settings have changed to a value users can already set). Update: This issue is now tracked in -Z rustdoc-scrape-examples panics with some profile overrides #10500. We've discussed it with @willcrichton, and I've incorporated a fix and tests in Fix docscrape memoization #10524.

Edit: with the updates above, this is ready to take out of draft.

The discussion issue #10481 also mentions the possible small strip addition that this PR doesn't do (and that could be done in the future), but since it's not a huge improvement, I'd say this closes #10481.

rust-highfive · 2022-03-21T14:41:33Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

bjorn3 · 2022-03-21T15:53:03Z

src/cargo/core/profiles.rs

            profile.opt_level = InternedString::new("0");
            profile.codegen_units = None;
+            profile.debuginfo = None;


Should this also add strip = "debuginfo" on all platforms but macOS?

the cargo team asked not to have strip for now in this message.

(incidentally, a friend tried the defaults I suggested and his project saw weird errors about unstable -Z flags being used on a stable compiler, because of stripping on android, so I'm not sure that all targets can use it rn)

src/cargo/core/compiler/custom_build.rs

joshtriplett · 2022-03-21T19:46:45Z

This looks great, thank you!

The approach to showing the message seems reasonable to me.

For tests that want to verify something only gets built once, turning on debug info in build-override seems reasonable.

-Zscrape-examples is an unstable feature; I'd suggest reaching out to the developers of -Zscrape-examples to work out what to do here. I don't think this feature should depend on reusing dependencies, if at all possible.

lqd · 2022-03-21T20:12:45Z

Thanks Josh, I'll do so ASAP.

(I've only now noticed I didn't open this PR as a draft GH PR as I intended to)

lqd · 2022-03-23T17:36:26Z

I updated the commits about:

fixing the tests relying on reuse, as josh said it looked OK
ignoring the scrape examples tests surfacing an existing problem, as I've opened a dedicated issue for that and it's not related to this PR

CI will fail because of #10500, and I'd assume this PR is kind of blocked on a resolution for that, otherwise -Z rustdoc-scrape-examples will likely stop working altogether.

lqd · 2022-03-29T08:58:00Z

-Zscrape-examples is an unstable feature; I'd suggest reaching out to the developers of -Zscrape-examples to work out what to do here.

I've reached out to @willcrichton about #10500, and turns out they had already fixed it amongst some of the changes in #10343.

#10343 may involve more coordination with rustc/rustdoc, waiting for stabilization, etc., compared to this PR so I've incorporated the minimum change to fix the issue here (with proper attribution) in case it's merged first, and added a few of the #10500 cases as tests.

With that fixed, and CI passing, this looks ready for review.

ehuss · 2022-03-30T21:37:10Z

Can you split the docscrape change into a separate PR? I would prefer to track and review it separately.

lqd · 2022-03-30T21:59:49Z

@ehuss sure. I've split the commits into #10524.

Turning off debuginfo makes a noticeable difference. Backtraces in build scripts, and opting into backtraces for proc-macros on nightly, can be worse. Debuggability can be improved by changing settings back, and this will be documented for the rare cases where it's needed.

This describes the new defaults for build-overrides, and how to make sure backtraces have the usual debug info, when needed.

it's only displayed when backtraces are requested

it displays an additional message on how to improve these backtraces, now that debuginfo is turned off by default in `dev.build-override`.

lqd · 2022-04-05T08:46:11Z

Now that #10533 has landed, CI passes on this PR.

ehuss · 2022-05-17T03:53:10Z

OK, sorry, I finally got around to posting the data:

https://docs.google.com/spreadsheets/d/1lru9ibjHLaXdFbROFIZXcvzXBKDDewGXjRYuvwJ0Xp8/edit?usp=sharing

To read this:

"Units Diff" - This is the number of extra rustc invocations.
"jxx" - This is the concurrency used.
"mean Factor" - This is the average factor of the wall-clock of several runs comparing before and after the PR. Example, 0.5 means this PR runs twice as fast as before the PR.
"user Factor" - The factor of the user-space time comparing before and after.
"system Factor" - The factor of the system-space time comparing before and after.

There's two sets of data. The one up top is from a Linux system was 16/32 CPUs. The bottom one is from a macOS system with 6/12 CPUs (it has fewer projects since I didn't want to wait for the rest).

There are little arrows in the headers where you can click to show hidden columns that contain the raw data.

This includes some projects that were intentionally selected to have skewed results (those that have a high count of shared dependencies) in order to examine some potentially poor scenarios. All of them should have some proc-macros or build scripts. I find it difficult to select "real world" projects, since they tend to be difficult to find, get access to, build correctly, or take too long to build.

@rust-lang/cargo Does anyone have any thoughts or conclusions you want to draw from this information? I'm still on the fence. It does look like wall clock time is improved in many situations, with a few situations made significantly worse like cargo-crev.

I feel like opportunistically sharing dependencies would be better, but I also feel like Cargo's "share things if possible" logic is already quite complex, and adding more logic to it would make it worse.

epage · 2022-05-17T12:26:59Z

If I'm reading this correctly, it looks like the relative improvement is small except for

most cargo check runs
toml-rs and gluons test --no-run runs

I'm assuming cargo check made a big difference because build runs have the builds dominate, taking up a larger percentage. At first, I wondered about special profiles just for cargo check to make it even faster because of how often it is run but fresh-build check times don't matter as much as repeat runs.

I'm curious why we saw a difference with test --no-run in those two cases. We build more targets which includes more linking which I assume would have dominated things even more than build does over check, making the relative gains even smaller. So why is it doing better? Oh, are test targets built "for the host", so we aren't putting in debug info which saves us on link time which is dominating due to a lot of small test binaries? Is this a bug with the implementation as I assume people will be debugging their tests and want debug info? This would instead be helped if we explored automatic collection into a single test binary, mimicing what cargo does with its tests/testsuite.

weihanglo · 2022-05-17T13:49:10Z

I find it difficult to select "real world" projects, since they tend to be difficult to find, get access to, build correctly, or take too long to build.

Any chance to collect more results from the new "Call For Testing" section in TWiR?

lqd · 2022-05-17T17:25:32Z

Oh, are test targets built "for the host", so we aren't putting in debug info which saves us on link time which is dominating due to a lot of small test binaries? Is this a bug with the implementation as I assume people will be debugging their tests and want debug info?

It is my understanding that test targets are not considered "for the host", no, nor that their defaults are changed by this PR. Only build dependencies are targeted.

I'm curious why we saw a difference with test --no-run in those two cases.

For toml-rs, for example, the reason you're seeing a noticeable improvement is that many of the proc-macros dependencies are enabled for tests only, and the total number of crates built with debuginfo is reduced from 20 to 10 (all the popular proc-macros libraries and their build scripts). That's the PR's goal, reducing the growing cost of building all these proc-macro dependencies whose debuginfo one cannot easily use in the first place, and its expected improvements when there are few shared dependencies. syn proc-macro2 quote etc. noticeably benefit from removing the debuginfo, and that translates well to their users.

Just to clarify, a high number of dependencies shared between the build-dependency subgraph and the dependency subgraph (a situation that I personally would think is less common than low-to-no sharing) is the only case where there can be regressions, and they can all be fixed by setting today's default value, to opt out of the debuginfo removal.

[profile.dev.build-override]
debug = 2

The crates that would be improved by this PR (in my results, or Eric's above) could equally well apply a debuginfo-removal override to their Cargo.toml and see the benefits. So the question looks to be more about which is the better default, to fall into the "pit of success" as we know users will rarely change defaults.

epage · 2022-05-17T17:34:57Z

For toml-rs, for example, the reason you're seeing a noticeable improvement is that many of the proc-macros dependencies are enabled for tests only, and the total number of crates built with debuginfo is reduced from 20 to 10 (all the popular proc-macros libraries and their build scripts

Ah, I had missed that most of those dependencies were introduced exclusively for testing, thanks for finding the actual root cause!

So the question looks to be more about which is the better default, to fall into the "pit of success" as we know users will rarely change defaults.

I agree. I feel we should prioritize individuals, local and CI. Large projects or companies will devote more resources to optimizing things.

lqd · 2023-02-01T22:29:09Z

Closing this PR in favor of #11252.

Turn off debuginfo for build dependencies v2 This PR is an alternative to #10493, fixing its most important issue: the unit graph optimization to reuse already built artifacts for dependencies shared between the build-time and runtime subgraphs is preserved (most of the time). By deferring the default debuginfo level in `dev.build-override` until unit graph sharing, we check whether re-use would happen. If so, we use the debuginfo level to ensure reuse does happen. Otherwise, we can avoid emitting debuginfo and improve compile times (on clean, debug and check builds -- although reuse only happens on debug builds). I've kept the message explaining how to bump the debuginfo level if an error occurs while running a build script (and backtraces were requested) that was in the previous PR. I've ran benchmarks on 800 crates, at different parallelism levels, and published the (surprisingly good) results with visualizations, summaries, and raw data [here](https://github.com/lqd/rustc-benchmarking-data/tree/main/experiments/cargo-build-defaults). Opening this PR as discussed in [Eric's message](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Defaults.20for.20faster.20compilation.20of.20.22for.20host.22.20units/near/304236576l) as draft since 2 tests won't pass. That fixes the `cargo-crev` case we saw as a blocker last time, but doesn't fix all such cases of reuse, like the 2 failing tests: - [`optional_build_dep_and_required_normal_dep`](https://github.com/rust-lang/cargo/blob/642a0e625d10099a0ca289827de85499d073c572/tests/testsuite/build_script.rs#L4449) - and [`proc_macro_ws`](https://github.com/rust-lang/cargo/blob/bd5db301b0c45ae540afcb19e030dd7c29d2ea4f/tests/testsuite/features2.rs#L1051) These failures make sense, since the debuginfo optimization is done during traversal, it depends on the contents of the unit graph. These tests ensure that sharing happens even on finer-grained invocations: respectively, with an optional shared dependency that is later enabled, and building shared dependencies by themselves first and later as part of a complete workspace build. In both these situations, there is no unit that is shared in the graph during the first invocation, so we do the optimization and remove the debuginfo. When the graph changes in the following invocation, sharing is present and we have to build the shared units again with debuginfo. These cases feel rarer than `cargo-crev`'s case, but I do wonder if there's a way to fix them, or if it's acceptable to not support them.

rust-highfive assigned ehuss Mar 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2022

lqd mentioned this pull request Mar 21, 2022

Discussion of defaults settings for build dependencies for fastest compile times #10481

Open

bjorn3 reviewed Mar 21, 2022

View reviewed changes

src/cargo/core/compiler/custom_build.rs Outdated Show resolved Hide resolved

lqd force-pushed the build-defaults branch from 56c5c10 to 1e95a15 Compare March 21, 2022 16:31

lqd changed the title ~~Turn off debuginfo for build dependencies to improve compile times~~ [WIP] Turn off debuginfo for build dependencies to improve compile times Mar 21, 2022

lqd marked this pull request as draft March 21, 2022 20:13

lqd force-pushed the build-defaults branch from 1e95a15 to 0b32184 Compare March 23, 2022 17:32

nnethercote mentioned this pull request Mar 28, 2022

-Z rustdoc-scrape-examples panics with some profile overrides #10500

Closed

lqd force-pushed the build-defaults branch 2 times, most recently from e19fe05 to fdab02e Compare March 29, 2022 08:57

lqd changed the title ~~[WIP] Turn off debuginfo for build dependencies to improve compile times~~ Turn off debuginfo for build dependencies to improve compile times Mar 29, 2022

lqd marked this pull request as ready for review March 29, 2022 09:41

lqd mentioned this pull request Mar 30, 2022

Fix docscrape memoization #10524

Closed

lqd force-pushed the build-defaults branch from fdab02e to 1407237 Compare March 30, 2022 21:58

lqd added 7 commits April 5, 2022 09:49

update compiler_json_error_format test for new build-deps defaults

8d8448c

update build_cmd_with_a_build_cmd test for new build-deps defaults

b160046

update tests relying on dev and build-deps reuse

8ab2c69

update reuse_panic_pm test for new build-deps defaults

596ae96

update named_config_profile test for new build-deps defaults

e55a70d

update profile_targets tests for new build-deps defaults

86cba71

lqd added 3 commits April 5, 2022 09:49

update build dependencies profiles documentation

e586544

This describes the new defaults for build-overrides, and how to make sure backtraces have the usual debug info, when needed.

display note to increase debuginfo level when build deps fail

835978b

it's only displayed when backtraces are requested

add build script failure test when requesting backtraces

d5f1078

it displays an additional message on how to improve these backtraces, now that debuginfo is turned off by default in `dev.build-override`.

lqd force-pushed the build-defaults branch from 1407237 to d5f1078 Compare April 5, 2022 07:49

weihanglo mentioned this pull request Oct 13, 2022

Don't use incremental compilation for build scripts #8545

Open

lqd mentioned this pull request Oct 17, 2022

Turn off debuginfo for build dependencies v2 #11252

Merged

lqd closed this Feb 1, 2023

lqd deleted the build-defaults branch February 1, 2023 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn off debuginfo for build dependencies to improve compile times #10493

Turn off debuginfo for build dependencies to improve compile times #10493

lqd commented Mar 21, 2022 •

edited

Loading

rust-highfive commented Mar 21, 2022

bjorn3 Mar 21, 2022

lqd Mar 21, 2022 •

edited

Loading

joshtriplett commented Mar 21, 2022

lqd commented Mar 21, 2022

lqd commented Mar 23, 2022 •

edited

Loading

lqd commented Mar 29, 2022

ehuss commented Mar 30, 2022

lqd commented Mar 30, 2022

lqd commented Apr 5, 2022

ehuss commented May 17, 2022 •

edited

Loading

epage commented May 17, 2022

weihanglo commented May 17, 2022

lqd commented May 17, 2022 •

edited

Loading

epage commented May 17, 2022

lqd commented Feb 1, 2023

Turn off debuginfo for build dependencies to improve compile times #10493

Turn off debuginfo for build dependencies to improve compile times #10493

Conversation

lqd commented Mar 21, 2022 • edited Loading

rust-highfive commented Mar 21, 2022

bjorn3 Mar 21, 2022

Choose a reason for hiding this comment

lqd Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

joshtriplett commented Mar 21, 2022

lqd commented Mar 21, 2022

lqd commented Mar 23, 2022 • edited Loading

lqd commented Mar 29, 2022

ehuss commented Mar 30, 2022

lqd commented Mar 30, 2022

lqd commented Apr 5, 2022

ehuss commented May 17, 2022 • edited Loading

epage commented May 17, 2022

weihanglo commented May 17, 2022

lqd commented May 17, 2022 • edited Loading

epage commented May 17, 2022

lqd commented Feb 1, 2023

lqd commented Mar 21, 2022 •

edited

Loading

lqd Mar 21, 2022 •

edited

Loading

lqd commented Mar 23, 2022 •

edited

Loading

ehuss commented May 17, 2022 •

edited

Loading

lqd commented May 17, 2022 •

edited

Loading