Optimize usage under rustup. #11917

ehuss · 2023-03-31T02:02:00Z

This optimizes cargo when running under rustup to circumvent the rustup proxies. The rustup proxies introduce overhead that can make a noticeable difference.

The solution here is to identify if cargo would normally run rustc from PATH, and the current rustc in PATH points to something that looks like a rustup proxy (by comparing it to the rustup binary which is a hard-link to the proxy). If it detects this situation, then it looks for a binary in $RUSTUP_HOME/toolchains/$TOOLCHAIN/bin/$TOOL. If it finds the direct toolchain executable, then it uses that instead.

Considerations

There have been some past attempts in the past to address this, but it has been a tricky problem to solve. This change has some risk because cargo is attempting to guess what the user and rustup wants, and it may guess wrong. Here are some considerations and risks for this:

Setting RUSTC (as in Set RUSTC and RUSTDOC env for child processes run through the proxy rustup#2958) isn't an option. This makes the RUSTC setting "sticky" through invocations of different toolchains, such as a cargo subcommand or build script which does something like cargo +nightly build.
Changing PATH isn't an option, due to issues like rustup 1.25: On Windows, nested cargo invocation with a toolchain specified fails rustup#3036 where cargo subcommands would be unable to execute proxies (so things like +toolchain shorthands don't work).
Setting other environment variables in rustup (as in Add RUSTUP_TOOLCHAIN_DIR rustup#3207 which adds RUSTUP_TOOLCHAIN_DIR the path to the toolchain dir) comes with various complications, as there is risk that the environment variables could get out of sync with one another (like with RUSTUP_TOOLCHAIN), causing tools to break or become confused.

There was some consideration in that PR for adding protections by using an encoded environment variable that could be cross-checked, but I have concerns about the complexity of the solution.

We may want to go with this solution in the long run, but I would like to try a short term solution in this PR first to see how it turns out.
This won't work for a rustup-toolchain.toml override with a path setting. Cargo will use the slow path in that case. In theory it could try to detect this situation, which may be an exercise for the future.
Some build-scripts, proc-macros, or custom cargo subcommands may be doing unusual things that interfere with the assumptions made in this PR. For example, a custom subcommand could call a cargo executable that is not managed by rustup. Proc-macros may be executing cargo or rustc, assuming it will reach some particular toolchain. It can be difficult to predict what unusual ways cargo and rustc are being used. This PR (and its tests) tries to make extra sure that it is resilient even in unusual circumstances.
The "dev" fallback in rustup can introduce some complications for some solutions to this problem. If a rustup toolchain does not have cargo, such as with a developer "toolchain link", then rustup will automatically call either the nightly, beta, or stable cargo if they are available. This PR should work correctly, since rustup sets the correct RUSTUP_TOOLCHAIN environment variable for the actual toolchain, not the one where cargo was executed from.
Special care should be considered for dynamic linking. LD_LIBRARY_PATH (linux), DYLD_LIBRARY_PATH (macos), and PATH (windows) need to be carefully set so that rustc can find its shared libraries. Directly executing rustc has some risk that it will load the wrong shared libraries. There are some mitigations for this. macOS and Linux use rpath, and Windows looks in the same directory as rustc.exe. Also, rustup configures the dyld environment variables from the outer cargo. Finally, cargo also configures these (particularly for the deprecated compiler plugins).
This shouldn't impact installations that don't use rustup.
I've done a variety of testing on the big three platforms, but certainly nowhere exhaustive.
- One of many examples is making sure Clippy's development environment works correctly, which has special requirements for dynamic linking.
There is risk about future rustup versions changing some assumptions made here. Some assumptions:
- It assumes that if RUSTUP_TOOLCHAIN is set, then the proxy always runs exactly that toolchain and no other. If this changes, cargo could execute the wrong version. Currently RUSTUP_TOOLCHAIN is the highest priority toolchain override and is fundamental to how toolchain selection becomes "sticky", so I think it is unlikely to change.
- It assumes rustup sets RUSTUP_TOOLCHAIN to a value that is exactly equal to the name of the toolchain in the toolchains directory. This works for user shorthands like RUSTUP_TOOLCHAIN=nightly, which gets converted to the full toolchain name. However, it does not work for path overrides (see above).
- It assumes the toolchains directory layout is always $RUSTUP_HOME/toolchains/$TOOLCHAIN. If this changes, then I think the only consequence is that cargo will go back to the slow path.
- It assumes downloading toolchains is not needed (since cargo running from the toolchain means it should already be downloaded).
- It assumes there is no other environment setup needed (such as the dyld paths mentioned above).
My hope is that if assumptions are no longer valid that the worst case is that cargo falls back to the slow path of running the proxy from PATH.

Performance

This change won't affect the performance on Windows because rustup currently alters PATH to point to the toolchain directory. However, rust-lang/rustup#3178 is attempting to remove that, so this PR will be required to avoid a performance penalty on Windows. That change is currently opt-in, and will likely take a long while to roll out since it won't be released until after the next release, and may be difficult to get sufficient testing.

I have done some rough performance testing on macOS, Windows, and Linux on a variety of different kinds of projects with different commands. The following attempts to summarize what I saw.

The timings are going to be heavily dependent on the system and the project. These are the values I get on my systems, but will likely be very different for everyone else.

The Windows tests were performed with a custom build of rustup with rust-lang/rustup#3178 applied and enabled (stock rustup shows no change in performance as explained above).

The data is summarized in this spreadsheet: https://docs.google.com/spreadsheets/d/1zSvU1fQ0uSELxv3VqWmegGBhbLR-8_KUkyIzCIk21X0/edit?usp=sharing

hello-world has a particularly large impact of about 1.68 to 2.7x faster. However, a large portion of this overhead is related to running rustc at the start to discover its version and querying it for information. This is cached after the first run, so except for first-time builds, the effect isn't as noticeable. The "check with info" row is an benchmark that removes target/debug/deps but keeps the .rustc_info.json file.

Incremental builds are a bit more difficult to construct since it requires customizing the commands for each project. I only did an incremental test for cargo itself, running touch src/cargo/lib.rs and then cargo check --lib.

These measurements excluded the initial overhead of launching the rustup proxy to launch the initial cargo process. This was done just for simplicity, but it makes the test a little less characteristic of a typical usage, which will have some constant overhead for running the proxy.

These tests were done using hyperfine version 1.16.1. The macOS system was an M2 Max (12-thread). The Windows and Linux experiments were run on a AMD Ryzen Threadripper 2950X (32-thread). Rust 1.68.2 was used for testing. I can share the commands if people want to see them.

ehuss · 2023-03-31T02:02:10Z

cc @rbtcollins

rustbot · 2023-03-31T02:02:15Z

Failed to set assignee to weihanglo: cannot assign: HTTP status server error (502 Bad Gateway) for url (https://api.github.com/repos/rust-lang/cargo/issues/11917/assignees)

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

rustbot · 2023-03-31T02:02:16Z

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

joshtriplett · 2023-03-31T03:26:10Z

src/cargo/util/config/mod.rs

+                // use hard links to a single binary. If rustup ever changes
+                // that setup, then I think the worst consequence is that this
+                // optimization will not work, and it will take the slow path.
+                if tool_meta.len() != rustup_meta.len() {


This seems hazardous. It's entirely possible for two binaries to have the same length without having the same contents. If you're checking for hardlinks, shouldn't this compare ino and dev, at least on Unix? (I'm not sure how to improve this on Windows; are we using Windows hardlinks where available?)

Following up: according to @ChrisDenton, you need to open both files, and then while both are open, call GetFileInformationByHandleEx and make sure both volume and ID are identical.

Essentially you'd need to compare FILE_ID_INFO structs. Though there are some nuances here and the point about keeping both file handles open is crucial because ids aren't otherwise guaranteed to be stable (see [MS-FSCC] reference). See also this LLVM bug.

That's possible, but I'm concerned about the potential complexity or difficulty in getting that right. There are various situations where symlinks are used, and I can't guarantee that the files won't end up as a copy, or have issues across network mounts, for example. The file sizes are currently an order of magnitude different, and I think the chance for them being the same is very unlikely.

I'm not sure if this matters but the reason the fs footprint of rustup looks quite different in different places is because of android (no hardlink support), bew (everything is a symlink to a symlink from our next release) (and I guess snap and other 3rd-party distributions can also differ to what one might expect from looking at our code).

In particular see rust-lang/rustup#3137 for some context.

tl;dr: there is no guarantee that the proxy and rustup itself are the same file, even though that is our default installation logic.

On android they are symlinks.
On MacOSX with brew they are symlinks from our next release.

And the consequence of the tool not being a proxy is that someone has deliberately placed e.g. a 'rustc' wrapper that does something, which cargo would then not run.

Checking for the same length will detect every common situation where they are different except for two cases I can see: two different binaries, alike in length, and two different symlinks, alike in length.

For binaries I agree - its very unlikely that two different binaries the same length as rustup is large enough that the law of small numbers doesn't really apply.

For symlinks, I suggest doing a readlink on the file. It is cheap enough to still be a lot faster than rustup manifest parsing (which I plan to do something about someday, but its not top of the list, and even after, not running code we don't need to run is how we make things fast).

Oh and the final case - I alluded to above with 'common' - lets exclude other file types from consideration. Special node types should just immediate take the slow path.

For symlinks, I suggest doing a readlink on the file. It is cheap enough to still be a lot faster than rustup manifest parsing (which I plan to do something about someday, but its not top of the list, and even after, not running code we don't need to run is how we make things fast).

Can you say more about why this would be needed? If the proxy symlink points at rustup, shouldn't they have the same size?

It is possible for the symlink to point at something else with the same length path.

The chance of two unrelated binary lengths colliding when they are ~11M in size (current rustup-init release size on Windows) is pretty low. But the chance of two ~100 byte paths being the same length is much much higher, and then multiply that out by our growing user bases I think its worth mitigating the risk.

Maybe I'm still unclear, but this uses the standard metadata function which reads the target of a symlink (recursively). If rustup and rustc are symlinks to different things (with the same length path), they'll still have different length targets.

If that doesn't resolve your concern, can you show a specific example? For example:

/usr/bin/rustc -> /usr/bin/rust-compiler
/usr/bin/rustup -> /usr/bin/rustup-thingy
/usr/bin/rust-compiler 669176
/usr/bin/rustup-thingy 8027337

These have the same length paths, but different length targets, so they should be treated as being different.

ok, so you're using fs::metadata(path).len(), not symlink_metadata ? Then I'm fine with that as-is

epage · 2023-03-31T13:06:36Z

src/cargo/util/config/mod.rs

+                // This is an optimization to circumvent the rustup proxies
+                // which can have a significant performance hit. The goal here
+                // is to determine if calling `rustc` from PATH would end up
+                // calling the proxies.
+                //
+                // This is somewhat cautious trying to determine if it is safe
+                // to circumvent rustup, because there are some situations
+                // where users may do things like modify PATH, call cargo
+                // directly, use a custom rustup toolchain link without a
+                // cargo executable, etc. However, there is still some risk
+                // this may make the wrong decision in unusual circumstances.
+                //
+                // First, we must be running under rustup in the first place.


This feels a lot more complicated and brittle to me than #10998. In reviewing #10998, the only downside I saw listed was that it wasn't being driven by rustup which this has the same problem.

Is there something I'm missing for why we'd prefer this route over #10998?

Querying rustc is certainly a possibility, but the approach taken there incurs additional startup time for an initial cache. Adding a new flag to rustc has a fairly high bar, but adding a transparent optimization in cargo has no user-facing interaction so should be easier to move forward with. So, in terms of the global complexity (changes to cargo, rustc, and/or rustup, user-facing documentation, etc.), this seemed like the simplest solution with the least risk, and can receive benefits immediately rather than waiting a potentially very long time.

If this solution ends up having issues that one of the other solutions could address, then I think it would be worthwhile to re-investigate a different approach.

Adding a new flag to rustc has a fairly high bar

Even if its just a new enumerated value for an existing flag?

If this solution ends up having issues that one of the other solutions could address, then I think it would be worthwhile to re-investigate a different approach.

If I understand correctly, this solution could start failing and we'd never know it, right?

Even if its just a new enumerated value for an existing flag?

Yea, new options are almost always added as an unstable option, and then it needs to go through the process of making a case for the compiler team to stabilize.

If I understand correctly, this solution could start failing and we'd never know it, right?

It is possible, though I think unlikely in most cases. I think any major regressions would require a significant change in the design of rustup, and I think that is unlikely for the foreseeable future. I could add a test that requires rustup to be installed if that may help with that concern.

I can add in that rustup has been consulted and we quite like this approach.

I definitely prefer a solution like this that doesn't add any additional invocations of rustc.

rbtcollins · 2023-04-01T06:41:50Z

src/cargo/util/config/mod.rs

+                // First, we must be running under rustup in the first place.
+                let toolchain = self.get_env_os("RUSTUP_TOOLCHAIN")?;
+                // If the tool on PATH is the same as `rustup` on path, then
+                // there is pretty good evidence that it will be a proxy.


We have an exact list of the proxies we offer btw. I think it is a good idea to only take the fastpath for things we are known to proxy. I'm happy to commit to keeping a copy of that list in Cargo up to date. New proxies are very rare.

Cargo is currently hard-coded to only use this for rustc and rustdoc. I added an assert to validate that requirement, and I think we can extend it in the future if needed. I don't think we quite yet need to have an exhaustive list for all the proxies (just to keep things simple for now).

rbtcollins · 2023-04-01T07:20:55Z

I'm in favour of this as discussed.

One note - I'm looking to sharply constrain toolchain path based toolchains in the next release - removing the relative-path support entirely (or at least feature-flagging it to drive some feedback), and in a later release I hope to remove it. There's no tracking bug yet but some discord discussion.

All of which to say, I don't think you should worry about path based toolchains, other than rejecting toolchains that contain '/' or ''.

rbtcollins · 2023-04-01T07:22:26Z

src/cargo/util/config/mod.rs

+                let toolchain_exe = home::rustup_home()
+                    .ok()?
+                    .join("toolchains")
+                    .join(&toolchain)


This could look at pretty random places on the filesystem with a path based toolchain. I suggest breaking out of this logic early based on toolchain == 'none' || toolchain.contains('/') || toolchain.contains('\\')

Good catch! I have added that.

rbtcollins · 2023-04-27T05:30:28Z

Another complication but probably ok:

Users can specify RUSTUP_TOOLCHAIN=nightly : this is part of the Rustup UI. When a proxy is run with that set, the toolchain is resolved to e.g. nightly-x86_64-pc-windows-msvc and then the child process (e.g. cargo) will receive RUSTUP_TOOLCHAIN=nightly-x86_64-pc-windows-msvc which will be an actual directory in ~/.rustup/toolchains.

I think this is ok because if someone runs cargo without the proxy, with that variable set, the directory won't exist, and the fallback path of just invoking the rustc proxy will invoke a rustup proxy, which will then perform resolution, ending up with the explicitly requested toolchain being run.

If the user is running a cargo from a different toolchain, directly, with a RUSTUP_TOOLCHAIN variable set, then I think they can keep both pieces.

ehuss · 2023-05-01T17:45:29Z

@weihanglo Just checking in to see if you have any questions or thoughts on this. I realize this PR contains a lot of words for a relatively small code change.

weihanglo · 2023-05-03T13:31:50Z

src/cargo/util/config/mod.rs

+        // assert is to ensure that if it is ever used for something else in
+        // the future that you must ensure that it is a proxy-able tool, or if
+        // not then you need to use `maybe_get_tool` instead.
+        assert!(matches!(tool, "rustc" | "rustdoc"));


(nit) Should change this to an enum instead of runtime assertion, and also update the function doc comment?

Sure, I went ahead and added an enum.

weihanglo · 2023-05-03T14:25:02Z

src/cargo/util/config/mod.rs

+                if toolchain_exe.exists() {
+                    Some(toolchain_exe)
+                } else {
+                    None
+                }


(nit)

Suggested change

if toolchain_exe.exists() {

Some(toolchain_exe)

} else {

None

}

toolchain_exe.exists().then_some(toolchain_exe)

weihanglo · 2023-05-03T15:47:09Z

src/cargo/util/config/mod.rs

+                }
+                // Try to find the tool in rustup's toolchain directory.
+                let tool_exe = Path::new(tool).with_extension(env::consts::EXE_EXTENSION);
+                let toolchain_exe = home::rustup_home()


home crate still access RUSTUP_HOME via std::env. Will there be an inconsistency when people set their RUSTUP_HOME in [env]?

cargo/crates/home/src/env.rs

Line 93 in 39684ff

match env.var_os("RUSTUP_HOME").filter(|h| !h.is_empty()) {

It shouldn't under normal circumstances, since the rustup proxies set RUSTUP_HOME, the [env] value will be ignored.

I think it would be a good idea to prohibit this before stablisation @ehuss

Sure, I went ahead and posted #12101.

… case.

weihanglo

This looks pretty good and simpler than other alternatives to me. Given that rustup folks agree on this change, Feel free to r=weihanglo if there is no further discussion needed with them.

ehuss · 2023-05-04T23:51:32Z

I think this should be ready to go. My intent is to get this out on nightly and hopefully get enough real-world testing to determine if there is a problem, and we can either fix it or back it out. Unfortunately nightly doesn't get enough testing in some of the more unusual environments, but hopefully it will be enough to be moderately confident it should be ok.

@bors r=weihanglo

bors · 2023-05-04T23:51:34Z

📌 Commit b9993bd has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-05-04T23:51:40Z

⌛ Testing commit b9993bd with merge 2d693e2...

bors · 2023-05-05T00:44:58Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 2d693e2 to master...

Update cargo 10 commits in ac84010322a31f4a581dafe26258aa4ac8dea9cd..569b648b5831ae8a515e90c80843a5287c3304ef 2023-05-02 13:41:16 +0000 to 2023-05-05 15:49:44 +0000 - xtask-unpublished: output a markdown table (rust-lang/cargo#12085) - fix: hack around `libsysroot` instead of `libtest` (rust-lang/cargo#12088) - Optimize usage under rustup. (rust-lang/cargo#11917) - Update lock to normalize `home` dep (rust-lang/cargo#12084) - fix: doc-test failures (rust-lang/cargo#12055) - feat(cargo-metadata): add `workspace_default_members` (rust-lang/cargo#11978) - doc: clarify implications of `cargo-yank` (rust-lang/cargo#11862) - chore: Use `[workspace.dependencies]` (rust-lang/cargo#12057) - support for shallow clones and fetches with `gitoxide` (rust-lang/cargo#11840) - Build by PackageIdSpec, not name, to avoid ambiguity (rust-lang/cargo#12015) r? `@ghost`

rustbot assigned weihanglo Mar 31, 2023

rustbot added A-configuration Area: cargo config files and env vars S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2023

joshtriplett reviewed Mar 31, 2023

View reviewed changes

epage reviewed Mar 31, 2023

View reviewed changes

ehuss force-pushed the rustup-circumvent branch from 839d8d6 to 7263f3b Compare March 31, 2023 15:54

rbtcollins reviewed Apr 1, 2023

View reviewed changes

ehuss force-pushed the rustup-circumvent branch from 7263f3b to f9386d9 Compare April 8, 2023 17:56

weihanglo added the A-rustup Area: rustup interaction label May 3, 2023

weihanglo reviewed May 3, 2023

View reviewed changes

ehuss added 6 commits May 3, 2023 13:45

Add some tests for simulating behavior under rustup.

52317aa

Add an optimization when running under rustup.

d4b0b49

Add an assert to confirm that this function is only used with proxies.

cdf60e3

Check for toolchains names with slashes and use the slow path in that…

42ef94f

… case.

Change get_tool to use an enum to constrain which values it accepts.

4f79ac3

Switch to then_some.

b9993bd

ehuss force-pushed the rustup-circumvent branch from f9386d9 to b9993bd Compare May 3, 2023 21:26

weihanglo approved these changes May 3, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2023

bors merged commit 2d693e2 into rust-lang:master May 5, 2023
18 checks passed

weihanglo mentioned this pull request May 5, 2023

Update cargo rust-lang/rust#111258

Merged

ehuss added this to the 1.71.0 milestone May 5, 2023

rbtcollins mentioned this pull request May 7, 2023

Disallow RUSTUP_HOME in the [env] table. #12101

Merged

ehuss mentioned this pull request May 23, 2023

Cargo will always invoke the 'true' rustc on Windows, rather than a rustc shim #5960

Closed

weihanglo mentioned this pull request Jun 20, 2023

Cargo does not use rust-toolchain.toml of separate project when executed within cargo run #12292

Closed

weihanglo mentioned this pull request Jun 29, 2023

Windows Defender causing slowdown on contents of .cargo\bin folder #5028

Open

Optimize usage under rustup. #11917

Optimize usage under rustup. #11917

Conversation

ehuss commented Mar 31, 2023

Considerations

Performance

ehuss commented Mar 31, 2023

rustbot commented Mar 31, 2023

rustbot commented Mar 31, 2023

joshtriplett Mar 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins Apr 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins Apr 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins Apr 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins commented Apr 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins commented Apr 27, 2023

ehuss commented May 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

ehuss commented May 4, 2023

bors commented May 4, 2023

bors commented May 4, 2023

bors commented May 5, 2023

joshtriplett Mar 31, 2023 •

edited

rbtcollins Apr 1, 2023 •

edited

rbtcollins Apr 10, 2023 •

edited

rbtcollins Apr 1, 2023 •

edited