Turn off debuginfo for build dependencies v2

[lqd]

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.

Opening this PR as discussed in Eric's message 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
• and proc_macro_ws

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]

r? @weihanglo

(rust-highfive has picked a reviewer for you, use r? to override)

[weihanglo]

Thank you for the experiment!

For the proc_macro_ws case, I don't think there is a feasible way to fix, since it needs to additionally query workspace info even when building only one specific package. In reality, top most used proc macros crates often have a little amount of dependencies. Those dependencies are typically for proc macro only, so it may not be a concern.

For optional_build_dep_and_required_normal_dep, I wonder which kind of crate is frequently used by both runtime and build time. It sounds rare to me as well. It is IMO not a problem can be simply fixed with only a few lines of code, so probably not worth doing so?

[weihanglo]

unit.kind.is_host() probably works, but it feels like a misuse.

CompileKind::is_host tells whether a unit compiles for host or target platform. By coincidence, build-deps seems to always true. What if it is not the case in the future. For example, build deps with artifact dependencies can be specified on a target platform.

[bjorn3]

A build dep with artifact dependencies should keep debuginfo I think as it may end up embedded in the main program.

[weihanglo]

If they are from build deps, I feel like it not correct a main program depending on them at run time. If one do want to embed an artifact dep, the user should specify artifact deps in normal deps instead.

Do you have any particular use case to share that a build dep gets embedded in the final executable?

[bjorn3]

If a build script needs to post-process an artifact to for example generate bindings using wasm-bindgen, it has to be specified as build dependency, right?

[weihanglo]

Sounds pretty valid and common to me. Thank you!

Then I'd suggest not nuking debuginfo from any kind of artifact dep, since we don't know users' intents.

[lqd]

Yes agreed, here I was trying to find the is_for_host that UnitFor has and the Profile sees to set defaults and so on.

But since the UnitDep is available when we recurse, I've pushed a commit to use UnitFor::is_for_host instead of CompileKind::is_host. That should be better, right ?

(I've also pushed a commit to not optimize debuginfo for artifact dependencies, but I'm not sure if that's what you all meant, nor what kind of test we should add to ensure that though, any thoughts ? I'd also think that if someone wanted to post-process an artifact in a build script and required debuginfo, they would specify the level they need in Cargo.toml: they already won't have any debuginfo for build deps when building in release mode today)

[lqd]

For the proc_macro_ws case [...]

For optional_build_dep_and_required_normal_dep [...]

Would we want to remove both these tests, or encode that these cases would not see the debuginfo optimization, and see the build dependencies optimized in the previous builds are rebuilt with debuginfo instead (since they are shared build/runtime dependencies in the unit graph of the later build commands) ?

[weihanglo]

For the proc_macro_ws case [...]
For optional_build_dep_and_required_normal_dep [...]

Would we want to remove both these tests, or encode that these cases would not see the debuginfo optimization …

I lean toward having them removed. They are not special anymore in terms of optimization. Even they accidentally get optimized, they will not be a regression IMO.

[weihanglo]

So far so good. It's in a good shape toward merging 👍🏾

Kindly ask @ehuss for taking a look if he has time, as this is where Eric's expertise shines, and he always catches something we missed :)

[lqd]

Thanks a bunch for the review @weihanglo 🙏

I lean toward having them removed. They are not special anymore in terms of optimization. Even they accidentally get optimized, they will not be a regression IMO.

Done, I've pushed their removal. It'll be easy to bring them back and change them if Eric wants to anyways. We'll see what he says :)

[bors]

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

[ehuss]

Thanks! This seems like it should be good to move forward with. Sorry for the super long delays.

I am a little uncomfortable with the increased complication around the sharing code. But I think this should be OK. I'm not even sure if cargo will keep the same sharing strategy long term. The sharing is already disabled in several situations, so it gets harder to justify it.

[ehuss]

I'm not clear why this test is being removed. It seems like it was added for a specific issue when mixing an optional with a non-optional dep (#5487). Can you say why it is being removed instead of just updating the test?

[weihanglo]

I guess it was my idea #11252 (comment). Can bring them back if needed.

[lqd]

I've brought the test back, and updated it to show that it doesn't get the build sharing optimization in this situation since there is no shared dependency in the first command.

[ehuss]

Same question about why this is being removed.

[lqd]

I've brought the test back, and explicitly opted into having debuginfo generated, so that the intent of this non-regression test is preserved, without the debuginfo optimization triggering.

[ehuss]

A couple suggestions here to make this a little more user friendly:

Can this print the environment variable the user needs to set? Looking up documentation and modifying Cargo.toml may be a bit onerous just to display a better backtrace. I think it can be done with something like:

// Outside the closure:
let env_profile_name = unit.profile.name.to_uppercase();
// ...
// Inside this error message:
format!("Set the CARGO_PROFILE_{env_profile_name}_BUILD_OVERRIDE_DEBUG=true environment variable to enable debug generation.\n")

The other suggestion is to only show this extra message if debug is actually turned off. Perhaps something like this:

// Outside the closure:
// I think this is roughly correct, but please scrutinize it.
let built_with_debug = cx
    .bcx
    .unit_graph
    .get(unit)
    .and_then(|deps| deps.iter().find(|dep| dep.unit.target == unit.target))
    .map(|dep| dep.unit.profile.debuginfo.is_turned_on())
    .unwrap_or(false);
// Inside the closure:
if !built_with_debug && show_backtraces != "0" {
   // ...
}

WDYT?

[lqd]

Done, and done. Let me know if you like the wording.

Thanks for the snippet, as I'm sure I would have tried looking at unit.profile.debuginfo first and would have been confused as to why the message didn't appear when expected.