Add a benchmark for workspace initialization #10754
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
clap 3.2.3 disables the deprecations by default and people can enable them with the In theory, this should unblock CI so I re-kicked off the jobs. |
epage
reviewed
Jun 17, 2022
|
@bors r+ |
|
📌 Commit f182411 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit f182411 with merge 6434777ca73ff8bf68cae42908e27dfcaa8b2ffc... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry Cannot tell why failed.
|
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
bors
added a commit
that referenced
this pull request
Jun 20, 2022
remove unused dependency from benchsuite In #10754 I added a new benchmark to the benchsuite. While figuring out the best way to add the new benchmark, I added `cargo-test-support` as a dependency. It appears I missed removing it before making the PR. This PR removes `cargo-test-support` since it is not needed
bors
added a commit
that referenced
this pull request
Jul 5, 2022
add a cache for discovered workspace roots ## History `@ehuss` [noticed that](#10736 (comment)) workspace inheritance caused a significant increase in startup times when using workspace inheritance. This brought up the creation of #10747. When using a similar test setup [to the original](#10736 (comment)) I got ``` Benchmark 1: cd rust; ../../../target/release/cargo metadata Time (mean ± σ): 149.4 ms ± 3.8 ms [User: 105.9 ms, System: 31.7 ms] Range (min … max): 144.2 ms … 162.2 ms 19 runs Benchmark 2: cd rust-ws-inherit; ../../../target/release/cargo metadata Time (mean ± σ): 191.6 ms ± 1.4 ms [User: 145.9 ms, System: 34.2 ms] Range (min … max): 188.8 ms … 193.9 ms 15 runs ``` This showed a large increase in time per cargo command when using workspace inheritance. During the investigation of this issue, other [performance concerns were found and addressed](#10761). This resulted in a drop in time across the board but heavily favored workspace inheritance. ``` Benchmark 1: cd rust; ../../../target/release/cargo metadata Time (mean ± σ): 139.3 ms ± 1.7 ms [User: 99.8 ms, System: 29.4 ms] Range (min … max): 137.1 ms … 144.5 ms 20 runs Benchmark 2: cd rust-ws-inherit; ../../../target/release/cargo metadata Time (mean ± σ): 161.7 ms ± 1.4 ms [User: 120.4 ms, System: 31.2 ms] Range (min … max): 158.0 ms … 164.6 ms 18 runs ``` ## Performance after changes `hyperfine --warmup 10 "cd rust; ../../../target/release/cargo metadata" "cd rust-ws-inherit; ../../../target/release/cargo metadata" --runs 40` ``` Benchmark 1: cd rust; ../../../target/release/cargo metadata Time (mean ± σ): 140.1 ms ± 1.5 ms [User: 99.5 ms, System: 30.7 ms] Range (min … max): 137.4 ms … 144.0 ms 40 runs Benchmark 2: cd rust-ws-inherit; ../../../target/release/cargo metadata Time (mean ± σ): 141.8 ms ± 1.6 ms [User: 100.9 ms, System: 30.9 ms] Range (min … max): 138.4 ms … 145.4 ms 40 runs ``` [New Benchmark](#10754) `cargo bench -- workspace_initialization/rust` ``` workspace_initialization/rust time: [14.779 ms 14.880 ms 14.997 ms] workspace_initialization/rust-ws-inherit time: [16.235 ms 16.293 ms 16.359 ms] ``` ## Changes Made - [Pulled a commit](bbd41a4) from `@ehuss` that deduplicated finding a workspace root to make the changes easier - Added a cache in `Config` to hold found `WorkspaceRootConfig`s - This makes it so manifests should only be parsed once - Made `WorkspaceRootConfig` get added to the cache when parsing a manifest ## Testing Steps To check the new benchmark: 1. `cd benches/benchsuite` 2. `cargo bench -- workspace_initialization/rust` Using `hyperfine`: 1. run `cargo build --release` 2. extract `rust` and `rust-ws-inherit` in `benches/workspaces` 3. cd `benches/workspaces` 4. Prime the target directory with a cache of `rustc` info. In `rust` and `rust-ws-inherit`, run: `cargo +nightly c -p linkchecker`. Otherwise it would be measuring `rustc` overhead. 4. run `hyperfine --warmup 10 "cd rust; ../../../target/release/cargo metadata" "cd rust-ws-inherit; ../../../target/release/cargo metadata" --runs 40` closes #10747
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It was suggested that a benchmark for workspace initialization should be added. This was suggested because there were issues with the performance of workspace inheritance as well as a general way to track the workspace initialization time across cargo changes
Changes
resolve.rsto a sharedlib.rsenv!("CARGO_TARGET_TMPDIR")would fail to compile when put inside of the newlib.rs