Skip to content

Commit

Permalink
Auto merge of #14663 - x-hgg-x:resolver-perf, r=Eh2406
Browse files Browse the repository at this point in the history
Improve resolver speed

### What does this PR try to resolve?

This PR improves the resolver speed after investigations from Eh2406/pubgrub-crates-benchmark#6.

### How should we test and review this PR?

Commit 1 adds a test showing a slow case in the resolver, where resolving time is cubic over the number of versions of a crate. It can be used for benchmarking the resolver.

Comparison of the resolving time with various values of `N=LAST_CRATE_VERSION_COUNT` and `C=TRANSITIVE_CRATES_COUNT`:

|      | N=100 | N=200 | N=400 |
|------|-------|-------|-------|
| C=1  | 0.25s | 0.5s  | 1.4s  |
| C=2  | 7s    | 44s   | 314s  |
| C=3  | 12s   | 77s   | 537s  |
| C=6  | 30s   | 149s  | 1107s |
| C=12 | 99s   | 447s  | 2393s |

Commit 2 replaces the default hasher with the hasher from `rustc-hash`, decreasing resolving time by more than 50%.

Performance comparison with the test from commit 1 by setting `LAST_CRATE_VERSION_COUNT = 100`:
| commit          | duration |
|-----------------|----------|
| master          | 16s      |
| with rustc-hash | 6.7s     |

Firefox profiles, can be read with https://profiler.firefox.com:
[perf.tar.gz](https://github.com/user-attachments/files/17318243/perf.tar.gz)

r? Eh2406
  • Loading branch information
bors committed Oct 10, 2024
2 parents 15fbd2f + 93db5bf commit 6d679d3
Show file tree
Hide file tree
Showing 8 changed files with 77 additions and 24 deletions.
11 changes: 9 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,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"
Expand Down Expand Up @@ -187,6 +188,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
Expand Down
2 changes: 1 addition & 1 deletion crates/resolver-tests/src/sat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ impl SatResolver {

let mut by_name: HashMap<InternedString, Vec<Summary>> = 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();
Expand Down
48 changes: 46 additions & 2 deletions crates/resolver-tests/tests/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -937,6 +937,50 @@ fn large_conflict_cache() {
let _ = resolve(root_deps, &reg);
}

#[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, &reg);
}

#[test]
fn cyclic_good_error_message() {
let input = vec![
Expand Down
18 changes: 8 additions & 10 deletions src/cargo/core/resolver/conflict_cache.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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<Dependency, ConflictStoreTrie>,
con_from_dep: FxHashMap<Dependency, ConflictStoreTrie>,
// `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<PackageId, HashSet<Dependency>>,
dep_from_pid: FxHashMap<PackageId, FxHashSet<Dependency>>,
}

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(
Expand Down Expand Up @@ -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<Dependency>> {
pub fn dependencies_conflicting_with(&self, pid: PackageId) -> Option<&FxHashSet<Dependency>> {
self.dep_from_pid.get(&pid)
}
}
16 changes: 9 additions & 7 deletions src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<PackageId, FeaturesSet>,
pub resolve_features: im_rc::HashMap<PackageId, FeaturesSet, rustc_hash::FxBuildHasher>,
/// get the package that will be linking to a native library by its links attribute
pub links: im_rc::HashMap<InternedString, PackageId>,
pub links: im_rc::HashMap<InternedString, PackageId, rustc_hash::FxBuildHasher>,

/// 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<PackageId, im_rc::HashSet<Dependency>>,
pub parents: Graph<PackageId, im_rc::HashSet<Dependency, rustc_hash::FxBuildHasher>>,
}

/// When backtracking it can be useful to know how far back to go.
Expand All @@ -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<ActivationsKey, (Summary, ContextAge)>;

pub type Activations =
im_rc::HashMap<ActivationsKey, (Summary, ContextAge), rustc_hash::FxBuildHasher>;

/// 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
Expand Down Expand Up @@ -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(),
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/util/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ impl<N: Eq + Ord + Clone, E: Default + Clone> Graph<N, E> {
.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.
Expand Down

0 comments on commit 6d679d3

Please sign in to comment.