mirror of
https://github.com/rust-lang/cargo
synced 2024-08-25 10:35:53 +00:00
Auto merge of #10776 - Muscraft:cache-workspace-discovery, r=weihanglo
add a cache for discovered workspace roots
## History
`@ehuss` [noticed that](https://github.com/rust-lang/cargo/pull/10736#discussion_r894071933) 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](https://github.com/rust-lang/cargo/pull/10736#discussion_r894822022) 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](https://github.com/rust-lang/cargo/pull/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](https://github.com/rust-lang/cargo/pull/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](bbd41a4dca
) 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
This commit is contained in:
commit
ca190acce0
|
@ -133,6 +133,34 @@ impl WorkspaceConfig {
|
|||
WorkspaceConfig::Member { .. } => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns the path of the workspace root based on this `[workspace]` configuration.
|
||||
///
|
||||
/// Returns `None` if the root is not explicitly known.
|
||||
///
|
||||
/// * `self_path` is the path of the manifest this `WorkspaceConfig` is located.
|
||||
/// * `look_from` is the path where discovery started (usually the current
|
||||
/// working directory), used for `workspace.exclude` checking.
|
||||
fn get_ws_root(&self, self_path: &Path, look_from: &Path) -> Option<PathBuf> {
|
||||
match self {
|
||||
WorkspaceConfig::Root(ances_root_config) => {
|
||||
debug!("find_root - found a root checking exclusion");
|
||||
if !ances_root_config.is_excluded(look_from) {
|
||||
debug!("find_root - found!");
|
||||
Some(self_path.to_owned())
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
WorkspaceConfig::Member {
|
||||
root: Some(path_to_root),
|
||||
} => {
|
||||
debug!("find_root - found pointer");
|
||||
Some(read_root_pointer(self_path, path_to_root))
|
||||
}
|
||||
WorkspaceConfig::Member { .. } => None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Intermediate configuration of a workspace root in a manifest.
|
||||
|
@ -592,40 +620,23 @@ impl<'cfg> Workspace<'cfg> {
|
|||
/// Returns an error if `manifest_path` isn't actually a valid manifest or
|
||||
/// if some other transient error happens.
|
||||
fn find_root(&mut self, manifest_path: &Path) -> CargoResult<Option<PathBuf>> {
|
||||
let current = self.packages.load(manifest_path)?;
|
||||
match current
|
||||
.workspace_config()
|
||||
.get_ws_root(manifest_path, manifest_path)
|
||||
{
|
||||
let current = self.packages.load(manifest_path)?;
|
||||
match *current.workspace_config() {
|
||||
WorkspaceConfig::Root(_) => {
|
||||
debug!("find_root - is root {}", manifest_path.display());
|
||||
return Ok(Some(manifest_path.to_path_buf()));
|
||||
}
|
||||
WorkspaceConfig::Member {
|
||||
root: Some(ref path_to_root),
|
||||
} => return Ok(Some(read_root_pointer(manifest_path, path_to_root))),
|
||||
WorkspaceConfig::Member { root: None } => {}
|
||||
Some(root_path) => {
|
||||
debug!("find_root - is root {}", manifest_path.display());
|
||||
Ok(Some(root_path))
|
||||
}
|
||||
None => find_workspace_root_with_loader(manifest_path, self.config, |self_path| {
|
||||
Ok(self
|
||||
.packages
|
||||
.load(self_path)?
|
||||
.workspace_config()
|
||||
.get_ws_root(self_path, manifest_path))
|
||||
}),
|
||||
}
|
||||
|
||||
for ances_manifest_path in find_root_iter(manifest_path, self.config) {
|
||||
debug!("find_root - trying {}", ances_manifest_path.display());
|
||||
match *self.packages.load(&ances_manifest_path)?.workspace_config() {
|
||||
WorkspaceConfig::Root(ref ances_root_config) => {
|
||||
debug!("find_root - found a root checking exclusion");
|
||||
if !ances_root_config.is_excluded(manifest_path) {
|
||||
debug!("find_root - found!");
|
||||
return Ok(Some(ances_manifest_path));
|
||||
}
|
||||
}
|
||||
WorkspaceConfig::Member {
|
||||
root: Some(ref path_to_root),
|
||||
} => {
|
||||
debug!("find_root - found pointer");
|
||||
return Ok(Some(read_root_pointer(&ances_manifest_path, path_to_root)));
|
||||
}
|
||||
WorkspaceConfig::Member { .. } => {}
|
||||
}
|
||||
}
|
||||
Ok(None)
|
||||
}
|
||||
|
||||
/// After the root of a workspace has been located, probes for all members
|
||||
|
@ -1669,31 +1680,39 @@ pub fn resolve_relative_path(
|
|||
}
|
||||
}
|
||||
|
||||
fn parse_manifest(manifest_path: &Path, config: &Config) -> CargoResult<EitherManifest> {
|
||||
let key = manifest_path.parent().unwrap();
|
||||
let source_id = SourceId::for_path(key)?;
|
||||
let (manifest, _nested_paths) = read_manifest(manifest_path, source_id, config)?;
|
||||
Ok(manifest)
|
||||
/// Finds the path of the root of the workspace.
|
||||
pub fn find_workspace_root(manifest_path: &Path, config: &Config) -> CargoResult<Option<PathBuf>> {
|
||||
find_workspace_root_with_loader(manifest_path, config, |self_path| {
|
||||
let key = self_path.parent().unwrap();
|
||||
let source_id = SourceId::for_path(key)?;
|
||||
let (manifest, _nested_paths) = read_manifest(self_path, source_id, config)?;
|
||||
Ok(manifest
|
||||
.workspace_config()
|
||||
.get_ws_root(self_path, manifest_path))
|
||||
})
|
||||
}
|
||||
|
||||
pub fn find_workspace_root(manifest_path: &Path, config: &Config) -> CargoResult<Option<PathBuf>> {
|
||||
/// Finds the path of the root of the workspace.
|
||||
///
|
||||
/// This uses a callback to determine if the given path tells us what the
|
||||
/// workspace root is.
|
||||
fn find_workspace_root_with_loader(
|
||||
manifest_path: &Path,
|
||||
config: &Config,
|
||||
mut loader: impl FnMut(&Path) -> CargoResult<Option<PathBuf>>,
|
||||
) -> CargoResult<Option<PathBuf>> {
|
||||
// Check if there are any workspace roots that have already been found that would work
|
||||
for (ws_root, ws_root_config) in config.ws_roots.borrow().iter() {
|
||||
if manifest_path.starts_with(ws_root) && !ws_root_config.is_excluded(manifest_path) {
|
||||
// Add `Cargo.toml` since ws_root is the root and not the file
|
||||
return Ok(Some(ws_root.join("Cargo.toml").clone()));
|
||||
}
|
||||
}
|
||||
|
||||
for ances_manifest_path in find_root_iter(manifest_path, config) {
|
||||
debug!("find_root - trying {}", ances_manifest_path.display());
|
||||
match *parse_manifest(&ances_manifest_path, config)?.workspace_config() {
|
||||
WorkspaceConfig::Root(ref ances_root_config) => {
|
||||
debug!("find_root - found a root checking exclusion");
|
||||
if !ances_root_config.is_excluded(manifest_path) {
|
||||
debug!("find_root - found!");
|
||||
return Ok(Some(ances_manifest_path));
|
||||
}
|
||||
}
|
||||
WorkspaceConfig::Member {
|
||||
root: Some(ref path_to_root),
|
||||
} => {
|
||||
debug!("find_root - found pointer");
|
||||
return Ok(Some(read_root_pointer(&ances_manifest_path, path_to_root)));
|
||||
}
|
||||
WorkspaceConfig::Member { .. } => {}
|
||||
if let Some(ws_root_path) = loader(&ances_manifest_path)? {
|
||||
return Ok(Some(ws_root_path));
|
||||
}
|
||||
}
|
||||
Ok(None)
|
||||
|
|
|
@ -68,7 +68,7 @@ use std::time::Instant;
|
|||
use self::ConfigValue as CV;
|
||||
use crate::core::compiler::rustdoc::RustdocExternMap;
|
||||
use crate::core::shell::Verbosity;
|
||||
use crate::core::{features, CliUnstable, Shell, SourceId, Workspace};
|
||||
use crate::core::{features, CliUnstable, Shell, SourceId, Workspace, WorkspaceRootConfig};
|
||||
use crate::ops;
|
||||
use crate::util::errors::CargoResult;
|
||||
use crate::util::toml as cargo_toml;
|
||||
|
@ -202,6 +202,8 @@ pub struct Config {
|
|||
/// NOTE: this should be set before `configure()`. If calling this from an integration test,
|
||||
/// consider using `ConfigBuilder::enable_nightly_features` instead.
|
||||
pub nightly_features_allowed: bool,
|
||||
/// WorkspaceRootConfigs that have been found
|
||||
pub ws_roots: RefCell<HashMap<PathBuf, WorkspaceRootConfig>>,
|
||||
}
|
||||
|
||||
impl Config {
|
||||
|
@ -285,6 +287,7 @@ impl Config {
|
|||
progress_config: ProgressConfig::default(),
|
||||
env_config: LazyCell::new(),
|
||||
nightly_features_allowed: matches!(&*features::channel(), "nightly" | "dev"),
|
||||
ws_roots: RefCell::new(HashMap::new()),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -1549,18 +1549,23 @@ impl TomlManifest {
|
|||
let project = &mut project.ok_or_else(|| anyhow!("no `package` section found"))?;
|
||||
|
||||
let workspace_config = match (me.workspace.as_ref(), project.workspace.as_ref()) {
|
||||
(Some(config), None) => {
|
||||
let mut inheritable = config.package.clone().unwrap_or_default();
|
||||
(Some(toml_config), None) => {
|
||||
let mut inheritable = toml_config.package.clone().unwrap_or_default();
|
||||
inheritable.update_ws_path(package_root.to_path_buf());
|
||||
inheritable.update_deps(config.dependencies.clone());
|
||||
WorkspaceConfig::Root(WorkspaceRootConfig::new(
|
||||
inheritable.update_deps(toml_config.dependencies.clone());
|
||||
let ws_root_config = WorkspaceRootConfig::new(
|
||||
package_root,
|
||||
&config.members,
|
||||
&config.default_members,
|
||||
&config.exclude,
|
||||
&toml_config.members,
|
||||
&toml_config.default_members,
|
||||
&toml_config.exclude,
|
||||
&Some(inheritable),
|
||||
&config.metadata,
|
||||
))
|
||||
&toml_config.metadata,
|
||||
);
|
||||
config
|
||||
.ws_roots
|
||||
.borrow_mut()
|
||||
.insert(package_root.to_path_buf(), ws_root_config.clone());
|
||||
WorkspaceConfig::Root(ws_root_config)
|
||||
}
|
||||
(None, root) => WorkspaceConfig::Member {
|
||||
root: root.cloned(),
|
||||
|
@ -2206,18 +2211,23 @@ impl TomlManifest {
|
|||
.map(|r| ResolveBehavior::from_manifest(r))
|
||||
.transpose()?;
|
||||
let workspace_config = match me.workspace {
|
||||
Some(ref config) => {
|
||||
let mut inheritable = config.package.clone().unwrap_or_default();
|
||||
Some(ref toml_config) => {
|
||||
let mut inheritable = toml_config.package.clone().unwrap_or_default();
|
||||
inheritable.update_ws_path(root.to_path_buf());
|
||||
inheritable.update_deps(config.dependencies.clone());
|
||||
WorkspaceConfig::Root(WorkspaceRootConfig::new(
|
||||
inheritable.update_deps(toml_config.dependencies.clone());
|
||||
let ws_root_config = WorkspaceRootConfig::new(
|
||||
root,
|
||||
&config.members,
|
||||
&config.default_members,
|
||||
&config.exclude,
|
||||
&toml_config.members,
|
||||
&toml_config.default_members,
|
||||
&toml_config.exclude,
|
||||
&Some(inheritable),
|
||||
&config.metadata,
|
||||
))
|
||||
&toml_config.metadata,
|
||||
);
|
||||
config
|
||||
.ws_roots
|
||||
.borrow_mut()
|
||||
.insert(root.to_path_buf(), ws_root_config.clone());
|
||||
WorkspaceConfig::Root(ws_root_config)
|
||||
}
|
||||
None => {
|
||||
bail!("virtual manifests must be configured with [workspace]");
|
||||
|
@ -2334,16 +2344,30 @@ impl TomlManifest {
|
|||
|
||||
fn inheritable_from_path(
|
||||
config: &Config,
|
||||
resolved_path: PathBuf,
|
||||
workspace_path: PathBuf,
|
||||
) -> CargoResult<InheritableFields> {
|
||||
let key = resolved_path.parent().unwrap();
|
||||
let source_id = SourceId::for_path(key)?;
|
||||
let (man, _) = read_manifest(&resolved_path, source_id, config)?;
|
||||
// Workspace path should have Cargo.toml at the end
|
||||
let workspace_path_root = workspace_path.parent().unwrap();
|
||||
|
||||
// Let the borrow exit scope so that it can be picked up if there is a need to
|
||||
// read a manifest
|
||||
if let Some(ws_root) = config.ws_roots.borrow().get(workspace_path_root) {
|
||||
return Ok(ws_root.inheritable().clone());
|
||||
};
|
||||
|
||||
let source_id = SourceId::for_path(workspace_path_root)?;
|
||||
let (man, _) = read_manifest(&workspace_path, source_id, config)?;
|
||||
match man.workspace_config() {
|
||||
WorkspaceConfig::Root(root) => Ok(root.inheritable().clone()),
|
||||
WorkspaceConfig::Root(root) => {
|
||||
config
|
||||
.ws_roots
|
||||
.borrow_mut()
|
||||
.insert(workspace_path, root.clone());
|
||||
Ok(root.inheritable().clone())
|
||||
}
|
||||
_ => bail!(
|
||||
"root of a workspace inferred but wasn't a root: {}",
|
||||
resolved_path.display()
|
||||
workspace_path.display()
|
||||
),
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Reference in a new issue