Resolve rustc path early to bypass rustup wrapper #10998
Conversation
|
r? @epage (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
This increases the amount of rustc invocations before building anything from 3 to 4: It would be nice if this could be reduced a bit. Maybe allowing |
|
I agree that this would be ideal1; however, Footnotes
|
I think that will hurt reproducible builds as I think the entire |
|
It would be nice to see perf results on these changes. Going from 3 -> 4 |
src/cargo/util/rustc.rs
Outdated
| // In order to avoid calling through rustup multiple times, we first ask | ||
| // rustc to give us the "resolved" rustc path, and use that instead. If | ||
| // this doesn't give us a path, then we just use the original path such | ||
| // that the following logic can handle any resulting errors normally. | ||
| let mut cmd = ProcessBuilder::new(&path); |
There was a problem hiding this comment.
I would also like sign off from rustup folks on this being a viable alternative to rust-lang/rustup#3035 (ie they like it enough not to continue down that path, making this obsolete)
Reproducing the flamegraph from #10986 would be the ideal way to see the impact. Unfortunately I'm on Windows so I don't think I can do that. A completely unscientific N=4 test that likely loses any signal in noise (e6bd7a6):simple-raytracer〉for $i in 1..4 { cargo clean; benchmark { rustup run stage1 cargo check -q } }
╭───┬────────────────────────╮
│ 0 │ 8sec 584ms 527µs 600ns │
│ 1 │ 8sec 658ms 529µs 200ns │
│ 2 │ 8sec 611ms 257µs 700ns │
│ 3 │ 8sec 614ms 52µs 200ns │
╰───┴────────────────────────╯
simple-raytracer〉for $i in 1..4 { cargo clean; benchmark { RUSTC=D:\.rust\rustup\toolchains\stage1\bin\rustc.exe rustup run stage1 cargo check -q } }
╭───┬────────────────────────╮
│ 0 │ 8sec 220ms 422µs 400ns │
│ 1 │ 9sec 88ms 127µs 800ns │
│ 2 │ 9sec 119ms 748µs 500ns │
│ 3 │ 9sec 97ms 148µs 700ns │
╰───┴────────────────────────╯
simple-raytracer〉for $i in 1..4 { cargo clean; benchmark { rustup run stage1 ./cargo.exe check -q } }
╭───┬────────────────────────╮
│ 0 │ 8sec 868ms 90µs 600ns │
│ 1 │ 8sec 722ms 460µs 200ns │
│ 2 │ 8sec 544ms 25µs 600ns │
│ 3 │ 8sec 912ms 617µs 400ns │
╰───┴────────────────────────╯
Yeah, likely just noise. A second try: simple-raytracer〉open .\bench.nu
rustup override set stage1
rustup show
let iter_count = 4
for _ in 1..$iter_count { cargo clean; benchmark { cargo check -q } } | math avg
with-env { RUSTC: $"(rustup which rustc | str trim)" } {
for _ in 1..$iter_count { cargo clean; benchmark { cargo check -q } } | math avg
}
for _ in 1..$iter_count { cargo clean; benchmark { ./cargo.exe check -q } } | math avg
rustup override remove
simple-raytracer〉source .\bench.nu
info: override toolchain for 'D:\git\ebobby\simple-raytracer' set to 'stage1'
Default host: x86_64-pc-windows-msvc
rustup home: D:\.rust\rustup
installed toolchains
--------------------
stable-x86_64-pc-windows-msvc (default)
nightly-2022-06-21-x86_64-pc-windows-msvc
nightly-x86_64-pc-windows-msvc
1.61-x86_64-pc-windows-msvc
stage0
stage1
1.46.0-x86_64-pc-windows-msvc
1.49.0-x86_64-pc-windows-msvc
1.53.0-x86_64-pc-windows-msvc
1.56.1-x86_64-pc-windows-msvc
active toolchain
----------------
stage1 (directory override for 'D:\git\ebobby\simple-raytracer')
rustc 1.65.0-dev
8sec 812ms 985µs 825ns
8sec 814ms 123µs 525ns
8sec 848ms 928µs 275ns
I'm clearly just measuring noise. Slightly better, N=128This is using locally compiled versions of 9809f8f (main) and e6bd7a6 (patched), so as fair a comparison as I can possibly get.
Something's wrong with the implementation to see opposite duration deltas from setting $RUSTC and from using --print. I'm hoping @davidlattimore can reproduce their benchmark from #10986 for us. (However, I've run enough tests to pretty clearly see that how this is currently implemented isn't accomplishing the stated goal. I spotted the antimalware service executable in Task Manager causing a significant portion of the CPU load since this is the Windows filesystem I'm working on...) |
|
With a further adjustment to run N=10,
N=64,
This seems to show that now this is a consistent win, and the overhead of the extra |
I have just made a very interesting observation: ~〉rustup run stable rustc --version --print=not-a-valid-print -Cnot-a-codegen
rustc 1.63.0 (4b91a6ea7 2022-08-08)i.e. I did try a local patch to use patchescargodiff --git a/src/cargo/util/rustc.rs b/src/cargo/util/rustc.rs
index 46c763108..3fc88d1dc 100644
--- a/src/cargo/util/rustc.rs
+++ b/src/cargo/util/rustc.rs
@@ -54,28 +54,27 @@ impl Rustc {
cache_location,
);
- // In order to avoid calling through rustup multiple times, we first ask
- // rustc to give us the "resolved" rustc path, and use that instead. If
- // this doesn't give us a path, then we just use the original path such
- // that the following logic can handle any resulting errors normally.
let mut cmd = ProcessBuilder::new(&rustc);
- cmd.arg("--print=rustc-path");
- if let Ok((stdout, _)) = cache.cached_output(&cmd, 0) {
- let resolved = PathBuf::from(stdout.trim());
- if resolved.exists() {
- rustc = resolved;
- cache.reset(
- wrapper.as_deref(),
- workspace_wrapper.as_deref(),
- &rustc,
- rustup_rustc,
- );
- }
- }
+ cmd.arg("-Zunstable-options")
+ .arg("--print=verbose-version")
+ .arg("--print=rustc-path");
+ let mut identifying = cache.cached_output(&cmd, 0)?.0;
- let mut cmd = ProcessBuilder::new(&rustc);
- cmd.arg("-vV");
- let verbose_version = cache.cached_output(&cmd, 0)?.0;
+ // In order to avoid calling through rustup multiple times, we asked
+ // rustc to give us the "resolved" rustc path, and use that from now on.
+ let resolved_path = identifying.lines().next_back().unwrap();
+ rustc = PathBuf::from(resolved_path);
+ // We need to also update the cache with the resolved path.
+ cache.reset(
+ wrapper.as_deref(),
+ workspace_wrapper.as_deref(),
+ &rustc,
+ rustup_rustc,
+ );
+ // Truncate the identifying string to remove the `--print=rustc-path`,
+ // leaving just the verbose version as if we had called `rustc -vV`.
+ identifying.truncate(identifying.len() - (1 + resolved_path.len()));
+ let verbose_version = identifying;
let extract = |field: &str| -> CargoResult<&str> {
verbose_versionrustcdiff --git a/compiler/rustc_driver/src/lib.rs b/compiler/rustc_driver/src/lib.rs
index 2e678cd54fd..df4990d9390 100644
--- a/compiler/rustc_driver/src/lib.rs
+++ b/compiler/rustc_driver/src/lib.rs
@@ -729,6 +729,10 @@ fn print_crate_info(
Ok(exe) => println!("{}", exe.display()),
Err(_) => early_error(ErrorOutputType::default(), "failed to get rustc path"),
},
+ RustcVersion(verbose) => {
+ let verbose_codegen_backend = if verbose { Some(codegen_backend) } else { None };
+ print_version_info("rustc", verbose_codegen_backend)
+ }
// Any output here interferes with Cargo's parsing of other printed output
NativeStaticLibs => {}
LinkArgs => {}
@@ -740,10 +744,23 @@ fn print_crate_info(
/// Prints version information
pub fn version(binary: &str, matches: &getopts::Matches) {
let verbose = matches.opt_present("verbose");
+ let codegen_backend;
+ let verbose_codegen_backend = if verbose {
+ let debug_flags = matches.opt_strs("Z");
+ let backend_name = debug_flags.iter().find_map(|x| x.strip_prefix("codegen-backend="));
+ codegen_backend = get_codegen_backend(&None, backend_name);
+ Some(&*codegen_backend)
+ } else {
+ None
+ };
+ print_version_info(binary, verbose_codegen_backend);
+}
+
+pub fn print_version_info(binary: &str, verbose_codegen_backend: Option<&dyn CodegenBackend>) {
println!("{} {}", binary, util::version_str().unwrap_or("unknown version"));
- if verbose {
+ if let Some(codegen_backend) = verbose_codegen_backend {
fn unw(x: Option<&str>) -> &str {
x.unwrap_or("unknown")
}
@@ -753,9 +770,7 @@ fn unw(x: Option<&str>) -> &str {
println!("host: {}", config::host_triple());
println!("release: {}", unw(util::release_str()));
- let debug_flags = matches.opt_strs("Z");
- let backend_name = debug_flags.iter().find_map(|x| x.strip_prefix("codegen-backend="));
- get_codegen_backend(&None, backend_name).print_version();
+ codegen_backend.print_version();
}
}
diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs
index 8b77fb7be29..ee830063417 100644
--- a/compiler/rustc_session/src/config.rs
+++ b/compiler/rustc_session/src/config.rs
@@ -546,6 +546,7 @@ pub enum PrintRequest {
StackProtectorStrategies,
LinkArgs,
RustcPath,
+ RustcVersion(bool),
}
pub enum Input {
@@ -1769,6 +1770,7 @@ fn collect_print_requests(
error_format: ErrorOutputType,
) -> Vec<PrintRequest> {
let mut prints = Vec::<PrintRequest>::new();
+
if cg.target_cpu.as_ref().map_or(false, |s| s == "help") {
prints.push(PrintRequest::TargetCPUs);
cg.target_cpu = None;
@@ -1778,6 +1780,20 @@ fn collect_print_requests(
cg.target_feature = String::new();
}
+ let gated = |req, opt| {
+ if unstable_opts.unstable_options {
+ req
+ } else {
+ early_error(
+ error_format,
+ &format!(
+ "the `-Z unstable-options` flag must also be passed to \
+ enable the {opt} print option"
+ ),
+ )
+ }
+ };
+
prints.extend(matches.opt_strs("print").into_iter().map(|s| match &*s {
"crate-name" => PrintRequest::CrateName,
"file-names" => PrintRequest::FileNames,
@@ -1792,19 +1808,11 @@ fn collect_print_requests(
"tls-models" => PrintRequest::TlsModels,
"native-static-libs" => PrintRequest::NativeStaticLibs,
"stack-protector-strategies" => PrintRequest::StackProtectorStrategies,
- "target-spec-json" => {
- if unstable_opts.unstable_options {
- PrintRequest::TargetSpec
- } else {
- early_error(
- error_format,
- "the `-Z unstable-options` flag must also be passed to \
- enable the target-spec-json print option",
- );
- }
- }
+ "target-spec-json" => gated(PrintRequest::TargetSpec, "target-spec-json"),
"link-args" => PrintRequest::LinkArgs,
- "rustc-path" => PrintRequest::RustcPath,
+ "rustc-path" => gated(PrintRequest::RustcPath, "rustc-path"),
+ "version" => gated(PrintRequest::RustcVersion(false), "version"),
+ "verbose-version" => gated(PrintRequest::RustcVersion(true), "verbose-version"),
req => early_error(error_format, &format!("unknown print request `{req}`")),
}));
|
…enkov Add `rustc --print rustc-path` Related: - rust-lang/cargo#10986 - rust-lang/rustup#3035 Goal: Like the original rust-lang/rustup#2958, the goal is to enable `cargo` to call `rustc` directly, rather than through the `rustup` shim. Solution: `cargo` asks `rustc` to tell it what executable to run. Sometime early in compilation, `cargo` will run `$(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path`[^1]. Further calls to `rustc` to do execution will use the resolved printed executable path rather than continuing to call the "input `rustc` path," which will allow it to bypass the `rustup` shim. The cargo side is rust-lang/cargo#10998. [^1]: This can potentially be combined with other `--print`s, as well! This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP. cc `@davidlattimore` `@bjorn3` `@rust-lang/cargo` `@rust-lang/compiler` (sorry for the big ping list 😅)
This allows existing tests to work without stubbing --print=rustc-path.
CAD97
force-pushed
the
rustc-print-rustc
branch
from
e76de76 to
30acc36
Compare
|
(Rebase was unnecessary, I just saw the GitHub button for it and pressed it while trying to remember how to access the convert to draft functionality.) |
|
Per my comment at #10986 (comment), I would prefer to see this resolved on the rustup side, and pass the information to cargo. There were some concerns raised about running cargo directly without rustup, but I do not think that should be a common enough occurrence to worry about. Rustup already knows the answer for the toolchain directory, and it should be relatively easy to pass to any tool. Is that something that can be pursued instead? |
…enkov Add `rustc --print rustc-path` Related: - rust-lang/cargo#10986 - rust-lang/rustup#3035 Goal: Like the original rust-lang/rustup#2958, the goal is to enable `cargo` to call `rustc` directly, rather than through the `rustup` shim. Solution: `cargo` asks `rustc` to tell it what executable to run. Sometime early in compilation, `cargo` will run `$(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path`[^1]. Further calls to `rustc` to do execution will use the resolved printed executable path rather than continuing to call the "input `rustc` path," which will allow it to bypass the `rustup` shim. The cargo side is rust-lang/cargo#10998. [^1]: This can potentially be combined with other `--print`s, as well! This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP. cc `@davidlattimore` `@bjorn3` `@rust-lang/cargo` `@rust-lang/compiler` (sorry for the big ping list 😅)
…enkov Add `rustc --print rustc-path` Related: - rust-lang/cargo#10986 - rust-lang/rustup#3035 Goal: Like the original rust-lang/rustup#2958, the goal is to enable `cargo` to call `rustc` directly, rather than through the `rustup` shim. Solution: `cargo` asks `rustc` to tell it what executable to run. Sometime early in compilation, `cargo` will run `$(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path`[^1]. Further calls to `rustc` to do execution will use the resolved printed executable path rather than continuing to call the "input `rustc` path," which will allow it to bypass the `rustup` shim. The cargo side is rust-lang/cargo#10998. [^1]: This can potentially be combined with other `--print`s, as well! This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP. cc ``@davidlattimore`` ``@bjorn3`` ``@rust-lang/cargo`` ``@rust-lang/compiler`` (sorry for the big ping list 😅)
…enkov Add `rustc --print rustc-path` Related: - rust-lang/cargo#10986 - rust-lang/rustup#3035 Goal: Like the original rust-lang/rustup#2958, the goal is to enable `cargo` to call `rustc` directly, rather than through the `rustup` shim. Solution: `cargo` asks `rustc` to tell it what executable to run. Sometime early in compilation, `cargo` will run `$(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path`[^1]. Further calls to `rustc` to do execution will use the resolved printed executable path rather than continuing to call the "input `rustc` path," which will allow it to bypass the `rustup` shim. The cargo side is rust-lang/cargo#10998. [^1]: This can potentially be combined with other `--print`s, as well! This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP. cc ```@davidlattimore``` ```@bjorn3``` ```@rust-lang/cargo``` ```@rust-lang/compiler``` (sorry for the big ping list 😅)
…enkov Add `rustc --print rustc-path` Related: - rust-lang/cargo#10986 - rust-lang/rustup#3035 Goal: Like the original rust-lang/rustup#2958, the goal is to enable `cargo` to call `rustc` directly, rather than through the `rustup` shim. Solution: `cargo` asks `rustc` to tell it what executable to run. Sometime early in compilation, `cargo` will run `$(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path`[^1]. Further calls to `rustc` to do execution will use the resolved printed executable path rather than continuing to call the "input `rustc` path," which will allow it to bypass the `rustup` shim. The cargo side is rust-lang/cargo#10998. [^1]: This can potentially be combined with other `--print`s, as well! This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP. cc ````@davidlattimore```` ````@bjorn3```` ````@rust-lang/cargo```` ````@rust-lang/compiler```` (sorry for the big ping list 😅)
|
I'm closing this PR since I don't have the bandwidth to pursue furthering this change. I personally still like this approach because it's completely agnostic to if/how rustc is being multiversioned and doesn't require setting up a hierarchy of multiple environment variables checked to determine what fun factIf this saves 30ms per codegen unit, napkin math says it'd take 21 crate builds a day for this to save me an hour of time over a full year. The cost-benefit for pushing this PR doesn't pan out for me. |
This is the cargo side of rust#100681.
What does this PR try to resolve?
Fixes #10986, obsoletes(?) rustup#3035
The goal is to bypass the
rustupwrapper when invokingrustcmultiple times. See #10986 for more context.How should we test and review this PR?
Relies on rust-lang/rust#100681.
A local
rustup run stage1 ($env.CARGO_TARGET_DIR | path join release/cargo.exe) build12 succeeded in building, though I'm terribly unsure how to go about testing that this is because--print=rustc-pathis being respected, especially since I allow--print=rustc-pathto fail and stick to the old behavior.Additional information
cc @davidlattimore, @bjorn3
Footnotes
nushell shell syntax ↩
Actually, the test run was without the
if output.status.success()check, which I added afterwards. ↩