Compute quoted args for debuginfo at most once per session #146973
Conversation
|
Some changes occurred in compiler/rustc_codegen_ssa |
rustbot
added
A-LLVM
rustbot
added
the
T-compiler
|
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Compute quoted args for debuginfo at most once per session
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
| /// forms of debuginfo (specifically PDB). | ||
| /// | ||
| /// This needs to _not_ be a query, because it would ruin incremental | ||
| /// compilation for CGUs that would otherwise be considered up-to-date. |
There was a problem hiding this comment.
If the commandline changed, then we should recompile all CGUs as otherwise the commandline arguments in the debuginfo would be wrong. If it doesn't change, then no CGUs would be recompiled even if it was a query. If unconditionally recompiling cgus when the cli changes is not acceptable, then maybe this entire feature should be put behind a cli flag and be disabled by default?
There was a problem hiding this comment.
As far as I can tell, bypassing the query system is already the current behaviour. So if that's a problem, maybe the whole “command-line in PDB” thing needs to be ripped out until it can be re-landed in an acceptable way.
(I don't have any attachment to the feature myself; I'm just trying to make the compiler do this quoting step once per process instead of literally 500+ times for no good reason.)
There was a problem hiding this comment.
As far as I can tell, bypassing the query system is already the current behaviour.
Yeah, I don't think anyone considered this when landing the original version.
There was a problem hiding this comment.
There's also the semi-related #128842, where the EXE path being embedded in PDB is reportedly troublesome.
There was a problem hiding this comment.
Yeah, I guess we should either rip out this feature or put it behind a flag.
There was a problem hiding this comment.
I opened https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Some.20PDB.20info.20bypasses.20the.20query.20system.20and.20path.20remapping/with/541369247 to ask if anyone else has opinions.
My preference is to just rip out the whole thing and wait for someone to complain, since it's having outsized impact relative to its niche use-case, and we have no idea whether anyone is actually benefiting from it.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ffa991c): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 471.082s -> 469.259s (-0.39%) |
rustbot
removed
the
S-waiting-on-perf
|
Closing this while we see how #147022 shakes out. |
rustbot
removed
the
S-waiting-on-review
Remove current code for embedding command-line args in PDB The compiler currently has code that will obtain a list of quoted command-line arguments, and pass it through to TargetMachine creation, so that the command-line args can be embedded in PDB output. This PR removes that code, due to subtle concerns that might not have been apparent when it was originally added. --- Those concerns include: - The entire command-line quoting process is repeated every time a target-machine-factory is created. In incremental builds this typically occurs 500+ times, instead of happening only once. The repeated quoting constitutes a large chunk of instructions executed in the `large-workspace` benchmark. - See #146804 (comment) for an example of the perf consequences of skipping all that work. - This overhead occurs even when building for targets or configurations that don't emit PDB output. - Command-line arguments are obtained in a way that completely bypasses the query system, which is a problem for the integrity of incremental compilation. - Fixing this alone is likely to inhibit incremental rebuilds for most or all CGUs, even in builds that don't emit PDB output. - Command-line arguments and the executable path are obtained in a way that completely bypasses the compiler's path-remapping system, which is a reproducibility hazard. - #128842 --- Relevant PRs: - #113492 - #130446 - #131805 - #146700 - #146973 Zulip thread: - https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Some.20PDB.20info.20bypasses.20the.20query.20system.20and.20path.20remapping/with/541432211 --- According to #96475, one of the big motivations for embedding the command-line arguments was to enable tools like Live++. [It appears that Live++ doesn't actually support Rust yet](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/embeded.20compiler.20args.20and.20--remap-path-prefix/near/523800010), so it's possible that there aren't any existing workflows for this removal to break. In the future, there could be a case for reintroducing some or all of this functionality, guarded behind an opt-in flag so that it doesn't cause problems for other users. But as it stands, the current implementation puts a disproportionate burden on other users and on compiler maintainers.
Remove current code for embedding command-line args in PDB The compiler currently has code that will obtain a list of quoted command-line arguments, and pass it through to TargetMachine creation, so that the command-line args can be embedded in PDB output. This PR removes that code, due to subtle concerns that might not have been apparent when it was originally added. --- Those concerns include: - The entire command-line quoting process is repeated every time a target-machine-factory is created. In incremental builds this typically occurs 500+ times, instead of happening only once. The repeated quoting constitutes a large chunk of instructions executed in the `large-workspace` benchmark. - See rust-lang/rust#146804 (comment) for an example of the perf consequences of skipping all that work. - This overhead occurs even when building for targets or configurations that don't emit PDB output. - Command-line arguments are obtained in a way that completely bypasses the query system, which is a problem for the integrity of incremental compilation. - Fixing this alone is likely to inhibit incremental rebuilds for most or all CGUs, even in builds that don't emit PDB output. - Command-line arguments and the executable path are obtained in a way that completely bypasses the compiler's path-remapping system, which is a reproducibility hazard. - rust-lang/rust#128842 --- Relevant PRs: - rust-lang/rust#113492 - rust-lang/rust#130446 - rust-lang/rust#131805 - rust-lang/rust#146700 - rust-lang/rust#146973 Zulip thread: - https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Some.20PDB.20info.20bypasses.20the.20query.20system.20and.20path.20remapping/with/541432211 --- According to rust-lang/rust#96475, one of the big motivations for embedding the command-line arguments was to enable tools like Live++. [It appears that Live++ doesn't actually support Rust yet](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/embeded.20compiler.20args.20and.20--remap-path-prefix/near/523800010), so it's possible that there aren't any existing workflows for this removal to break. In the future, there could be a case for reintroducing some or all of this functionality, guarded behind an opt-in flag so that it doesn't cause problems for other users. But as it stands, the current implementation puts a disproportionate burden on other users and on compiler maintainers.
5 participants
EDIT: There is now a proposal to remove the underlying code entirely, which would make this PR obsolete.
Via #146700 and #146845, it was discovered that we were building multiple identical copies of these quoted command-line args for every CGU, instead of just building them 0-1 times for the whole compilation session.
Memoizing them turned out to be harder than expected, for two reasons:
This PR therefore computes the quoted args via a hook, backed by an explicit OnceLock stored in TyCtxt.
One of the side-effects of this PR is that we also no longer supply command-line arguments when creating an informational target machine; we only supply them when creating one that will (presumably) be used for actual codegen.