-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add preloading for workspace packages in resolve_with_previous
#10761
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
…anifests aren't parsed twice
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
label
Jun 16, 2022
Muscraft
changed the title
dd preloading for workspace packages in
Add preloading for workspace packages in Jun 16, 2022
resolve_with_previous
so sdfadfasdfresolve_with_previous
Looks like my finger hit enter on accident before I could get the title and comment done. I'll update them to be correct |
(Sorry, this should have gone through bors.) |
arlosi
added a commit
to arlosi/rust
that referenced
this pull request
Jun 23, 2022
8 commits in 03a849043e25104e8b7ad0d4a96c525787b69379..a5e08c4703f202e30cdaf80ca3e7c00baa59c496 2022-06-20 14:47:36 +0000 to 2022-06-23 20:12:03 +0000 - Fix tests due to change in dead_code diagnostic. (rust-lang/cargo#10785) - Stabilize config-cli (rust-lang/cargo#10755) - Restrict duplicate deps warning only to published packages (rust-lang/cargo#10767) - Use fingerprint_hash when computing fingerprints for custom targets (rust-lang/cargo#10746) - Add preloading for workspace packages in `resolve_with_previous` (rust-lang/cargo#10761) - capitalise, for consistency (rust-lang/cargo#10772) - remove unused dependency from benchsuite (rust-lang/cargo#10774) - docs(contrib): Add documentation for ui tests (rust-lang/cargo#10758)
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 24, 2022
Update cargo 8 commits in 03a849043e25104e8b7ad0d4a96c525787b69379..a5e08c4703f202e30cdaf80ca3e7c00baa59c496 2022-06-20 14:47:36 +0000 to 2022-06-23 20:12:03 +0000 - Fix tests due to change in dead_code diagnostic. (rust-lang/cargo#10785) - Stabilize config-cli (rust-lang/cargo#10755) - Restrict duplicate deps warning only to published packages (rust-lang/cargo#10767) - Use fingerprint_hash when computing fingerprints for custom targets (rust-lang/cargo#10746) - Add preloading for workspace packages in `resolve_with_previous` (rust-lang/cargo#10761) - capitalise, for consistency (rust-lang/cargo#10772) - remove unused dependency from benchsuite (rust-lang/cargo#10774) - docs(contrib): Add documentation for ui tests (rust-lang/cargo#10758)
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.
Minor Performance Hit
When I was looking into concerns about the performance of workspace inheritance, I found that cargo would parse manifests twice for
cargo metadata
andcargo check
.I found this after @epage suggested looking into the concerns using
dbg!(package.name)
placed in different places. When placing that inside ofTomlManifest::to_real_manifest
and running against any workspace (inheritance or not) it would show that we are parsing each workspace member'sCargo.toml
twice.e.g.
cargo run -- metadata --manifest-path ./temp/lunatic/Cargo.toml > /dev/null
I would get the output of:Observed Cause
As I dug into this it looks like the manifests are parsed once when creating the
Workspace
and then again inresolve::resolve_with_previous
. I dug deeper into whyresolve::resolve_with_previous
was parsing the manifests again and found the cause to be from these lines since they call intoPackageRegistry::ensure_loaded
.ensure_loaded
will callself.load(namespace, kind)?;
andself.block_until_ready()?;
for each of the members. The problem with those lines comes fromself.block_until_ready()?;
. This is because the source for each member should bePathSource
(from what I have seen) which callsself.update()
and reparses the manifest.Proposed solution
After discussing in the related zulip thread,
Workspace::preload
seemed to fit the needs. It solves the problem by creating aPathSource
for all packages that have been found during the creation of aWorkspace
. Once they have been created they get added to thePackageRegistry
that gets passed in.The reason for keeping the loop over the workspace members is that there are cases where
preload
does not work (cargo install
,cargo package
, virtual packages, etc).Performance gains
This change has notable performance gains if cargo is invoked in a large workspace. On my machine, there was a ~10ms drop-in time on
benches/workspace/rust
. This could be even bigger depending on the machine and its storage device. While this does help mitigate some of the performance hit brought on by workspace inheritance, it does not solve it entirely.Before changes
After changes