Skip to content

Commit ca190ac

Browse files
borsbors
committed
Auto merge of #10776 - Muscraft:cache-workspace-discovery, r=weihanglo
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
2 parents c0bbd42 + fd8bcf9 commit ca190ac

File tree

3 files changed

+123
-77
lines changed

3 files changed

+123
-77
lines changed

‎src/cargo/core/workspace.rs

+71-52
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,34 @@ impl WorkspaceConfig {
133133
WorkspaceConfig::Member { .. } => None,
134134
}
135135
}
136+
137+
/// Returns the path of the workspace root based on this `[workspace]` configuration.
138+
///
139+
/// Returns `None` if the root is not explicitly known.
140+
///
141+
/// * `self_path` is the path of the manifest this `WorkspaceConfig` is located.
142+
/// * `look_from` is the path where discovery started (usually the current
143+
/// working directory), used for `workspace.exclude` checking.
144+
fn get_ws_root(&self, self_path: &Path, look_from: &Path) -> Option<PathBuf> {
145+
match self {
146+
WorkspaceConfig::Root(ances_root_config) => {
147+
debug!("find_root - found a root checking exclusion");
148+
if !ances_root_config.is_excluded(look_from) {
149+
debug!("find_root - found!");
150+
Some(self_path.to_owned())
151+
} else {
152+
None
153+
}
154+
}
155+
WorkspaceConfig::Member {
156+
root: Some(path_to_root),
157+
} => {
158+
debug!("find_root - found pointer");
159+
Some(read_root_pointer(self_path, path_to_root))
160+
}
161+
WorkspaceConfig::Member { .. } => None,
162+
}
163+
}
136164
}
137165

138166
/// Intermediate configuration of a workspace root in a manifest.
@@ -592,40 +620,23 @@ impl<'cfg> Workspace<'cfg> {
592620
/// Returns an error if `manifest_path` isn't actually a valid manifest or
593621
/// if some other transient error happens.
594622
fn find_root(&mut self, manifest_path: &Path) -> CargoResult<Option<PathBuf>> {
623+
let current = self.packages.load(manifest_path)?;
624+
match current
625+
.workspace_config()
626+
.get_ws_root(manifest_path, manifest_path)
595627
{
596-
let current = self.packages.load(manifest_path)?;
597-
match *current.workspace_config() {
598-
WorkspaceConfig::Root(_) => {
599-
debug!("find_root - is root {}", manifest_path.display());
600-
return Ok(Some(manifest_path.to_path_buf()));
601-
}
602-
WorkspaceConfig::Member {
603-
root: Some(ref path_to_root),
604-
} => return Ok(Some(read_root_pointer(manifest_path, path_to_root))),
605-
WorkspaceConfig::Member { root: None } => {}
606-
}
607-
}
608-
609-
for ances_manifest_path in find_root_iter(manifest_path, self.config) {
610-
debug!("find_root - trying {}", ances_manifest_path.display());
611-
match *self.packages.load(&ances_manifest_path)?.workspace_config() {
612-
WorkspaceConfig::Root(ref ances_root_config) => {
613-
debug!("find_root - found a root checking exclusion");
614-
if !ances_root_config.is_excluded(manifest_path) {
615-
debug!("find_root - found!");
616-
return Ok(Some(ances_manifest_path));
617-
}
618-
}
619-
WorkspaceConfig::Member {
620-
root: Some(ref path_to_root),
621-
} => {
622-
debug!("find_root - found pointer");
623-
return Ok(Some(read_root_pointer(&ances_manifest_path, path_to_root)));
624-
}
625-
WorkspaceConfig::Member { .. } => {}
628+
Some(root_path) => {
629+
debug!("find_root - is root {}", manifest_path.display());
630+
Ok(Some(root_path))
626631
}
632+
None => find_workspace_root_with_loader(manifest_path, self.config, |self_path| {
633+
Ok(self
634+
.packages
635+
.load(self_path)?
636+
.workspace_config()
637+
.get_ws_root(self_path, manifest_path))
638+
}),
627639
}
628-
Ok(None)
629640
}
630641

631642
/// After the root of a workspace has been located, probes for all members
@@ -1669,31 +1680,39 @@ pub fn resolve_relative_path(
16691680
}
16701681
}
16711682

1672-
fn parse_manifest(manifest_path: &Path, config: &Config) -> CargoResult<EitherManifest> {
1673-
let key = manifest_path.parent().unwrap();
1674-
let source_id = SourceId::for_path(key)?;
1675-
let (manifest, _nested_paths) = read_manifest(manifest_path, source_id, config)?;
1676-
Ok(manifest)
1683+
/// Finds the path of the root of the workspace.
1684+
pub fn find_workspace_root(manifest_path: &Path, config: &Config) -> CargoResult<Option<PathBuf>> {
1685+
find_workspace_root_with_loader(manifest_path, config, |self_path| {
1686+
let key = self_path.parent().unwrap();
1687+
let source_id = SourceId::for_path(key)?;
1688+
let (manifest, _nested_paths) = read_manifest(self_path, source_id, config)?;
1689+
Ok(manifest
1690+
.workspace_config()
1691+
.get_ws_root(self_path, manifest_path))
1692+
})
16771693
}
16781694

1679-
pub fn find_workspace_root(manifest_path: &Path, config: &Config) -> CargoResult<Option<PathBuf>> {
1695+
/// Finds the path of the root of the workspace.
1696+
///
1697+
/// This uses a callback to determine if the given path tells us what the
1698+
/// workspace root is.
1699+
fn find_workspace_root_with_loader(
1700+
manifest_path: &Path,
1701+
config: &Config,
1702+
mut loader: impl FnMut(&Path) -> CargoResult<Option<PathBuf>>,
1703+
) -> CargoResult<Option<PathBuf>> {
1704+
// Check if there are any workspace roots that have already been found that would work
1705+
for (ws_root, ws_root_config) in config.ws_roots.borrow().iter() {
1706+
if manifest_path.starts_with(ws_root) && !ws_root_config.is_excluded(manifest_path) {
1707+
// Add `Cargo.toml` since ws_root is the root and not the file
1708+
return Ok(Some(ws_root.join("Cargo.toml").clone()));
1709+
}
1710+
}
1711+
16801712
for ances_manifest_path in find_root_iter(manifest_path, config) {
16811713
debug!("find_root - trying {}", ances_manifest_path.display());
1682-
match *parse_manifest(&ances_manifest_path, config)?.workspace_config() {
1683-
WorkspaceConfig::Root(ref ances_root_config) => {
1684-
debug!("find_root - found a root checking exclusion");
1685-
if !ances_root_config.is_excluded(manifest_path) {
1686-
debug!("find_root - found!");
1687-
return Ok(Some(ances_manifest_path));
1688-
}
1689-
}
1690-
WorkspaceConfig::Member {
1691-
root: Some(ref path_to_root),
1692-
} => {
1693-
debug!("find_root - found pointer");
1694-
return Ok(Some(read_root_pointer(&ances_manifest_path, path_to_root)));
1695-
}
1696-
WorkspaceConfig::Member { .. } => {}
1714+
if let Some(ws_root_path) = loader(&ances_manifest_path)? {
1715+
return Ok(Some(ws_root_path));
16971716
}
16981717
}
16991718
Ok(None)

‎src/cargo/util/config/mod.rs

+4-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ use std::time::Instant;
6868
use self::ConfigValue as CV;
6969
use crate::core::compiler::rustdoc::RustdocExternMap;
7070
use crate::core::shell::Verbosity;
71-
use crate::core::{features, CliUnstable, Shell, SourceId, Workspace};
71+
use crate::core::{features, CliUnstable, Shell, SourceId, Workspace, WorkspaceRootConfig};
7272
use crate::ops;
7373
use crate::util::errors::CargoResult;
7474
use crate::util::toml as cargo_toml;
@@ -202,6 +202,8 @@ pub struct Config {
202202
/// NOTE: this should be set before `configure()`. If calling this from an integration test,
203203
/// consider using `ConfigBuilder::enable_nightly_features` instead.
204204
pub nightly_features_allowed: bool,
205+
/// WorkspaceRootConfigs that have been found
206+
pub ws_roots: RefCell<HashMap<PathBuf, WorkspaceRootConfig>>,
205207
}
206208

207209
impl Config {
@@ -285,6 +287,7 @@ impl Config {
285287
progress_config: ProgressConfig::default(),
286288
env_config: LazyCell::new(),
287289
nightly_features_allowed: matches!(&*features::channel(), "nightly" | "dev"),
290+
ws_roots: RefCell::new(HashMap::new()),
288291
}
289292
}
290293

‎src/cargo/util/toml/mod.rs

+48-24
Original file line numberDiff line numberDiff line change
@@ -1549,18 +1549,23 @@ impl TomlManifest {
15491549
let project = &mut project.ok_or_else(|| anyhow!("no `package` section found"))?;
15501550

15511551
let workspace_config = match (me.workspace.as_ref(), project.workspace.as_ref()) {
1552-
(Some(config), None) => {
1553-
let mut inheritable = config.package.clone().unwrap_or_default();
1552+
(Some(toml_config), None) => {
1553+
let mut inheritable = toml_config.package.clone().unwrap_or_default();
15541554
inheritable.update_ws_path(package_root.to_path_buf());
1555-
inheritable.update_deps(config.dependencies.clone());
1556-
WorkspaceConfig::Root(WorkspaceRootConfig::new(
1555+
inheritable.update_deps(toml_config.dependencies.clone());
1556+
let ws_root_config = WorkspaceRootConfig::new(
15571557
package_root,
1558-
&config.members,
1559-
&config.default_members,
1560-
&config.exclude,
1558+
&toml_config.members,
1559+
&toml_config.default_members,
1560+
&toml_config.exclude,
15611561
&Some(inheritable),
1562-
&config.metadata,
1563-
))
1562+
&toml_config.metadata,
1563+
);
1564+
config
1565+
.ws_roots
1566+
.borrow_mut()
1567+
.insert(package_root.to_path_buf(), ws_root_config.clone());
1568+
WorkspaceConfig::Root(ws_root_config)
15641569
}
15651570
(None, root) => WorkspaceConfig::Member {
15661571
root: root.cloned(),
@@ -2206,18 +2211,23 @@ impl TomlManifest {
22062211
.map(|r| ResolveBehavior::from_manifest(r))
22072212
.transpose()?;
22082213
let workspace_config = match me.workspace {
2209-
Some(ref config) => {
2210-
let mut inheritable = config.package.clone().unwrap_or_default();
2214+
Some(ref toml_config) => {
2215+
let mut inheritable = toml_config.package.clone().unwrap_or_default();
22112216
inheritable.update_ws_path(root.to_path_buf());
2212-
inheritable.update_deps(config.dependencies.clone());
2213-
WorkspaceConfig::Root(WorkspaceRootConfig::new(
2217+
inheritable.update_deps(toml_config.dependencies.clone());
2218+
let ws_root_config = WorkspaceRootConfig::new(
22142219
root,
2215-
&config.members,
2216-
&config.default_members,
2217-
&config.exclude,
2220+
&toml_config.members,
2221+
&toml_config.default_members,
2222+
&toml_config.exclude,
22182223
&Some(inheritable),
2219-
&config.metadata,
2220-
))
2224+
&toml_config.metadata,
2225+
);
2226+
config
2227+
.ws_roots
2228+
.borrow_mut()
2229+
.insert(root.to_path_buf(), ws_root_config.clone());
2230+
WorkspaceConfig::Root(ws_root_config)
22212231
}
22222232
None => {
22232233
bail!("virtual manifests must be configured with [workspace]");
@@ -2334,16 +2344,30 @@ impl TomlManifest {
23342344

23352345
fn inheritable_from_path(
23362346
config: &Config,
2337-
resolved_path: PathBuf,
2347+
workspace_path: PathBuf,
23382348
) -> CargoResult<InheritableFields> {
2339-
let key = resolved_path.parent().unwrap();
2340-
let source_id = SourceId::for_path(key)?;
2341-
let (man, _) = read_manifest(&resolved_path, source_id, config)?;
2349+
// Workspace path should have Cargo.toml at the end
2350+
let workspace_path_root = workspace_path.parent().unwrap();
2351+
2352+
// Let the borrow exit scope so that it can be picked up if there is a need to
2353+
// read a manifest
2354+
if let Some(ws_root) = config.ws_roots.borrow().get(workspace_path_root) {
2355+
return Ok(ws_root.inheritable().clone());
2356+
};
2357+
2358+
let source_id = SourceId::for_path(workspace_path_root)?;
2359+
let (man, _) = read_manifest(&workspace_path, source_id, config)?;
23422360
match man.workspace_config() {
2343-
WorkspaceConfig::Root(root) => Ok(root.inheritable().clone()),
2361+
WorkspaceConfig::Root(root) => {
2362+
config
2363+
.ws_roots
2364+
.borrow_mut()
2365+
.insert(workspace_path, root.clone());
2366+
Ok(root.inheritable().clone())
2367+
}
23442368
_ => bail!(
23452369
"root of a workspace inferred but wasn't a root: {}",
2346-
resolved_path.display()
2370+
workspace_path.display()
23472371
),
23482372
}
23492373
}

0 commit comments

Comments
 (0)