From 4ac6c42286834fae582a50d6572cd95676c5fc99 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 20 Nov 2024 15:39:16 -0500 Subject: [PATCH] Create an arena for package names (#242) --- src/internal/arena.rs | 43 ++++++++ src/internal/core.rs | 66 ++++++------ src/internal/incompatibility.rs | 171 ++++++++++++++++++------------- src/internal/mod.rs | 2 +- src/internal/partial_solution.rs | 135 ++++++++++++------------ src/internal/small_map.rs | 21 ---- src/solver.rs | 77 ++++++++------ 7 files changed, 294 insertions(+), 221 deletions(-) diff --git a/src/internal/arena.rs b/src/internal/arena.rs index 6edb85df..e044bc37 100644 --- a/src/internal/arena.rs +++ b/src/internal/arena.rs @@ -3,6 +3,8 @@ use std::hash::{Hash, Hasher}; use std::marker::PhantomData; use std::ops::{Index, Range}; +type FnvIndexSet = indexmap::IndexSet; + /// The index of a value allocated in an arena that holds `T`s. /// /// The Clone, Copy and other traits are defined manually because @@ -124,3 +126,44 @@ impl Index>> for Arena { &self.data[(id.start.raw as usize)..(id.end.raw as usize)] } } + +/// Yet another index-based arena. This one de-duplicates entries by hashing. +/// +/// An arena is a kind of simple grow-only allocator, backed by a `Vec` +/// where all items have the same lifetime, making it easier +/// to have references between those items. +/// In this case the `Vec` is inside a `IndexSet` allowing fast lookup by value not just index. +/// They are all dropped at once when the arena is dropped. +#[derive(Clone, PartialEq, Eq)] +pub struct HashArena { + data: FnvIndexSet, +} + +impl fmt::Debug for HashArena { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + fmt.debug_struct("Arena") + .field("len", &self.data.len()) + .field("data", &self.data) + .finish() + } +} + +impl HashArena { + pub fn new() -> Self { + HashArena { + data: FnvIndexSet::default(), + } + } + + pub fn alloc(&mut self, value: T) -> Id { + let (raw, _) = self.data.insert_full(value); + Id::from(raw as u32) + } +} + +impl Index> for HashArena { + type Output = T; + fn index(&self, id: Id) -> &T { + &self.data[id.raw as usize] + } +} diff --git a/src/internal/core.rs b/src/internal/core.rs index 0e30d15c..15bf176a 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -7,19 +7,19 @@ use std::collections::HashSet as Set; use std::sync::Arc; use crate::internal::{ - Arena, DecisionLevel, IncompDpId, Incompatibility, PartialSolution, Relation, SatisfierSearch, - SmallVec, + Arena, DecisionLevel, HashArena, Id, IncompDpId, Incompatibility, PartialSolution, Relation, + SatisfierSearch, SmallVec, }; use crate::{DependencyProvider, DerivationTree, Map, NoSolutionError, VersionSet}; /// Current state of the PubGrub algorithm. #[derive(Clone)] pub(crate) struct State { - root_package: DP::P, + pub root_package: Id, root_version: DP::V, #[allow(clippy::type_complexity)] - incompatibilities: Map>>, + incompatibilities: Map, Vec>>, /// Store the ids of incompatibilities that are already contradicted. /// For each one keep track of the decision level when it was found to be contradicted. @@ -29,7 +29,7 @@ pub(crate) struct State { /// All incompatibilities expressing dependencies, /// with common dependents merged. #[allow(clippy::type_complexity)] - merged_dependencies: Map<(DP::P, DP::P), SmallVec>>, + merged_dependencies: Map<(Id, Id), SmallVec>>, /// Partial solution. /// TODO: remove pub. @@ -38,22 +38,27 @@ pub(crate) struct State { /// The store is the reference storage for all incompatibilities. pub(crate) incompatibility_store: Arena>, + /// The store is the reference storage for all packages. + pub(crate) package_store: HashArena, + /// This is a stack of work to be done in `unit_propagation`. /// It can definitely be a local variable to that method, but /// this way we can reuse the same allocation for better performance. - unit_propagation_buffer: SmallVec, + unit_propagation_buffer: SmallVec>, } impl State { /// Initialization of PubGrub state. pub(crate) fn init(root_package: DP::P, root_version: DP::V) -> Self { let mut incompatibility_store = Arena::new(); + let mut package_store = HashArena::new(); + let root_package = package_store.alloc(root_package); let not_root_id = incompatibility_store.alloc(Incompatibility::not_root( - root_package.clone(), + root_package, root_version.clone(), )); let mut incompatibilities = Map::default(); - incompatibilities.insert(root_package.clone(), vec![not_root_id]); + incompatibilities.insert(root_package, vec![not_root_id]); Self { root_package, root_version, @@ -61,6 +66,7 @@ impl State { contradicted_incompatibilities: Map::default(), partial_solution: PartialSolution::empty(), incompatibility_store, + package_store, unit_propagation_buffer: SmallVec::Empty, merged_dependencies: Map::default(), } @@ -75,18 +81,19 @@ impl State { /// Add an incompatibility to the state. pub(crate) fn add_incompatibility_from_dependencies( &mut self, - package: DP::P, + package: Id, version: DP::V, deps: impl IntoIterator, ) -> std::ops::Range> { // Create incompatibilities and allocate them in the store. let new_incompats_id_range = self.incompatibility_store - .alloc_iter(deps.into_iter().map(|dep| { + .alloc_iter(deps.into_iter().map(|(dep_p, dep_vs)| { + let dep_pid = self.package_store.alloc(dep_p); Incompatibility::from_dependency( - package.clone(), + package, ::singleton(version.clone()), - dep, + (dep_pid, dep_vs), ) })); // Merge the newly created incompatibilities with the older ones. @@ -98,7 +105,10 @@ impl State { /// Unit propagation is the core mechanism of the solving algorithm. /// CF - pub(crate) fn unit_propagation(&mut self, package: DP::P) -> Result<(), NoSolutionError> { + pub(crate) fn unit_propagation( + &mut self, + package: Id, + ) -> Result<(), NoSolutionError> { self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package); while let Some(current_package) = self.unit_propagation_buffer.pop() { @@ -120,7 +130,7 @@ impl State { Relation::Satisfied => { log::info!( "Start conflict resolution because incompat satisfied:\n {}", - current_incompat + current_incompat.display(&self.package_store) ); conflict_id = Some(incompat_id); break; @@ -131,7 +141,7 @@ impl State { // but so does allocating a hash map and hashing each item. // In practice `unit_propagation_buffer` is small enough that we can just do a linear scan. if !self.unit_propagation_buffer.contains(&package_almost) { - self.unit_propagation_buffer.push(package_almost.clone()); + self.unit_propagation_buffer.push(package_almost); } // Add (not term) to the partial solution with incompat as cause. self.partial_solution.add_derivation( @@ -157,7 +167,7 @@ impl State { self.build_derivation_tree(terminal_incompat_id) })?; self.unit_propagation_buffer.clear(); - self.unit_propagation_buffer.push(package_almost.clone()); + self.unit_propagation_buffer.push(package_almost); // Add to the partial solution with incompat as cause. self.partial_solution.add_derivation( package_almost, @@ -180,12 +190,12 @@ impl State { fn conflict_resolution( &mut self, incompatibility: IncompDpId, - ) -> Result<(DP::P, IncompDpId), IncompDpId> { + ) -> Result<(Id, IncompDpId), IncompDpId> { let mut current_incompat_id = incompatibility; let mut current_incompat_changed = false; loop { if self.incompatibility_store[current_incompat_id] - .is_terminal(&self.root_package, &self.root_version) + .is_terminal(self.root_package, &self.root_version) { return Err(current_incompat_id); } else { @@ -197,7 +207,6 @@ impl State { SatisfierSearch::DifferentDecisionLevels { previous_satisfier_level, } => { - let package = package.clone(); self.backtrack( current_incompat_id, current_incompat_changed, @@ -213,7 +222,7 @@ impl State { package, &self.incompatibility_store, ); - log::info!("prior cause: {}", prior_cause); + log::info!("prior cause: {}", prior_cause.display(&self.package_store)); current_incompat_id = self.incompatibility_store.alloc(prior_cause); current_incompat_changed = true; } @@ -256,19 +265,16 @@ impl State { fn merge_incompatibility(&mut self, mut id: IncompDpId) { if let Some((p1, p2)) = self.incompatibility_store[id].as_dependency() { // If we are a dependency, there's a good chance we can be merged with a previous dependency - let deps_lookup = self - .merged_dependencies - .entry((p1.clone(), p2.clone())) - .or_default(); + let deps_lookup = self.merged_dependencies.entry((p1, p2)).or_default(); if let Some((past, merged)) = deps_lookup.as_mut_slice().iter_mut().find_map(|past| { self.incompatibility_store[id] .merge_dependents(&self.incompatibility_store[*past]) .map(|m| (past, m)) }) { let new = self.incompatibility_store.alloc(merged); - for (pkg, _) in self.incompatibility_store[new].iter() { + for (&pkg, _) in self.incompatibility_store[new].iter() { self.incompatibilities - .entry(pkg.clone()) + .entry(pkg) .or_default() .retain(|id| id != past); } @@ -278,14 +284,11 @@ impl State { deps_lookup.push(id); } } - for (pkg, term) in self.incompatibility_store[id].iter() { + for (&pkg, term) in self.incompatibility_store[id].iter() { if cfg!(debug_assertions) { assert_ne!(term, &crate::term::Term::any()); } - self.incompatibilities - .entry(pkg.clone()) - .or_default() - .push(id); + self.incompatibilities.entry(pkg).or_default().push(id); } } @@ -320,6 +323,7 @@ impl State { id, &shared_ids, &self.incompatibility_store, + &self.package_store, &precomputed, ); precomputed.insert(id, Arc::new(tree)); diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 33b59cfe..428ab597 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -3,13 +3,13 @@ //! An incompatibility is a set of terms for different packages //! that should never be satisfied all together. -use std::fmt::{self, Debug, Display}; +use std::fmt::{Debug, Display}; use std::sync::Arc; -use crate::internal::{Arena, Id, SmallMap}; +use crate::internal::{Arena, HashArena, Id, SmallMap}; use crate::{ - term, DefaultStringReportFormatter, DependencyProvider, DerivationTree, Derived, External, Map, - Package, ReportFormatter, Set, Term, VersionSet, + term, DependencyProvider, DerivationTree, Derived, External, Map, Package, Set, Term, + VersionSet, }; /// An incompatibility is a set of terms for different packages @@ -29,7 +29,7 @@ use crate::{ /// [PubGrub documentation](https://github.com/dart-lang/pub/blob/master/doc/solver.md#incompatibility). #[derive(Debug, Clone)] pub(crate) struct Incompatibility { - package_terms: SmallMap>, + package_terms: SmallMap, Term>, kind: Kind, } @@ -48,12 +48,12 @@ enum Kind { /// /// This incompatibility drives the resolution, it requires that we pick the (virtual) root /// packages. - NotRoot(P, VS::V), + NotRoot(Id

, VS::V), /// There are no versions in the given range for this package. /// /// This incompatibility is used when we tried all versions in a range and no version /// worked, so we have to backtrack - NoVersions(P, VS), + NoVersions(Id

, VS), /// Incompatibility coming from the dependencies of a given package. /// /// If a@1 depends on b>=1,<2, we create an incompatibility with terms `{a 1, b <1,>=2}` with @@ -61,7 +61,7 @@ enum Kind { /// /// We can merge multiple dependents with the same version. For example, if a@1 depends on b and /// a@2 depends on b, we can say instead a@1||2 depends on b. - FromDependencyOf(P, VS, P, VS), + FromDependencyOf(Id

, VS, Id

, VS), /// Derived from two causes. Stores cause ids. /// /// For example, if a -> b and b -> c, we can derive a -> c. @@ -71,7 +71,7 @@ enum Kind { /// Examples: /// * The version would require building the package, but builds are disabled. /// * The package is not available in the cache, but internet access has been disabled. - Custom(P, VS, M), + Custom(Id

, VS, M), } /// A Relation describes how a set of terms can be compared to an incompatibility. @@ -83,20 +83,20 @@ pub(crate) enum Relation { Satisfied, /// We say that S contradicts I /// if S contradicts at least one term in I. - Contradicted(P), + Contradicted(Id

), /// If S satisfies all but one of I's terms and is inconclusive for the remaining term, /// we say S "almost satisfies" I and we call the remaining term the "unsatisfied term". - AlmostSatisfied(P), + AlmostSatisfied(Id

), /// Otherwise, we say that their relation is inconclusive. Inconclusive, } impl Incompatibility { /// Create the initial "not Root" incompatibility. - pub(crate) fn not_root(package: P, version: VS::V) -> Self { + pub(crate) fn not_root(package: Id

, version: VS::V) -> Self { Self { package_terms: SmallMap::One([( - package.clone(), + package, Term::Negative(VS::singleton(version.clone())), )]), kind: Kind::NotRoot(package, version), @@ -104,59 +104,59 @@ impl Incompatibilit } /// Create an incompatibility to remember that a given set does not contain any version. - pub(crate) fn no_versions(package: P, term: Term) -> Self { + pub(crate) fn no_versions(package: Id

, term: Term) -> Self { let set = match &term { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), }; Self { - package_terms: SmallMap::One([(package.clone(), term)]), + package_terms: SmallMap::One([(package, term)]), kind: Kind::NoVersions(package, set), } } /// Create an incompatibility for a reason outside pubgrub. #[allow(dead_code)] // Used by uv - pub(crate) fn custom_term(package: P, term: Term, metadata: M) -> Self { + pub(crate) fn custom_term(package: Id

, term: Term, metadata: M) -> Self { let set = match &term { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), }; Self { - package_terms: SmallMap::One([(package.clone(), term)]), + package_terms: SmallMap::One([(package, term)]), kind: Kind::Custom(package, set, metadata), } } /// Create an incompatibility for a reason outside pubgrub. - pub(crate) fn custom_version(package: P, version: VS::V, metadata: M) -> Self { + pub(crate) fn custom_version(package: Id

, version: VS::V, metadata: M) -> Self { let set = VS::singleton(version); let term = Term::Positive(set.clone()); Self { - package_terms: SmallMap::One([(package.clone(), term)]), + package_terms: SmallMap::One([(package, term)]), kind: Kind::Custom(package, set, metadata), } } /// Build an incompatibility from a given dependency. - pub(crate) fn from_dependency(package: P, versions: VS, dep: (P, VS)) -> Self { + pub(crate) fn from_dependency(package: Id

, versions: VS, dep: (Id

, VS)) -> Self { let (p2, set2) = dep; Self { package_terms: if set2 == VS::empty() { - SmallMap::One([(package.clone(), Term::Positive(versions.clone()))]) + SmallMap::One([(package, Term::Positive(versions.clone()))]) } else { SmallMap::Two([ - (package.clone(), Term::Positive(versions.clone())), - (p2.clone(), Term::Negative(set2.clone())), + (package, Term::Positive(versions.clone())), + (p2, Term::Negative(set2.clone())), ]) }, kind: Kind::FromDependencyOf(package, versions, p2, set2), } } - pub(crate) fn as_dependency(&self) -> Option<(&P, &P)> { + pub(crate) fn as_dependency(&self) -> Option<(Id

, Id

)> { match &self.kind { - Kind::FromDependencyOf(p1, _, p2, _) => Some((p1, p2)), + Kind::FromDependencyOf(p1, _, p2, _) => Some((*p1, *p2)), _ => None, } } @@ -186,40 +186,40 @@ impl Incompatibilit if dep_term != other.get(p2) { return None; } - return Some(Self::from_dependency( - p1.clone(), + Some(Self::from_dependency( + p1, self.get(p1) .unwrap() .unwrap_positive() .union(other.get(p1).unwrap().unwrap_positive()), // It is safe to `simplify` here ( - p2.clone(), + p2, dep_term.map_or(VS::empty(), |v| v.unwrap_negative().clone()), ), - )); + )) } /// Prior cause of two incompatibilities using the rule of resolution. pub(crate) fn prior_cause( incompat: Id, satisfier_cause: Id, - package: &P, + package: Id

, incompatibility_store: &Arena, ) -> Self { let kind = Kind::DerivedFrom(incompat, satisfier_cause); // Optimization to avoid cloning and dropping t1 let (t1, mut package_terms) = incompatibility_store[incompat] .package_terms - .split_one(package) + .split_one(&package) .unwrap(); let satisfier_cause_terms = &incompatibility_store[satisfier_cause].package_terms; package_terms.merge( - satisfier_cause_terms.iter().filter(|(p, _)| p != &package), + satisfier_cause_terms.iter().filter(|(p, _)| p != &&package), |t1, t2| Some(t1.intersection(t2)), ); - let term = t1.union(satisfier_cause_terms.get(package).unwrap()); + let term = t1.union(satisfier_cause_terms.get(&package).unwrap()); if term != Term::any() { - package_terms.insert(package.clone(), term); + package_terms.insert(package, term); } Self { package_terms, @@ -229,24 +229,24 @@ impl Incompatibilit /// Check if an incompatibility should mark the end of the algorithm /// because it satisfies the root package. - pub(crate) fn is_terminal(&self, root_package: &P, root_version: &VS::V) -> bool { + pub(crate) fn is_terminal(&self, root_package: Id

, root_version: &VS::V) -> bool { if self.package_terms.len() == 0 { true } else if self.package_terms.len() > 1 { false } else { let (package, term) = self.package_terms.iter().next().unwrap(); - (package == root_package) && term.contains(root_version) + (package == &root_package) && term.contains(root_version) } } /// Get the term related to a given package (if it exists). - pub(crate) fn get(&self, package: &P) -> Option<&Term> { - self.package_terms.get(package) + pub(crate) fn get(&self, package: Id

) -> Option<&Term> { + self.package_terms.get(&package) } /// Iterate over packages. - pub(crate) fn iter(&self) -> impl Iterator)> { + pub(crate) fn iter(&self) -> impl Iterator, &Term)> { self.package_terms.iter() } @@ -265,12 +265,17 @@ impl Incompatibilit self_id: Id, shared_ids: &Set>, store: &Arena, + package_store: &HashArena

, precomputed: &Map, Arc>>, ) -> DerivationTree { match store[self_id].kind.clone() { Kind::DerivedFrom(id1, id2) => { - let derived = Derived { - terms: store[self_id].package_terms.as_map(), + let derived: Derived = Derived { + terms: store[self_id] + .package_terms + .iter() + .map(|(&a, b)| (package_store[a].clone(), b.clone())) + .collect(), shared_id: shared_ids.get(&self_id).map(|id| id.into_raw()), cause1: precomputed .get(&id1) @@ -284,21 +289,22 @@ impl Incompatibilit DerivationTree::Derived(derived) } Kind::NotRoot(package, version) => { - DerivationTree::External(External::NotRoot(package, version)) - } - Kind::NoVersions(package, set) => { - DerivationTree::External(External::NoVersions(package.clone(), set.clone())) + DerivationTree::External(External::NotRoot(package_store[package].clone(), version)) } + Kind::NoVersions(package, set) => DerivationTree::External(External::NoVersions( + package_store[package].clone(), + set.clone(), + )), Kind::FromDependencyOf(package, set, dep_package, dep_set) => { DerivationTree::External(External::FromDependencyOf( - package.clone(), + package_store[package].clone(), set.clone(), - dep_package.clone(), + package_store[dep_package].clone(), dep_set.clone(), )) } Kind::Custom(package, set, metadata) => DerivationTree::External(External::Custom( - package.clone(), + package_store[package].clone(), set.clone(), metadata.clone(), )), @@ -310,13 +316,13 @@ impl<'a, P: Package, VS: VersionSet + 'a, M: Eq + Clone + Debug + Display + 'a> Incompatibility { /// CF definition of Relation enum. - pub(crate) fn relation(&self, terms: impl Fn(&P) -> Option<&'a Term>) -> Relation

{ + pub(crate) fn relation(&self, terms: impl Fn(Id

) -> Option<&'a Term>) -> Relation

{ let mut relation = Relation::Satisfied; - for (package, incompat_term) in self.package_terms.iter() { + for (&package, incompat_term) in self.package_terms.iter() { match terms(package).map(|term| incompat_term.relation_with(term)) { Some(term::Relation::Satisfied) => {} Some(term::Relation::Contradicted) => { - return Relation::Contradicted(package.clone()); + return Relation::Contradicted(package); } None | Some(term::Relation::Inconclusive) => { // If a package is not present, the intersection is the same as [Term::any]. @@ -325,7 +331,7 @@ impl<'a, P: Package, VS: VersionSet + 'a, M: Eq + Clone + Debug + Display + 'a> // but we systematically remove those from incompatibilities // so we're safe on that front. if relation == Relation::Satisfied { - relation = Relation::AlmostSatisfied(package.clone()); + relation = Relation::AlmostSatisfied(package); } else { return Relation::Inconclusive; } @@ -336,18 +342,35 @@ impl<'a, P: Package, VS: VersionSet + 'a, M: Eq + Clone + Debug + Display + 'a> } } -impl Display - for Incompatibility -{ - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!( - f, - "{}", - ReportFormatter::::format_terms( - &DefaultStringReportFormatter, - &self.package_terms.as_map() - ) - ) +impl Incompatibility { + pub fn display<'a>(&'a self, package_store: &'a HashArena

) -> impl Display + 'a { + match self.iter().collect::>().as_slice() { + [] => "version solving failed".into(), + // TODO: special case when that unique package is root. + [(package, Term::Positive(range))] => { + format!("{} {} is forbidden", package_store[**package], range) + } + [(package, Term::Negative(range))] => { + format!("{} {} is mandatory", package_store[**package], range) + } + [(p_pos, Term::Positive(r_pos)), (p_neg, Term::Negative(r_neg))] + | [(p_neg, Term::Negative(r_neg)), (p_pos, Term::Positive(r_pos))] => { + External::<_, _, M>::FromDependencyOf( + &package_store[**p_pos], + r_pos.clone(), + &package_store[**p_neg], + r_neg.clone(), + ) + .to_string() + } + slice => { + let str_terms: Vec<_> = slice + .iter() + .map(|(p, t)| format!("{} {}", package_store[**p], t)) + .collect(); + str_terms.join(", ") + " are incompatible" + } + } } } @@ -373,22 +396,26 @@ pub(crate) mod tests { #[test] fn rule_of_resolution(t1 in term_strat(), t2 in term_strat(), t3 in term_strat()) { let mut store = Arena::new(); + let mut package_store = HashArena::new(); + let p1 = package_store.alloc("p1"); + let p2 = package_store.alloc("p2"); + let p3 = package_store.alloc("p3"); let i1 = store.alloc(Incompatibility { - package_terms: SmallMap::Two([("p1", t1.clone()), ("p2", t2.negate())]), - kind: Kind::<_, _, String>::FromDependencyOf("p1", Ranges::full(), "p2", Ranges::full()) + package_terms: SmallMap::Two([(p1, t1.clone()), (p2, t2.negate())]), + kind: Kind::<_, _, String>::FromDependencyOf(p1, Ranges::full(), p2, Ranges::full()) }); let i2 = store.alloc(Incompatibility { - package_terms: SmallMap::Two([("p2", t2), ("p3", t3.clone())]), - kind: Kind::<_, _, String>::FromDependencyOf("p2", Ranges::full(), "p3", Ranges::full()) + package_terms: SmallMap::Two([(p2, t2), (p3, t3.clone())]), + kind: Kind::<_, _, String>::FromDependencyOf(p2, Ranges::full(), p3, Ranges::full()) }); let mut i3 = Map::default(); - i3.insert("p1", t1); - i3.insert("p3", t3); + i3.insert(p1, t1); + i3.insert(p3, t3); - let i_resolution = Incompatibility::prior_cause(i1, i2, &"p2", &store); - assert_eq!(i_resolution.package_terms.as_map(), i3); + let i_resolution = Incompatibility::prior_cause(i1, i2, p2, &store); + assert_eq!(i_resolution.package_terms.iter().map(|(&k, v)|(k, v.clone())).collect::>(), i3); } } diff --git a/src/internal/mod.rs b/src/internal/mod.rs index 643634f3..e10770d4 100644 --- a/src/internal/mod.rs +++ b/src/internal/mod.rs @@ -9,7 +9,7 @@ mod partial_solution; mod small_map; mod small_vec; -pub(crate) use arena::{Arena, Id}; +pub(crate) use arena::{Arena, HashArena, Id}; pub(crate) use core::State; pub(crate) use incompatibility::{IncompDpId, IncompId, Incompatibility, Relation}; pub(crate) use partial_solution::{DecisionLevel, PartialSolution, SatisfierSearch}; diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 44795582..7d4d2c8e 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -9,9 +9,10 @@ use std::hash::BuildHasherDefault; use priority_queue::PriorityQueue; use rustc_hash::FxHasher; -use super::small_vec::SmallVec; -use crate::internal::{Arena, IncompDpId, IncompId, Incompatibility, Relation, SmallMap}; -use crate::{DependencyProvider, Package, SelectedDependencies, Term, VersionSet}; +use crate::internal::{ + Arena, HashArena, Id, IncompDpId, IncompId, Incompatibility, Relation, SmallMap, SmallVec, +}; +use crate::{DependencyProvider, Package, Term, VersionSet}; type FnvIndexMap = indexmap::IndexMap>; @@ -41,30 +42,39 @@ pub(crate) struct PartialSolution { /// the last time `prioritize` has been called. The inverse is not necessarily true, some packages in the range /// did not have a change. Within this range there is no sorting. #[allow(clippy::type_complexity)] - package_assignments: FnvIndexMap>, + package_assignments: FnvIndexMap, PackageAssignments>, /// `prioritized_potential_packages` is primarily a HashMap from a package with no desition and a positive assignment /// to its `Priority`. But, it also maintains a max heap of packages by `Priority` order. prioritized_potential_packages: - PriorityQueue>, + PriorityQueue, DP::Priority, BuildHasherDefault>, changed_this_decision_level: usize, has_ever_backtracked: bool, } -impl Display for PartialSolution { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let mut assignments: Vec<_> = self - .package_assignments - .iter() - .map(|(p, pa)| format!("{}: {}", p, pa)) - .collect(); - assignments.sort(); - write!( - f, - "next_global_index: {}\ncurrent_decision_level: {:?}\npackage_assignments:\n{}", - self.next_global_index, - self.current_decision_level, - assignments.join("\t\n") - ) +impl PartialSolution { + pub fn display<'a>(&'a self, package_store: &'a HashArena) -> impl Display + 'a { + struct PSDisplay<'a, DP: DependencyProvider>(&'a PartialSolution, &'a HashArena); + + impl Display for PSDisplay<'_, DP> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut assignments: Vec<_> = self + .0 + .package_assignments + .iter() + .map(|(p, pa)| format!("{:?} = '{}': {}", p, self.1[*p], pa)) + .collect(); + assignments.sort(); + write!( + f, + "next_global_index: {}\ncurrent_decision_level: {:?}\npackage_assignments:\n{}", + self.0.next_global_index, + self.0.current_decision_level, + assignments.join("\t\n") + ) + } + } + + PSDisplay(self, package_store) } } @@ -142,7 +152,7 @@ pub(crate) enum SatisfierSearch = SmallMap<&'i P, (Option>, u32, DecisionLevel)>; +type SatisfiedMap = SmallMap, (Option>, u32, DecisionLevel)>; impl PartialSolution { /// Initialize an empty PartialSolution. @@ -158,7 +168,7 @@ impl PartialSolution { } /// Add a decision. - pub(crate) fn add_decision(&mut self, package: DP::P, version: DP::V) { + pub(crate) fn add_decision(&mut self, package: Id, version: DP::V) { // Check that add_decision is never used in the wrong context. if cfg!(debug_assertions) { match self.package_assignments.get_mut(&package) { @@ -170,7 +180,7 @@ impl PartialSolution { AssignmentsIntersection::Derivations(term) => { debug_assert!( term.contains(&version), - "{}: {} was expected to be contained in {}", + "{:?}: {} was expected to be contained in {}", package, version, term, @@ -205,7 +215,7 @@ impl PartialSolution { /// Add a derivation. pub(crate) fn add_derivation( &mut self, - package: DP::P, + package: Id, cause: IncompDpId, store: &Arena>, ) { @@ -214,7 +224,7 @@ impl PartialSolution { global_index: self.next_global_index, decision_level: self.current_decision_level, cause, - accumulated_intersection: store[cause].get(&package).unwrap().negate(), + accumulated_intersection: store[cause].get(package).unwrap().negate(), }; self.next_global_index += 1; let pa_last_index = self.package_assignments.len().saturating_sub(1); @@ -259,8 +269,8 @@ impl PartialSolution { pub(crate) fn pick_highest_priority_pkg( &mut self, - prioritizer: impl Fn(&DP::P, &DP::VS) -> DP::Priority, - ) -> Option { + prioritizer: impl Fn(Id, &DP::VS) -> DP::Priority, + ) -> Option> { let check_all = self.changed_this_decision_level == self.current_decision_level.0.saturating_sub(1) as usize; let current_decision_level = self.current_decision_level; @@ -276,10 +286,10 @@ impl PartialSolution { // or if we backtracked in the meantime. check_all || pa.highest_decision_level == current_decision_level }) - .filter_map(|(p, pa)| pa.assignments_intersection.potential_package_filter(p)) + .filter_map(|(&p, pa)| pa.assignments_intersection.potential_package_filter(p)) .for_each(|(p, r)| { let priority = prioritizer(p, r); - prioritized_potential_packages.push(p.clone(), priority); + prioritized_potential_packages.push(p, priority); }); self.changed_this_decision_level = self.package_assignments.len(); prioritized_potential_packages.pop().map(|(p, _)| p) @@ -288,23 +298,22 @@ impl PartialSolution { /// If a partial solution has, for every positive derivation, /// a corresponding decision that satisfies that assignment, /// it's a total solution and version solving has succeeded. - pub(crate) fn extract_solution(&self) -> SelectedDependencies { + pub(crate) fn extract_solution(&self) -> impl Iterator, DP::V)> + '_ { self.package_assignments .iter() .take(self.current_decision_level.0 as usize) - .map(|(p, pa)| match &pa.assignments_intersection { - AssignmentsIntersection::Decision((_, v, _)) => (p.clone(), v.clone()), + .map(|(&p, pa)| match &pa.assignments_intersection { + AssignmentsIntersection::Decision((_, v, _)) => (p, v.clone()), AssignmentsIntersection::Derivations(_) => { panic!("Derivations in the Decision part") } }) - .collect() } /// Backtrack the partial solution to a given decision level. pub(crate) fn backtrack(&mut self, decision_level: DecisionLevel) { self.current_decision_level = decision_level; - self.package_assignments.retain(|_p, pa| { + self.package_assignments.retain(|_, pa| { if pa.smallest_decision_level > decision_level { // Remove all entries that have a smallest decision level higher than the backtrack target. false @@ -350,7 +359,7 @@ impl PartialSolution { /// is already in the partial solution with an incompatible version. pub(crate) fn add_version( &mut self, - package: DP::P, + package: Id, version: DP::V, new_incompatibilities: std::ops::Range>, store: &Arena>, @@ -359,14 +368,14 @@ impl PartialSolution { // Nothing has yet gone wrong during this resolution. This call is unlikely to be the first problem. // So let's live with a little bit of risk and add the decision without checking the dependencies. // The worst that can happen is we will have to do a full backtrack which only removes this one decision. - log::info!("add_decision: {package} @ {version} without checking dependencies"); + log::info!("add_decision: {package:?} @ {version} without checking dependencies"); self.add_decision(package, version); } else { // Check if any of the new dependencies preclude deciding on this crate version. let exact = Term::exact(version.clone()); let not_satisfied = |incompat: &Incompatibility| { incompat.relation(|p| { - if p == &package { + if p == package { Some(&exact) } else { self.term_intersection_for_package(p) @@ -377,14 +386,10 @@ impl PartialSolution { // Check none of the dependencies (new_incompatibilities) // would create a conflict (be satisfied). if store[new_incompatibilities].iter().all(not_satisfied) { - log::info!("add_decision: {} @ {}", package, version); + log::info!("add_decision: {package:?} @ {version}"); self.add_decision(package, version); } else { - log::info!( - "not adding {} @ {} because of its dependencies", - package, - version - ); + log::info!("not adding {package:?} @ {version} because of its dependencies",); } } } @@ -398,19 +403,22 @@ impl PartialSolution { } /// Retrieve intersection of terms related to package. - pub(crate) fn term_intersection_for_package(&self, package: &DP::P) -> Option<&Term> { + pub(crate) fn term_intersection_for_package( + &self, + package: Id, + ) -> Option<&Term> { self.package_assignments - .get(package) + .get(&package) .map(|pa| pa.assignments_intersection.term()) } /// Figure out if the satisfier and previous satisfier are of different decision levels. #[allow(clippy::type_complexity)] - pub(crate) fn satisfier_search<'i>( + pub(crate) fn satisfier_search( &self, - incompat: &'i Incompatibility, + incompat: &Incompatibility, store: &Arena>, - ) -> (&'i DP::P, SatisfierSearch) { + ) -> (Id, SatisfierSearch) { let satisfied_map = Self::find_satisfier(incompat, &self.package_assignments); let (&satisfier_package, &(satisfier_cause, _, satisfier_decision_level)) = satisfied_map .iter() @@ -445,13 +453,13 @@ impl PartialSolution { /// It would be nice if we could get rid of it, but I don't know if then it will be possible /// to return a coherent previous_satisfier_level. #[allow(clippy::type_complexity)] - fn find_satisfier<'i>( - incompat: &'i Incompatibility, - package_assignments: &FnvIndexMap>, - ) -> SatisfiedMap<'i, DP::P, DP::VS, DP::M> { + fn find_satisfier( + incompat: &Incompatibility, + package_assignments: &FnvIndexMap, PackageAssignments>, + ) -> SatisfiedMap { let mut satisfied = SmallMap::Empty; - for (package, incompat_term) in incompat.iter() { - let pa = package_assignments.get(package).expect("Must exist"); + for (&package, incompat_term) in incompat.iter() { + let pa = package_assignments.get(&package).expect("Must exist"); satisfied.insert(package, pa.satisfier(package, &incompat_term.negate())); } satisfied @@ -461,15 +469,15 @@ impl PartialSolution { /// such that incompatibility is satisfied by the partial solution up to /// and including that assignment plus satisfier. #[allow(clippy::type_complexity)] - fn find_previous_satisfier<'i>( + fn find_previous_satisfier( incompat: &Incompatibility, - satisfier_package: &'i DP::P, - mut satisfied_map: SatisfiedMap<'i, DP::P, DP::VS, DP::M>, - package_assignments: &FnvIndexMap>, + satisfier_package: Id, + mut satisfied_map: SatisfiedMap, + package_assignments: &FnvIndexMap, PackageAssignments>, store: &Arena>, ) -> DecisionLevel { // First, let's retrieve the previous derivations and the initial accum_term. - let satisfier_pa = package_assignments.get(satisfier_package).unwrap(); + let satisfier_pa = package_assignments.get(&satisfier_package).unwrap(); let (satisfier_cause, _gidx, _dl) = satisfied_map.get(&satisfier_package).unwrap(); let accum_term = if let &Some(cause) = satisfier_cause { @@ -509,7 +517,7 @@ impl PartialSolution { impl PackageAssignments { fn satisfier( &self, - package: &P, + package: Id

, start_term: &Term, ) -> (Option>, u32, DecisionLevel) { let empty = Term::empty(); @@ -531,7 +539,7 @@ impl PackageAssignm AssignmentsIntersection::Derivations(accumulated_intersection) => { unreachable!( concat!( - "while processing package {}: ", + "while processing package {:?}: ", "accum_term = {} has overlap with incompat_term = {}, ", "which means the last assignment should have been a decision, ", "but instead it was a derivation. This shouldn't be possible! ", @@ -557,10 +565,7 @@ impl AssignmentsIntersection { /// selected version (no "decision") /// and if it contains at least one positive derivation term /// in the partial solution. - fn potential_package_filter<'a, P: Package>( - &'a self, - package: &'a P, - ) -> Option<(&'a P, &'a VS)> { + fn potential_package_filter(&self, package: Id

) -> Option<(Id

, &VS)> { match self { Self::Decision(_) => None, Self::Derivations(term_intersection) => { diff --git a/src/internal/small_map.rs b/src/internal/small_map.rs index 2b6f3b10..ed66409a 100644 --- a/src/internal/small_map.rs +++ b/src/internal/small_map.rs @@ -190,27 +190,6 @@ impl SmallMap { } } -impl SmallMap { - pub(crate) fn as_map(&self) -> Map { - match self { - Self::Empty => Map::default(), - Self::One([(k, v)]) => { - let mut map = Map::with_capacity_and_hasher(1, Default::default()); - map.insert(k.clone(), v.clone()); - map - } - Self::Two(data) => { - let mut map = Map::with_capacity_and_hasher(2, Default::default()); - for (k, v) in data { - map.insert(k.clone(), v.clone()); - } - map - } - Self::Flexible(data) => data.clone(), - } - } -} - enum IterSmallMap<'a, K, V> { Inline(std::slice::Iter<'a, (K, V)>), Map(std::collections::hash_map::Iter<'a, K, V>), diff --git a/src/solver.rs b/src/solver.rs index 0f61fbf4..ac032913 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -65,7 +65,7 @@ use std::fmt::{Debug, Display}; use log::{debug, info}; -use crate::internal::{Incompatibility, State}; +use crate::internal::{Id, Incompatibility, State}; use crate::{DependencyConstraints, Map, Package, PubGrubError, SelectedDependencies, VersionSet}; /// Main function of the library. @@ -76,44 +76,59 @@ pub fn resolve( version: impl Into, ) -> Result, PubGrubError> { let mut state: State = State::init(package.clone(), version.into()); - let mut added_dependencies: Map> = Map::default(); - let mut next = package; + let mut added_dependencies: Map, Set> = Map::default(); + let mut next = state.root_package; loop { dependency_provider .should_cancel() - .map_err(PubGrubError::ErrorInShouldCancel)?; + .map_err(|err| PubGrubError::ErrorInShouldCancel(err))?; - info!("unit_propagation: {}", &next); + info!( + "unit_propagation: {:?} = '{}'", + &next, state.package_store[next] + ); state.unit_propagation(next)?; debug!( "Partial solution after unit propagation: {}", - state.partial_solution + state.partial_solution.display(&state.package_store) ); - let Some(highest_priority_pkg) = state - .partial_solution - .pick_highest_priority_pkg(|p, r| dependency_provider.prioritize(p, r)) + let Some(highest_priority_pkg) = + state.partial_solution.pick_highest_priority_pkg(|p, r| { + dependency_provider.prioritize(&state.package_store[p], r) + }) else { - return Ok(state.partial_solution.extract_solution()); + return Ok(state + .partial_solution + .extract_solution() + .map(|(p, v)| (state.package_store[p].clone(), v)) + .collect()); }; next = highest_priority_pkg; let term_intersection = state .partial_solution - .term_intersection_for_package(&next) + .term_intersection_for_package(next) .ok_or_else(|| { PubGrubError::Failure("a package was chosen but we don't have a term.".into()) })?; let decision = dependency_provider - .choose_version(&next, term_intersection.unwrap_positive()) + .choose_version( + &state.package_store[next], + term_intersection.unwrap_positive(), + ) .map_err(PubGrubError::ErrorChoosingPackageVersion)?; - info!("DP chose: {} @ {:?}", next, decision); + + info!( + "DP chose: {:?} = '{}' @ {:?}", + &next, state.package_store[next], decision + ); // Pick the next compatible version. let v = match decision { None => { - let inc = Incompatibility::no_versions(next.clone(), term_intersection.clone()); + let inc = Incompatibility::no_versions(next, term_intersection.clone()); state.add_incompatibility(inc); continue; } @@ -127,25 +142,25 @@ pub fn resolve( } let is_new_dependency = added_dependencies - .entry(next.clone()) + .entry(next) .or_default() .insert(v.clone()); if is_new_dependency { // Retrieve that package dependencies. - let p = &next; - let dependencies = dependency_provider.get_dependencies(p, &v).map_err(|err| { - PubGrubError::ErrorRetrievingDependencies { - package: p.clone(), + let p = next; + let dependencies = dependency_provider + .get_dependencies(&state.package_store[p], &v) + .map_err(|err| PubGrubError::ErrorRetrievingDependencies { + package: state.package_store[p].clone(), version: v.clone(), source: err, - } - })?; + })?; let dependencies = match dependencies { Dependencies::Unavailable(reason) => { state.add_incompatibility(Incompatibility::custom_version( - p.clone(), + p, v.clone(), reason, )); @@ -156,19 +171,19 @@ pub fn resolve( // Add that package and version if the dependencies are not problematic. let dep_incompats = - state.add_incompatibility_from_dependencies(p.clone(), v.clone(), dependencies); + state.add_incompatibility_from_dependencies(p, v.clone(), dependencies); - state.partial_solution.add_version( - p.clone(), - v.clone(), - dep_incompats, - &state.incompatibility_store, - ); + state + .partial_solution + .add_version(p, v, dep_incompats, &state.incompatibility_store); } else { // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied // terms and can add the decision directly. - info!("add_decision (not first time): {} @ {}", &next, v); - state.partial_solution.add_decision(next.clone(), v); + info!( + "add_decision (not first time): {:?} = '{}' @ {}", + &next, state.package_store[next], v + ); + state.partial_solution.add_decision(next, v); } } }