From 7ea2f4c43e5fedf100d01293f713d48540cf8b0b Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 11 Dec 2024 18:08:11 +0000 Subject: [PATCH 1/4] use let else to clean up the code --- src/cargo/core/resolver/conflict_cache.rs | 50 ++++++++++++----------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index ea9f544f454..2e551a70db1 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -44,31 +44,33 @@ impl ConflictStoreTrie { .unwrap_or_else(|| m.range(..)) { // If the key is active, then we need to check all of the corresponding subtrie. - if let Some(age_this) = is_active(pid) { - if age_this >= max_age && must_contain != Some(pid) { - // not worth looking at, it is to old. - continue; - } - if let Some((o, age_o)) = - store.find(is_active, must_contain.filter(|&f| f != pid), max_age) - { - let age = if must_contain == Some(pid) { - // all the results will include `must_contain` - // so the age of must_contain is not relevant to find the best result. - age_o - } else { - std::cmp::max(age_this, age_o) - }; - if max_age > age { - // we found one that can jump-back further so replace the out. - out = Some((o, age)); - // and don't look at anything older - max_age = age - } - } + let Some(age_this) = is_active(pid) else { + // Else, if it is not active then there is no way any of the corresponding + // subtrie will be conflicting. + continue; + }; + if age_this >= max_age && must_contain != Some(pid) { + // not worth looking at, it is to old. + continue; + } + let Some((o, age_o)) = + store.find(is_active, must_contain.filter(|&f| f != pid), max_age) + else { + continue; + }; + let age = if must_contain == Some(pid) { + // all the results will include `must_contain` + // so the age of must_contain is not relevant to find the best result. + age_o + } else { + std::cmp::max(age_this, age_o) + }; + if max_age > age { + // we found one that can jump-back further so replace the out. + out = Some((o, age)); + // and don't look at anything older + max_age = age } - // Else, if it is not active then there is no way any of the corresponding - // subtrie will be conflicting. } out } From 0772d515bdd11de42d1fe9f1d0d7921e1b56fade Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 11 Dec 2024 18:08:11 +0000 Subject: [PATCH 2/4] only look up the active package when searching the conflict cash --- src/cargo/core/resolver/conflict_cache.rs | 53 ++++++++++++++++------- src/cargo/core/resolver/context.rs | 6 +++ src/cargo/core/resolver/mod.rs | 7 +-- 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index 2e551a70db1..da31700be5a 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -3,7 +3,7 @@ use std::collections::{BTreeMap, HashMap}; use rustc_hash::{FxHashMap, FxHashSet}; use tracing::trace; -use super::types::ConflictMap; +use super::types::{ActivationsKey, ConflictMap}; use crate::core::resolver::ResolverContext; use crate::core::{Dependency, PackageId}; @@ -14,7 +14,9 @@ enum ConflictStoreTrie { Leaf(ConflictMap), /// A map from an element to a subtrie where /// all the sets in the subtrie contains that element. - Node(BTreeMap), + Node( + BTreeMap>, + ), } impl ConflictStoreTrie { @@ -24,7 +26,7 @@ impl ConflictStoreTrie { /// one that will allow for the most jump-back. fn find( &self, - is_active: &impl Fn(PackageId) -> Option, + in_activation_slot: &impl Fn(&ActivationsKey) -> Option<(PackageId, usize)>, must_contain: Option, mut max_age: usize, ) -> Option<(&ConflictMap, usize)> { @@ -39,23 +41,30 @@ impl ConflictStoreTrie { } ConflictStoreTrie::Node(m) => { let mut out = None; - for (&pid, store) in must_contain - .map(|f| m.range(..=f)) + for (activations_key, map) in must_contain + .map(|f| m.range(..=(f.as_activations_key()))) .unwrap_or_else(|| m.range(..)) { - // If the key is active, then we need to check all of the corresponding subtrie. - let Some(age_this) = is_active(pid) else { + // Find the package that is active for this activation key. + let Some((pid, age_this)) = in_activation_slot(&activations_key) else { // Else, if it is not active then there is no way any of the corresponding - // subtrie will be conflicting. + // subtries will be conflicting. continue; }; if age_this >= max_age && must_contain != Some(pid) { // not worth looking at, it is to old. continue; } - let Some((o, age_o)) = - store.find(is_active, must_contain.filter(|&f| f != pid), max_age) - else { + // If the active package has a stored conflict ... + let Some(store) = map.get(&pid) else { + continue; + }; + // then we need to check the corresponding subtrie. + let Some((o, age_o)) = store.find( + in_activation_slot, + must_contain.filter(|&f| f != pid), + max_age, + ) else { continue; }; let age = if must_contain == Some(pid) { @@ -80,7 +89,9 @@ impl ConflictStoreTrie { fn insert(&mut self, mut iter: impl Iterator, con: ConflictMap) { if let Some(pid) = iter.next() { if let ConflictStoreTrie::Node(p) = self { - p.entry(pid) + p.entry(pid.as_activations_key().clone()) + .or_default() + .entry(pid) .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new())) .insert(iter, con); } @@ -159,13 +170,13 @@ impl ConflictCache { pub fn find( &self, dep: &Dependency, - is_active: &impl Fn(PackageId) -> Option, + in_activation_slot: &impl Fn(&ActivationsKey) -> Option<(PackageId, usize)>, must_contain: Option, max_age: usize, ) -> Option<&ConflictMap> { self.con_from_dep .get(dep)? - .find(is_active, must_contain, max_age) + .find(in_activation_slot, must_contain, max_age) .map(|(c, _)| c) } /// Finds any known set of conflicts, if any, @@ -178,7 +189,12 @@ impl ConflictCache { dep: &Dependency, must_contain: Option, ) -> Option<&ConflictMap> { - let out = self.find(dep, &|id| cx.is_active(id), must_contain, usize::MAX); + let out = self.find( + dep, + &|id| cx.in_activation_slot(id), + must_contain, + usize::MAX, + ); if cfg!(debug_assertions) { if let Some(c) = &out { assert!(cx.is_conflicting(None, c).is_some()); @@ -197,6 +213,13 @@ impl ConflictCache { /// `dep` is known to be unresolvable if /// all the `PackageId` entries are activated. pub fn insert(&mut self, dep: &Dependency, con: &ConflictMap) { + if cfg!(debug_assertions) { + // Check that the iterator is sorted by activation key. + // `ConflictMap` is a `BTreeMap` so it is already sorted. + // But, if the ord on `ActivationsKey` changes to not match ord on `PackageId`, this will fail. + assert!(con.keys().is_sorted_by_key(|p| p.as_activations_key())) + } + self.con_from_dep .entry(dep.clone()) .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new())) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index d1bae02f8fa..0dc5568bb69 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -156,6 +156,12 @@ impl ResolverContext { .and_then(|(s, l)| if s.package_id() == id { Some(*l) } else { None }) } + /// If a package is active that has the same semver compatibility range + /// returns the `PackageId` and `ContextAge` when it was added + pub fn in_activation_slot(&self, id: &ActivationsKey) -> Option<(PackageId, ContextAge)> { + self.activations.get(id).map(|(s, l)| (s.package_id(), *l)) + } + /// Checks whether all of `parent` and the keys of `conflicting activations` /// are still active. /// If so returns the `ContextAge` when the newest one was added. diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index a35f2d4643b..0e8d57d8a8f 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -857,15 +857,16 @@ fn generalize_conflicting( .iter() .rev() // the last one to be tried is the least likely to be in the cache, so start with that. .map(|other| { + let other_activations_key = other.package_id().as_activations_key(); past_conflicting_activations .find( dep, &|id| { - if id == other.package_id() { + if id == &other_activations_key { // we are imagining that we used other instead - Some(backtrack_critical_age) + Some((other.package_id(), backtrack_critical_age)) } else { - cx.is_active(id) + cx.in_activation_slot(id) } }, Some(other.package_id()), From 0af09813e23a4c0fb67b0cf15e595ce3ac17dff4 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 11 Dec 2024 19:58:35 +0000 Subject: [PATCH 3/4] switch ord to match --- src/cargo/core/resolver/types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index 2248d79bbf3..470d420dbde 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -193,9 +193,9 @@ impl std::hash::Hash for ActivationsKey { /// same. #[derive(Clone, Copy, Eq, PartialEq, Hash, Debug, PartialOrd, Ord)] pub enum SemverCompatibility { - Major(NonZeroU64), - Minor(NonZeroU64), Patch(u64), + Minor(NonZeroU64), + Major(NonZeroU64), } impl From<&semver::Version> for SemverCompatibility { From fcf403b7c846ee6ea1ee05a3e472bf0e4f60c7fa Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 11 Dec 2024 20:25:05 +0000 Subject: [PATCH 4/4] just look up the Version --- src/cargo/core/resolver/conflict_cache.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index da31700be5a..fcca2fc1637 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -15,7 +15,10 @@ enum ConflictStoreTrie { /// A map from an element to a subtrie where /// all the sets in the subtrie contains that element. Node( - BTreeMap>, + BTreeMap< + ActivationsKey, + HashMap<&'static semver::Version, ConflictStoreTrie, rustc_hash::FxBuildHasher>, + >, ), } @@ -56,7 +59,7 @@ impl ConflictStoreTrie { continue; } // If the active package has a stored conflict ... - let Some(store) = map.get(&pid) else { + let Some(store) = map.get(pid.version()) else { continue; }; // then we need to check the corresponding subtrie. @@ -91,7 +94,7 @@ impl ConflictStoreTrie { if let ConflictStoreTrie::Node(p) = self { p.entry(pid.as_activations_key().clone()) .or_default() - .entry(pid) + .entry(pid.version()) .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new())) .insert(iter, con); }