From feaa75db457f3e3a7268c7939d7fde484b351d0a Mon Sep 17 00:00:00 2001 From: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com> Date: Thu, 10 Oct 2024 16:08:02 +0200 Subject: [PATCH 1/2] test: add a test showing a slow case in the resolver --- crates/resolver-tests/tests/resolve.rs | 48 ++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index be358459a078..659a83c5d425 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -4,8 +4,8 @@ use cargo::util::GlobalContext; use resolver_tests::{ helpers::{ - assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names, pkg_id, - pkg_loc, registry, ToPkgId, + assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names, pkg, + pkg_dep, pkg_dep_with, pkg_id, pkg_loc, registry, ToDep, ToPkgId, }, pkg, resolve, resolve_with_global_context, }; @@ -937,6 +937,50 @@ fn large_conflict_cache() { let _ = resolve(root_deps, ®); } +#[test] +fn resolving_slow_case_missing_feature() { + let mut reg = Vec::new(); + + const LAST_CRATE_VERSION_COUNT: usize = 50; + + // increase in resolve time is at least cubic over `INTERMEDIATE_CRATES_VERSION_COUNT`. + // it should be `>= LAST_CRATE_VERSION_COUNT` to reproduce slowdown. + const INTERMEDIATE_CRATES_VERSION_COUNT: usize = LAST_CRATE_VERSION_COUNT + 5; + + // should be `>= 2` to reproduce slowdown + const TRANSITIVE_CRATES_COUNT: usize = 3; + + reg.push(pkg_dep_with(("last", "1.0.0"), vec![], &[("f", &[])])); + for v in 1..LAST_CRATE_VERSION_COUNT { + reg.push(pkg(("last", format!("1.0.{v}")))); + } + + reg.push(pkg_dep( + ("dep", "1.0.0"), + vec![ + dep("last"), // <-- needed to reproduce slowdown + dep_req("intermediate-1", "1.0.0"), + ], + )); + + for n in 0..INTERMEDIATE_CRATES_VERSION_COUNT { + let version = format!("1.0.{n}"); + for c in 1..TRANSITIVE_CRATES_COUNT { + reg.push(pkg_dep( + (format!("intermediate-{c}"), &version), + vec![dep_req(&format!("intermediate-{}", c + 1), &version)], + )); + } + reg.push(pkg_dep( + (format!("intermediate-{TRANSITIVE_CRATES_COUNT}"), &version), + vec![dep_req("last", "1.0.0").with(&["f"])], + )); + } + + let deps = vec![dep("dep")]; + let _ = resolve(deps, ®); +} + #[test] fn cyclic_good_error_message() { let input = vec![ From 93db5bfe59c2971485b15c708b1bf1711bd18bd3 Mon Sep 17 00:00:00 2001 From: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com> Date: Thu, 10 Oct 2024 16:09:22 +0200 Subject: [PATCH 2/2] fix: use rustc-hash in the resolver --- Cargo.lock | 11 +++++++++-- Cargo.toml | 2 ++ crates/resolver-tests/src/sat.rs | 2 +- src/cargo/core/resolver/conflict_cache.rs | 18 ++++++++---------- src/cargo/core/resolver/context.rs | 16 +++++++++------- src/cargo/core/resolver/mod.rs | 2 +- src/cargo/util/graph.rs | 2 +- 7 files changed, 31 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 23a660a0f933..2fea5e8b4060 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -316,6 +316,7 @@ dependencies = [ "rand", "regex", "rusqlite", + "rustc-hash 2.0.0", "rustfix", "same-file", "semver", @@ -2973,6 +2974,12 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "08d43f7aa6b08d49f382cde6a7982047c3426db949b1424bc4b7ec9ae12c6ce2" +[[package]] +name = "rustc-hash" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "583034fd73374156e66797ed8e5b0d5690409c9226b22d87cb7f19821c05d152" + [[package]] name = "rustfix" version = "0.8.7" @@ -3715,7 +3722,7 @@ dependencies = [ "log", "ordered-float", "partial_ref", - "rustc-hash", + "rustc-hash 1.1.0", "serde", "thiserror", "varisat-checker", @@ -3735,7 +3742,7 @@ dependencies = [ "anyhow", "log", "partial_ref", - "rustc-hash", + "rustc-hash 1.1.0", "smallvec", "thiserror", "varisat-dimacs", diff --git a/Cargo.toml b/Cargo.toml index bc86f4086103..c451ea3730b8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -79,6 +79,7 @@ pulldown-cmark = { version = "0.11.0", default-features = false, features = ["ht rand = "0.8.5" regex = "1.10.5" rusqlite = { version = "0.32.0", features = ["bundled"] } +rustc-hash = "2.0.0" rustfix = { version = "0.8.2", path = "crates/rustfix" } same-file = "1.0.6" security-framework = "2.11.1" @@ -185,6 +186,7 @@ pathdiff.workspace = true rand.workspace = true regex.workspace = true rusqlite.workspace = true +rustc-hash.workspace = true rustfix.workspace = true same-file.workspace = true semver.workspace = true diff --git a/crates/resolver-tests/src/sat.rs b/crates/resolver-tests/src/sat.rs index 60546e85d1e8..f04a585b296a 100644 --- a/crates/resolver-tests/src/sat.rs +++ b/crates/resolver-tests/src/sat.rs @@ -352,7 +352,7 @@ impl SatResolver { let mut by_name: HashMap> = HashMap::new(); for pkg in registry { - by_name.entry(pkg.name()).or_default().push(pkg.clone()) + by_name.entry(pkg.name()).or_default().push(pkg.clone()); } let mut solver = varisat::Solver::new(); diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index f8543644561e..ea9f544f454b 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -1,5 +1,6 @@ -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap}; +use rustc_hash::{FxHashMap, FxHashSet}; use tracing::trace; use super::types::ConflictMap; @@ -140,17 +141,17 @@ pub(super) struct ConflictCache { // as a global cache which we never delete from. Any entry in this map is // unconditionally true regardless of our resolution history of how we got // here. - con_from_dep: HashMap, + con_from_dep: FxHashMap, // `dep_from_pid` is an inverse-index of `con_from_dep`. // For every `PackageId` this lists the `Dependency`s that mention it in `dep_from_pid`. - dep_from_pid: HashMap>, + dep_from_pid: FxHashMap>, } impl ConflictCache { pub fn new() -> ConflictCache { ConflictCache { - con_from_dep: HashMap::new(), - dep_from_pid: HashMap::new(), + con_from_dep: HashMap::default(), + dep_from_pid: HashMap::default(), } } pub fn find( @@ -207,14 +208,11 @@ impl ConflictCache { ); for c in con.keys() { - self.dep_from_pid - .entry(*c) - .or_insert_with(HashSet::new) - .insert(dep.clone()); + self.dep_from_pid.entry(*c).or_default().insert(dep.clone()); } } - pub fn dependencies_conflicting_with(&self, pid: PackageId) -> Option<&HashSet> { + pub fn dependencies_conflicting_with(&self, pid: PackageId) -> Option<&FxHashSet> { self.dep_from_pid.get(&pid) } } diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 4bc4758feef6..08c289d9f8be 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -19,13 +19,13 @@ pub struct ResolverContext { pub age: ContextAge, pub activations: Activations, /// list the features that are activated for each package - pub resolve_features: im_rc::HashMap, + pub resolve_features: im_rc::HashMap, /// get the package that will be linking to a native library by its links attribute - pub links: im_rc::HashMap, + pub links: im_rc::HashMap, /// a way to look up for a package in activations what packages required it /// and all of the exact deps that it fulfilled. - pub parents: Graph>, + pub parents: Graph>, } /// When backtracking it can be useful to know how far back to go. @@ -40,7 +40,9 @@ pub type ContextAge = usize; /// semver compatible version of each crate. /// This all so stores the `ContextAge`. pub type ActivationsKey = (InternedString, SourceId, SemverCompatibility); -pub type Activations = im_rc::HashMap; + +pub type Activations = + im_rc::HashMap; /// A type that represents when cargo treats two Versions as compatible. /// Versions `a` and `b` are compatible if their left-most nonzero digit is the @@ -74,10 +76,10 @@ impl ResolverContext { pub fn new() -> ResolverContext { ResolverContext { age: 0, - resolve_features: im_rc::HashMap::new(), - links: im_rc::HashMap::new(), + resolve_features: im_rc::HashMap::default(), + links: im_rc::HashMap::default(), parents: Graph::new(), - activations: im_rc::HashMap::new(), + activations: im_rc::HashMap::default(), } } diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 010c03bb85b1..e78b2c0d9b3b 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -679,7 +679,7 @@ fn activate( Rc::make_mut( cx.resolve_features .entry(candidate.package_id()) - .or_insert_with(Rc::default), + .or_default(), ) .extend(used_features); } diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 9e8ba143f401..f955032c59a7 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -22,7 +22,7 @@ impl Graph { .entry(node) .or_insert_with(im_rc::OrdMap::new) .entry(child) - .or_insert_with(Default::default) + .or_default() } /// Returns the graph obtained by reversing all edges.