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

testsuite: Improve performance when using rustup. #9206

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Feb 25, 2021

The rustup wrapper adds a considerable amount of overhead when running processes like rustc. This overrides PATH when running the testsuite to prioritize the actual executables to avoid the wrapper.

I'm not 100% confident this won't break something somewhere, but it seems like it should be safe.

In my tests, this makes the testsuite 1.5 to 1.7 times faster.

@rust-highfive
Copy link

r? @Eh2406

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2021
@Eh2406
Copy link
Contributor

Eh2406 commented Feb 25, 2021

Worth a try, we can always back it out if it breaks something.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 25, 2021

📌 Commit c73765f has been approved by Eh2406

@bors 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 Feb 25, 2021
@bors
Copy link
Contributor

bors commented Feb 25, 2021

⌛ Testing commit c73765f with merge 5d97225...

@bors
Copy link
Contributor

bors commented Feb 25, 2021

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 5d97225 to master...

@bors bors merged commit 5d97225 into rust-lang:master Feb 25, 2021
bors added a commit that referenced this pull request Feb 25, 2021
testsuite: Use split debuginfo on macos.

This switches the testsuite to use "unpacked" debuginfo on macos, which is a substantial performance boost. On my system, the testsuite runs 1.55 times faster with this change.  Along with #9206, total testsuite time is 3.1 times faster.
@ehuss
Copy link
Contributor Author

ehuss commented Feb 25, 2021

Nice, here's the change in CI:

Job Before After Change
linux stable 23m12s 14m53 1.55 times faster
linux beta 27m9s 14m43 1.84 times faster
linux nightly 22m36s 15m53 1.42 times faster
macos stable 25m51s 15m55 1.62 times faster
windows stable 26m34s 17m50 1.48 times faster
windows gnu nightly 35m28s 28m36 1.24 times faster

(Not very accurate since each run can use different hardware, but still pretty reasonable.)

@alexcrichton
Copy link
Member

Do you have a good sense for why rustup is adding so much overhead here? Is it just the overhead of having a separate process or is rustup doing something particularly slow?

@ehuss
Copy link
Contributor Author

ehuss commented Feb 25, 2021

I'm not too familiar with rustup's internals. The overhead of rustup for running rustc -V on my system is 0.129s, which when multiplied across thousands of tests (which also run rustc multiple times), that easily adds up to several minutes.

There is more discussion of this at rust-lang/rustup#2626.

From a quick look, it seems like rustup is loading the config file (using TOML which is pretty slow), and doing all sorts of manifest processing and more. Kinnison also noted there is some dylib startup cost. I don't know why it does that once RUSTUP_TOOLCHAIN is set, I would expect it to read the environment variable and skip all the work.

@alexcrichton
Copy link
Member

Phew, well at least these are some very nice improvements, thanks for investigating and measuring this @ehuss!

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 3, 2021
Update cargo

12 commits in 572e201536dc2e4920346e28037b63c0f4d88b3c..c68432f1e5cbbc09833699a951b1b5b059651dff
2021-02-24 16:51:20 +0000 to 2021-03-02 18:26:29 +0000
- Don't panic when printing JSON with non-utf8 paths (rust-lang/cargo#9226)
- Detect changes for JSON spec targets. (rust-lang/cargo#9223)
- Fix `cargo_target_empty_cfg` test with env var. (rust-lang/cargo#9225)
- Correct default cargo new edition (rust-lang/cargo#9202)
- Update split-debuginfo docs around the default. (rust-lang/cargo#9224)
- Minor update to registry API error messages. (rust-lang/cargo#9213)
- Some minor code cleanup. (rust-lang/cargo#9214)
- doc: Fix spelling worksapce->workspace (rust-lang/cargo#9212)
- Update SPDX version in docs. (rust-lang/cargo#9209)
- Throw error if CARGO_TARGET_DIR is an empty string (rust-lang/cargo#8939)
- testsuite: Use split debuginfo on macos. (rust-lang/cargo#9207)
- testsuite: Improve performance when using rustup. (rust-lang/cargo#9206)
bors added a commit that referenced this pull request Jul 12, 2021
Make it easier to run testsuite with a custom toolchain.

The optimization added in #9206 to circumvent the rustup wrapper for rustc had a bad interaction when using a custom toolchain (like `cargo +stage1 test`).  It was using the `rustc` from where `cargo` is located, but custom toolchains often don't have cargo. This would instead use the nightly rustc (due to rustup's [fallback](https://github.com/rust-lang/rustup/blob/eaee3e723cd44b4b968b79b0ec2e8f766f1dfc77/src/config.rs#L942-L975)).  This changes it to query rustup directly to ask it where the overridden rustc is located.
@ehuss ehuss added this to the 1.52.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants