Skip to content
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

Bypass rustup wrapper when invoking rustc #10986

Closed
davidlattimore opened this issue Aug 14, 2022 · 19 comments · Fixed by #11917
Closed

Bypass rustup wrapper when invoking rustc #10986

davidlattimore opened this issue Aug 14, 2022 · 19 comments · Fixed by #11917
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@davidlattimore
Copy link

davidlattimore commented Aug 14, 2022

Problem

I was just using flamegraph to profile a clean cargo build and observed that more than 4% of time was being spent parsing rustup configuration files. For a clean cargo check it was about 7%.

This seems to be because when you run cargo, it invokes rustup which then figures out which actual cargo binary to invoke (fine so far), then cargo invokes rustc many times and each time it does so, it's invoking rustup again, which has to go and reparse configuration files to figure out which rustc binary to invoke.

flamegraph

See rustup_init::main on the right (7.54%)

This was produced by running:

sudo apt install mold
cargo install flamegraph
git clone https://github.com/ebobby/simple-raytracer.git
cd simple-raytracer
cargo clean
flamegraph -- mold -run cargo check

To see the potential savings of bypassing rustup, we can set the RUSTC environment variable to point directly at rustc.

With rustup:

hyperfine --prepare 'cargo clean' 'cargo check'
Benchmark 1: cargo check
  Time (mean ± σ):      7.029 s ±  0.046 s    [User: 22.295 s, System: 2.782 s]
  Range (min … max):    6.939 s …  7.107 s    10 runs

Bypassing rustup:

RUSTC=`rustup which rustc` hyperfine --prepare 'cargo clean' 'cargo check'
Benchmark 1: cargo check
  Time (mean ± σ):      6.418 s ±  0.040 s    [User: 19.979 s, System: 2.409 s]
  Range (min … max):    6.347 s …  6.481 s    10 runs

So about a 9.5% speedup. I'd expect for crates with relatively few, large dependencies, the time spent by rustup would be less as a percentage. For crates with lots of small dependencies it could be more.

For a trivial (hello world) binary, a warm cargo check with a single line change is 101.5ms. If we set RUSTC to bypass rustup, this drops to 67.6ms.

The above times were all on my laptop. For an extra datapoint, I tried building nushell on a relatively powerful desktop with lots of RAM and CPUs. Cold cargo check went from 23.061 s ±  0.169 s to 22.313 s ±  0.188 s (3.4% speedup). A warm cargo check (with trivial one line change) went from 613.6 ms ±   3.6 ms to 582.3 ms ±   7.4 ms (5.4% speedup).

Steps performed by cargo to determine what rustc to run:

  1. The value of the environment variable RUSTC if that's set
  2. The value of build.rustc from .cargo/config.toml if set
  3. placeholder for extra step added by proposed solution and most alternatives
  4. Just run rustc and find it from $PATH

Other tools (e.g rustdoc) follow the same pattern.

rustc is actually a little different in that steps 1 and 2 are first performed for rustc_wrapper and rustc_workspace_wrapper. This doesn't affect the proposed solution or the alternatives.

Proposed solution

Add step 3: Use tool (e.g. rustc) from the directory that contains the current cargo binary in preference to using PATH.

Draft commit - still needs testing, updating of tests etc.

The main change to behaviour, would be a scenario like the following:

  • A rust toolchain exists in /aaa
  • A rust toolchain exists in /bbb
  • PATH=/aaa:/bbb or PATH=/aaa - i.e. /aaa is the first or only toolchain on the path.
  • User runs /bbb/cargo build
  • Before, /bbb/cargo would have invoked /aaa/rustc because that's what's on the PATH
  • After the proposed change, /bbb/cargo would invoke /bbb/rustc

While any change has the potential to break someone, the new behaviour actually seems less surprising to me. From the perspective of someone who only uses cargo, rustc could be considered an internal implementation detail and one might expect invoking cargo via an explicit path to use the associated rustc and other related tools.

Alternatives considered

Alternative 1 - make rustup faster

This alone probably doesn't get us all the speedup we'd like. Even if we sped up rustup by a factor of 2, we'd still be spending a significant amount of time once we invoke it once for every rustc invocation.

Alternative 2 - change rustup to set the RUSTC environment variable

This would break crates that set build.rustc in .cargo/config.toml since the value in RUSTC would override it. So this isn't really an option.

Alternative 3a - rustup sets new environment variable that's then used by cargo

Rustup could set DEFAULT_RUSTC, which would be like RUSTC but would, if set, be used at step 3.

A downside is that this option treats rustc differently to the other tools that rustup wraps - unless we also add environment variables for other tools as well. e.g. DEFAULT_RUSTDOC - but that is messy and verbose.

Alternative 3b - environment variable is the directory containing the tools

Similar to alternative 3a, but instead of having the new environment variable point to RUSTC, have it point to the directory. This would allow all tools to be treated consistently.

One downside of this option (and 3a) is that if the user bypasses the rustup wrapper by explicitly invoking cargo, then cargo would revert to invoking rustup for every call to rustc.

Alternative 4 - cargo could run rustup which rustc once

If cargo ran rustup which rustc, it could then (if the command succeeded) use the result at step 3.

We'd still be running rustup twice - once to determine which cargo binary to invoke, then again within cargo. It's a lot better than invoking it N times though.

Notes

No response

@davidlattimore davidlattimore added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Aug 14, 2022
@Muscraft
Copy link
Contributor

If I remember correctly rust-lang/rustup#2626 is related to this and was fixed in rustup version 1.25.0 but had to be reverted in 1.25.1. rust-lang/rustup#3035 should be the related tracking issue.

@epage
Copy link
Contributor

epage commented Aug 15, 2022

If its possible to do this in cargo, I wonder if this is another viable option for solving the problem.

@CAD97
Copy link
Contributor

CAD97 commented Aug 17, 2022

🤔

Alternative 4a - run rustc --print rustc once

Add a new rustc command, rustc --print rustc (or similar; add a path to -Vv?), which just prints out the path to the executable. This avoids cargo needing to have any knowledge about how rustup functions (i.e. running rustup which or knowing the directory layout puts cargo next to rustc). Additionally, by not assuming rustup, things will work properly with other ways of multiboxing multiple toolchains. The main advantage being potentially working for mixed-toolchain workflows (like when using a local cargo running rustup's rustc, or any other local path hijacking of the tools).

@bjorn3
Copy link
Member

bjorn3 commented Aug 17, 2022

Add a new rustc command, rustc --print rustc (or similar; add a path to -Vv?), which just prints out the path to the executable.

I like this one. The --print rustc could be combined with the other --print usages such that it doesn't even cost an extra invocation of rustc. I think it should be called --print rustc-path or something like that though. --print rustc could also be interpreted as printing the same as --version.

The main advantage being potentially working for mixed-toolchain workflows

Yeah, when using a linked toolchain, the default cargo is combined with the local rustc. This would break for the solution proposed by @davidlattimore, but works just fine for all alternatives I think.

@CAD97
Copy link
Contributor

CAD97 commented Aug 17, 2022

This seems simple enough that I just went ahead and started implementing it: rust-lang/rust#100681; #10998

@ehuss
Copy link
Contributor

ehuss commented Aug 17, 2022

I personally would lean towards option 3b. I wouldn't think the scenario of directly executing cargo without rustup, but wanting rustc to be driven by rustup is too common (mostly applicable to those developing cargo itself?). Rustup already knows where things are, it might as well just inform the tools.

Another option I don't see mentioned is for rustup to alter PATH to put the toolchain first. Rustup used to work like that at one point, but then it was removed due to rust-lang/rustup#809. However, I didn't see in that discussion why rustup didn't just prepend the actual toolchain directory first, instead of the cargo fallback toolchain. If that is viable, I might prefer that since then no tools need to even bother with knowing how to set things up.

@the8472
Copy link
Member

the8472 commented Aug 17, 2022

A much more complicated option 5: Implement a fork server for rustc. This has the advantage that commonly used data (e.g. std metadata) can be preloaded before fork. This would not only save the rustup startup time but also additional time spent by rustc to initialize various things.

@bjorn3
Copy link
Member

bjorn3 commented Aug 17, 2022

I don't think having a fork server supersedes this change as presumably the executable talking to the fork server would also be behind the rustup shim.

@the8472
Copy link
Member

the8472 commented Aug 17, 2022

Depends on how you talk to the fork-server. If cargo can open a socket to it to send commands then there's no further process spawning overhead (other than forks).

@CAD97
Copy link
Contributor

CAD97 commented Aug 17, 2022

I don't think Windows has an equivalent to a fork server. Windows system libraries aren't capable of being forked into two the way that Unix system libraries are.

Of course, you can do something like a fork server, where you spawn a new process and give it a copy of the dynamic set up state over a channel/port/socket/whatever. Doing so as a "fork server" is perhaps better than doing so from the outside world. But the benefit over just doing the regular startup is much diminished.

@the8472
Copy link
Member

the8472 commented Aug 17, 2022

Windows has to have a fork implementation to support WSL1 and previously SFU. But I don't know what that would do to win32 processes.

@CAD97
Copy link
Contributor

CAD97 commented Aug 17, 2022

Yes, the kernel supports a fork. It's the system libraries which will fall over and cause problems. https://stackoverflow.com/a/62888362

For example, the system DLLs kernel32.dll and user32.dll maintain a private connection to the Win32 server process csrss.exe. After a fork, there are two processes on the client end of that connection, which is going to cause problems. The child process should inform csrss.exe of its existence and make a new connection – but there's no interface to do that, because these libraries weren't designed with fork() in mind.

@bjorn3
Copy link
Member

bjorn3 commented Aug 17, 2022

Also WSL1 uses picoprocesses as originating from project drawbridge, not regular NT processes.

@CAD97
Copy link
Contributor

CAD97 commented Aug 17, 2022

WSL is a unix, so has fork(). However, MSVC is a tier-one target, and should not be ignored just because it's different from Unixy targets and that's inconvenient.

@the8472
Copy link
Member

the8472 commented Aug 17, 2022

@CAD97
Copy link
Contributor

CAD97 commented Aug 17, 2022

https://twitter.com/tiraniddo/status/1100151078677610496?s=20&t=gZ-8Q2Op73DA5IuBRFeYbg

On the NT side it's not a working fork, it doesn't necessarily copy all memory across or file handles, or a working connection to CSRSS. The only supported use I know is to create a process copy to create a crash dump. WSL is different as it has full control to do what it needs. [--James Forshaw @tiraniddo

https://twitter.com/AmarSaar/status/1100182770175877121?s=20&t=gZ-8Q2Op73DA5IuBRFeYbg

Rtl!RtlCloneUserProcess clones the executing process. The new child process has an identical address space AND it begins from the exact same spot. Many other resources may not be fully copy, but it still cool to know this:) [-- Saar Amar @AmarSaar]

It exists, but it's an undocumented API surface. Again, just because you can doesn't mean that anything is going to work if you try to talk to the OS, which rustc very much needs to do.

@davidlattimore
Copy link
Author

Another option I don't see mentioned is for rustup to alter PATH to put the toolchain first. Rustup used to work like that at one point, but then it was removed due to rust-lang/rustup#809. However, I didn't see in that discussion why rustup didn't just prepend the actual toolchain directory first, instead of the cargo fallback toolchain. If that is viable, I might prefer that since then no tools need to even bother with knowing how to set things up.

One potential issue with doing something like that is that it would affect any nested invocations of cargo or rustc. e.g. if someone runs cargo or rustc from their build.rs, they might not get the tools they were expecting.

@CAD97
Copy link
Contributor

CAD97 commented Aug 18, 2022

Or also the initial reason rust-lang/rustup#2958 was reverted; people have been doing cargo +toolchain from within build.rs, and expect that to work. Making cargo resolve to not the rustup shim breaks that. (If buildscripts were doing $CARGO +toolchain, imho that's on them, though.)

ChrisDenton added a commit to ChrisDenton/rust that referenced this issue Aug 20, 2022
…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 😅)
@davidlattimore
Copy link
Author

I like that alternative 4a works even if you bypass the rustup wrapper when invoking cargo. It's unfortunate though that for the common use-case of running cargo build via the rustup wrapper, that rustup still needs to run twice - once to locate cargo, then a second time to locate rustc.

Option 3b would allow the outer rustup (cargo wrapper) to do the work once, then it wouldn't need to be repeated in order to locate rustc, so we'd only have one rustup invocation. Unfortunately 3b would mean that if you invoked cargo directly, not via the wrapper, then all the rustc invocations would go via the wrapper.

It's a shame that there doesn't seem to be a single option that has all the following properties:

  • Doesn't break linked toolchains
  • Invokes rustup wrapper only once
  • Works when invoking cargo directly rather than via a rustup wrapper

I guess doing 4a now doesn't preclude also doing 3b later in order to avoid the second rustup invocation. Or perhaps once 4a is done, the next thing would be to try to speed up rustup itself so that the second (and first) invocation doesn't matter so much.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 21, 2022
…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 😅)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 21, 2022
…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 😅)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 21, 2022
…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 😅)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 21, 2022
…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 😅)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
7 participants