diff --git a/src/error.rs b/src/error.rs index ce193766..d784ed0a 100644 --- a/src/error.rs +++ b/src/error.rs @@ -42,10 +42,6 @@ pub enum PubGrubError { /// returned an error in the method [should_cancel](DependencyProvider::should_cancel). #[error("We should cancel")] ErrorInShouldCancel(#[source] DP::Err), - - /// Something unexpected happened. - #[error("{0}")] - Failure(String), } impl From> for PubGrubError { @@ -78,7 +74,6 @@ where Self::ErrorInShouldCancel(arg0) => { f.debug_tuple("ErrorInShouldCancel").field(arg0).finish() } - Self::Failure(arg0) => f.debug_tuple("Failure").field(arg0).finish(), } } } diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 2ae6cae8..213893b2 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -16,6 +16,7 @@ use crate::internal::{ use crate::{DependencyProvider, Package, Term, VersionSet}; type FnvIndexMap = indexmap::IndexMap>; +type FnvIndexSet = indexmap::IndexSet>; #[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] pub(crate) struct DecisionLevel(pub(crate) u32); @@ -36,9 +37,9 @@ pub(crate) struct PartialSolution { /// Store for all known package decisions and package derivations. /// /// "assignment" refers to both packages with decisions and package with only derivations and - /// no decision yet. We combine this in a single index map, where different sections (of - /// indexes) contain package with different level of information, and make a decision moves a - /// package from the derivations sections to the decisions section. + /// no decision yet. We combine this in a single index map with a decisions section in front and + /// a derivations sections in the back, where making a decision moves a package from the + /// derivations sections to the decisions section. /// /// `[..current_decision_level]`: Packages that have had a decision made, sorted by the /// `decision_level`. The section is can be seen as the partial solution, it contains a @@ -46,24 +47,11 @@ pub(crate) struct PartialSolution { /// extract the solution, and to backtrack to a particular decision level. The /// `AssignmentsIntersection` is always a `Decision`. /// - /// `[prioritize_decision_level..]`: Packages that are dependencies of some other package, + /// `[current_decision_level..]`: Packages that are dependencies of some other package, /// but have not yet been decided. The `AssignmentsIntersection` is always a `Derivations`, the - /// derivations store the obligations from the decided packages. This section has two - /// subsections to optimize the number of `prioritize` calls: - /// - /// `[current_decision_level..prioritize_decision_level]`: The assignments of packages in this - /// range have not changed since the last time `prioritize` has been called, their - /// priority in `prioritized_potential_packages` is fresh. There is no sorting within this - /// range. - /// - /// `[prioritize_decision_level..]`: The assignments of packages in this range may have changed - /// since the last time `prioritize` has been called, their priority in - /// `prioritized_potential_packages` needs to be refreshed. There is no sorting within this - /// range. + /// derivations store the obligations from the decided packages. #[allow(clippy::type_complexity)] package_assignments: FnvIndexMap, PackageAssignments>, - /// Index into `package_assignments` to decide which packages need to be re-prioritized. - prioritize_decision_level: usize, /// The undecided packages order by their `Priority`. /// /// The max heap allows quickly `pop`ing the highest priority package. @@ -73,6 +61,9 @@ pub(crate) struct PartialSolution { #[allow(clippy::type_complexity)] prioritized_potential_packages: PriorityQueue, (DP::Priority, Reverse), BuildHasherDefault>, + /// Packages whose derivations changed since the last time `prioritize` was called and need + /// their priorities to be updated. + outdated_priorities: FnvIndexSet>, /// Whether we have never backtracked, to enable fast path optimizations. has_ever_backtracked: bool, } @@ -180,7 +171,7 @@ impl PartialSolution { current_decision_level: DecisionLevel(0), package_assignments: FnvIndexMap::default(), prioritized_potential_packages: PriorityQueue::default(), - prioritize_decision_level: 0, + outdated_priorities: FnvIndexSet::default(), has_ever_backtracked: false, } } @@ -233,10 +224,6 @@ impl PartialSolution { } }, } - assert_eq!( - self.prioritize_decision_level, - self.package_assignments.len() - ); } let new_idx = self.current_decision_level.0 as usize; self.current_decision_level = self.current_decision_level.increment(); @@ -272,10 +259,8 @@ impl PartialSolution { accumulated_intersection: store[cause].get(package).unwrap().negate(), }; self.next_global_index += 1; - let pa_last_index = self.package_assignments.len().saturating_sub(1); match self.package_assignments.entry(package) { Entry::Occupied(mut occupied) => { - let idx = occupied.index(); let pa = occupied.get_mut(); pa.highest_decision_level = self.current_decision_level; match &mut pa.assignments_intersection { @@ -287,10 +272,7 @@ impl PartialSolution { *t = t.intersection(&dated_derivation.accumulated_intersection); dated_derivation.accumulated_intersection = t.clone(); if t.is_positive() { - // we can use `swap_indices` to make `prioritize_decision_level` only go down by 1 - // but the copying is slower then the larger search - self.prioritize_decision_level = - std::cmp::min(self.prioritize_decision_level, idx); + self.outdated_priorities.insert(package); } } } @@ -299,8 +281,7 @@ impl PartialSolution { Entry::Vacant(v) => { let term = dated_derivation.accumulated_intersection.clone(); if term.is_positive() { - self.prioritize_decision_level = - std::cmp::min(self.prioritize_decision_level, pa_last_index); + self.outdated_priorities.insert(package); } v.insert(PackageAssignments { smallest_decision_level: self.current_decision_level, @@ -316,29 +297,27 @@ impl PartialSolution { pub(crate) fn pick_highest_priority_pkg( &mut self, mut prioritizer: impl FnMut(Id, &DP::VS) -> DP::Priority, - ) -> Option> { - let check_all = self.prioritize_decision_level - == self.current_decision_level.0.saturating_sub(1) as usize; - let current_decision_level = self.current_decision_level; + ) -> Option<(Id, &DP::VS)> { let prioritized_potential_packages = &mut self.prioritized_potential_packages; - self.package_assignments - .get_range(self.prioritize_decision_level..) - .unwrap() - .iter() - .filter(|(_, pa)| { - // We only actually need to update the package if it has been changed - // since the last time we called prioritize. - // Which means it's highest decision level is the current decision level, - // 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)) - .for_each(|(p, r)| { - let priority = prioritizer(p, r); - prioritized_potential_packages.push(p, (priority, Reverse(p.into_raw() as u32))); - }); - self.prioritize_decision_level = self.package_assignments.len(); - prioritized_potential_packages.pop().map(|(p, _)| p) + while let Some(p) = self.outdated_priorities.pop() { + let Some(pa) = self.package_assignments.get(&p) else { + continue; + }; + let Some(r) = pa.assignments_intersection.potential_package_filter() else { + continue; + }; + let priority = prioritizer(p, r); + prioritized_potential_packages.push(p, (priority, Reverse(p.into_raw() as u32))); + } + while let Some(p) = self.prioritized_potential_packages.pop().map(|(p, _)| p) { + let Some(pa) = self.package_assignments.get(&p) else { + continue; + }; + if let Some(r) = pa.assignments_intersection.potential_package_filter() { + return Some((p, r)); + } + } + None } /// If a partial solution has, for every positive derivation, @@ -378,12 +357,20 @@ impl PartialSolution { /// 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(|_, pa| { + self.package_assignments.retain(|p, pa| { if pa.smallest_decision_level > decision_level { // Remove all entries that have a smallest decision level higher than the backtrack target. false } else if pa.highest_decision_level <= decision_level { // Do not change entries older than the backtrack decision level target. + if pa + .assignments_intersection + .potential_package_filter() + .is_some() + && self.prioritized_potential_packages.get(p).is_none() + { + self.outdated_priorities.insert(*p); + } true } else { // smallest_decision_level <= decision_level < highest_decision_level @@ -408,12 +395,14 @@ impl PartialSolution { // Reset the assignments intersection. pa.assignments_intersection = AssignmentsIntersection::Derivations(last.accumulated_intersection.clone()); + + self.prioritized_potential_packages.remove(p); + if pa.assignments_intersection.term().is_positive() { + self.outdated_priorities.insert(*p); + } true } }); - // Throw away all stored priority levels, And mark that they all need to be recomputed. - self.prioritized_potential_packages.clear(); - self.prioritize_decision_level = self.current_decision_level.0.saturating_sub(1) as usize; self.has_ever_backtracked = true; } @@ -648,12 +637,12 @@ impl AssignmentsIntersection { /// selected version (no "decision") /// and if it contains at least one positive derivation term /// in the partial solution. - fn potential_package_filter(&self, package: Id

) -> Option<(Id

, &VS)> { + fn potential_package_filter(&self) -> Option<&VS> { match self { Self::Decision { .. } => None, Self::Derivations(term_intersection) => { if term_intersection.is_positive() { - Some((package, term_intersection.unwrap_positive())) + Some(term_intersection.unwrap_positive()) } else { None } diff --git a/src/solver.rs b/src/solver.rs index d30b2147..eb09371b 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -66,7 +66,9 @@ use std::fmt::{Debug, Display}; use log::{debug, info}; use crate::internal::{Id, Incompatibility, State}; -use crate::{DependencyConstraints, Map, Package, PubGrubError, SelectedDependencies, VersionSet}; +use crate::{ + DependencyConstraints, Map, Package, PubGrubError, SelectedDependencies, Term, VersionSet, +}; /// Statistics on how often a package conflicted with other packages. #[derive(Debug, Default, Clone)] @@ -148,7 +150,7 @@ pub fn resolve( state.partial_solution.display(&state.package_store) ); - let Some(highest_priority_pkg) = + let Some((highest_priority_pkg, term_intersection)) = state.partial_solution.pick_highest_priority_pkg(|p, r| { dependency_provider.prioritize( &state.package_store[p], @@ -165,17 +167,8 @@ pub fn resolve( }; next = highest_priority_pkg; - let term_intersection = state - .partial_solution - .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( - &state.package_store[next], - term_intersection.unwrap_positive(), - ) + .choose_version(&state.package_store[next], term_intersection) .map_err(PubGrubError::ErrorChoosingPackageVersion)?; info!( @@ -186,7 +179,8 @@ pub fn resolve( // Pick the next compatible version. let v = match decision { None => { - let inc = Incompatibility::no_versions(next, term_intersection.clone()); + let inc = + Incompatibility::no_versions(next, Term::Positive(term_intersection.clone())); state.add_incompatibility(inc); continue; } @@ -194,9 +188,10 @@ pub fn resolve( }; if !term_intersection.contains(&v) { - return Err(PubGrubError::Failure( - "choose_package_version picked an incompatible version".into(), - )); + panic!( + "`choose_version` picked an incompatible version for package {}, {} is not in {}", + state.package_store[next], v, term_intersection + ); } let is_new_dependency = added_dependencies