From 4a483710a32318ece97e0de54096147291a3fc5b Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 3 Jan 2024 13:40:52 -0500 Subject: [PATCH] perf: fewer intersections in satisfier (#174) * check that nothing changed * inline subset_of * Apply !incompat_term to both sides * T.intersection( !T ) == empty * precomputed start_term.intersection(&incompat_term.negate()) * move the checking code * switch to partition_point * remove the option * compute intersection_term outside of satisfier * intersection with any is self * remove test code * remove unused arguments * rename vars * dont clone P * nothing meaningful * use cause not index * clippy --- src/internal/core.rs | 3 +- src/internal/partial_solution.rs | 105 ++++++++++++++----------------- src/term.rs | 1 + 3 files changed, 49 insertions(+), 60 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 147a5a22..38bdca17 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -194,6 +194,7 @@ impl State { DifferentDecisionLevels { previous_satisfier_level, } => { + let package = package.clone(); self.backtrack( current_incompat_id, current_incompat_changed, @@ -206,7 +207,7 @@ impl State { let prior_cause = Incompatibility::prior_cause( current_incompat_id, satisfier_cause, - &package, + package, &self.incompatibility_store, ); log::info!("prior cause: {}", prior_cause); diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 68d4b41e..cf1d24db 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -143,6 +143,8 @@ pub enum SatisfierSearch { }, } +type SatisfiedMap<'i, P, VS> = SmallMap<&'i P, (Option>, u32, DecisionLevel)>; + impl PartialSolution { /// Initialize an empty PartialSolution. pub fn empty() -> Self { @@ -390,37 +392,33 @@ impl PartialSolution( &self, - incompat: &Incompatibility, + incompat: &'i Incompatibility, store: &Arena>, - ) -> (P, SatisfierSearch) { + ) -> (&'i P, SatisfierSearch) { let satisfied_map = Self::find_satisfier(incompat, &self.package_assignments); - let (satisfier_package, &(satisfier_index, _, satisfier_decision_level)) = satisfied_map + let (&satisfier_package, &(satisfier_cause, _, satisfier_decision_level)) = satisfied_map .iter() .max_by_key(|(_p, (_, global_index, _))| global_index) .unwrap(); - let satisfier_package = satisfier_package.clone(); let previous_satisfier_level = Self::find_previous_satisfier( incompat, - &satisfier_package, + satisfier_package, satisfied_map, &self.package_assignments, store, ); - if previous_satisfier_level < satisfier_decision_level { - let search_result = SatisfierSearch::DifferentDecisionLevels { - previous_satisfier_level, - }; - (satisfier_package, search_result) + let search_result = if previous_satisfier_level >= satisfier_decision_level { + SatisfierSearch::SameDecisionLevels { + satisfier_cause: satisfier_cause.unwrap(), + } } else { - let satisfier_pa = self.package_assignments.get(&satisfier_package).unwrap(); - let dd = &satisfier_pa.dated_derivations[satisfier_index]; - let search_result = SatisfierSearch::SameDecisionLevels { - satisfier_cause: dd.cause, - }; - (satisfier_package, search_result) - } + SatisfierSearch::DifferentDecisionLevels { + previous_satisfier_level, + } + }; + (satisfier_package, search_result) } /// A satisfier is the earliest assignment in partial solution such that the incompatibility @@ -432,17 +430,14 @@ impl PartialSolution, + fn find_satisfier<'i>( + incompat: &'i Incompatibility, package_assignments: &FnvIndexMap>, - ) -> SmallMap { + ) -> SatisfiedMap<'i, P, VS> { let mut satisfied = SmallMap::Empty; for (package, incompat_term) in incompat.iter() { let pa = package_assignments.get(package).expect("Must exist"); - satisfied.insert( - package.clone(), - pa.satisfier(package, incompat_term, &Term::any()), - ); + satisfied.insert(package, pa.satisfier(package, &incompat_term.negate())); } satisfied } @@ -450,25 +445,24 @@ impl PartialSolution( incompat: &Incompatibility, - satisfier_package: &P, - mut satisfied_map: SmallMap, + satisfier_package: &'i P, + mut satisfied_map: SatisfiedMap<'i, P, VS>, package_assignments: &FnvIndexMap>, 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_index, _gidx, _dl) = satisfied_map.get_mut(satisfier_package).unwrap(); + let (satisfier_cause, _gidx, _dl) = satisfied_map.get(&satisfier_package).unwrap(); - let accum_term = if *satisfier_index == satisfier_pa.dated_derivations.len() { + let accum_term = if let &Some(cause) = satisfier_cause { + store[cause].get(satisfier_package).unwrap().negate() + } else { match &satisfier_pa.assignments_intersection { AssignmentsIntersection::Derivations(_) => panic!("must be a decision"), AssignmentsIntersection::Decision((_, _, term)) => term.clone(), } - } else { - let dd = &satisfier_pa.dated_derivations[*satisfier_index]; - store[dd.cause].get(satisfier_package).unwrap().negate() }; let incompat_term = incompat @@ -476,8 +470,11 @@ impl PartialSolution PackageAssignments { fn satisfier( &self, package: &P, - incompat_term: &Term, start_term: &Term, - ) -> (usize, u32, DecisionLevel) { + ) -> (Option>, u32, DecisionLevel) { + let empty = Term::empty(); // Indicate if we found a satisfier in the list of derivations, otherwise it will be the decision. - for (idx, dated_derivation) in self.dated_derivations.iter().enumerate() { - if dated_derivation - .accumulated_intersection - .intersection(start_term) - .subset_of(incompat_term) - { - // We found the derivation causing satisfaction. - return ( - idx, - dated_derivation.global_index, - dated_derivation.decision_level, - ); - } + let idx = self + .dated_derivations + .as_slice() + .partition_point(|dd| dd.accumulated_intersection.intersection(start_term) != empty); + if let Some(dd) = self.dated_derivations.get(idx) { + debug_assert_eq!(dd.accumulated_intersection.intersection(start_term), empty); + return (Some(dd.cause), dd.global_index, dd.decision_level); } // If it wasn't found in the derivations, // it must be the decision which is last (if called in the right context). match &self.assignments_intersection { - AssignmentsIntersection::Decision((global_index, _, _)) => ( - self.dated_derivations.len(), - *global_index, - self.highest_decision_level, - ), + AssignmentsIntersection::Decision((global_index, _, _)) => { + (None, *global_index, self.highest_decision_level) + } AssignmentsIntersection::Derivations(accumulated_intersection) => { unreachable!( concat!( "while processing package {}: ", - "accum_term = {} isn't a subset of incompat_term = {}, ", + "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! ", "(Maybe your Version ordering is broken?)" ), - package, - accumulated_intersection.intersection(start_term), - incompat_term + package, accumulated_intersection, start_term ) } } diff --git a/src/term.rs b/src/term.rs index 3a9906bd..2974da62 100644 --- a/src/term.rs +++ b/src/term.rs @@ -107,6 +107,7 @@ impl Term { /// Indicate if this term is a subset of another term. /// Just like for sets, we say that t1 is a subset of t2 /// if and only if t1 ∩ t2 = t1. + #[cfg(test)] pub(crate) fn subset_of(&self, other: &Self) -> bool { self == &self.intersection(other) }