From 9adc5479098cf019b75ac89e28e3e4b1d34d27e3 Mon Sep 17 00:00:00 2001 From: konstin Date: Mon, 11 Mar 2024 19:21:51 +0000 Subject: [PATCH] Specialize range operations for better performance --- src/internal/partial_solution.rs | 2 +- src/range.rs | 8 ++++++ src/term.rs | 47 +++++++++++++++++++++++++++++--- src/version_set.rs | 10 +++++++ 4 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index d6745993..f517a9e8 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -519,7 +519,7 @@ impl PackageAssignments { let idx = self .dated_derivations .as_slice() - .partition_point(|dd| dd.accumulated_intersection.intersection(start_term) != empty); + .partition_point(|dd| !dd.accumulated_intersection.is_disjoint(start_term)); 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); diff --git a/src/range.rs b/src/range.rs index 3f543467..ee98bb92 100644 --- a/src/range.rs +++ b/src/range.rs @@ -738,6 +738,14 @@ impl VersionSet for Range { fn union(&self, other: &Self) -> Self { Range::union(self, other) } + + fn is_disjoint(&self, other: &Self) -> bool { + Range::is_disjoint(self, other) + } + + fn subset_of(&self, other: &Self) -> bool { + Range::subset_of(self, other) + } } // REPORT ###################################################################### diff --git a/src/term.rs b/src/term.rs index 0043a13e..20e82bb6 100644 --- a/src/term.rs +++ b/src/term.rs @@ -6,7 +6,14 @@ use crate::version_set::VersionSet; use std::fmt::{self, Display}; -/// A positive or negative expression regarding a set of versions. +/// A positive or negative expression regarding a set of versions. +/// +/// If a version is selected then `Positive(r)` and `Negative(r.complement())` are equivalent, but +/// they have different semantics when no version is selected. A `Positive` term in the partial +/// solution requires a version to be selected. But a `Negative` term allows for a solution that +/// does not have that package selected. Specifically, `Positive(VS::empty())` means that there was +/// a conflict, we need to select a version for the package but can't pick any, while +/// `Negative(VS::full())` would mean it is fine as long as we don't select the package. #[derive(Debug, Clone, Eq, PartialEq)] pub enum Term { /// For example, "1.0.0 <= v < 2.0.0" is a positive expression @@ -84,7 +91,8 @@ impl Term { /// Set operations with terms. impl Term { /// Compute the intersection of two terms. - /// If at least one term is positive, the intersection is also positive. + /// + /// The intersection is positive is at least one of the two terms is positive. pub(crate) fn intersection(&self, other: &Self) -> Self { match (self, other) { (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.intersection(r2)), @@ -98,10 +106,34 @@ impl Term { } } + /// Check whether two terms are mutually exclusive. + /// + /// An optimization for the native implementation of checking whether the intersection of two sets is empty. + pub(crate) fn is_disjoint(&self, other: &Self) -> bool { + match (self, other) { + (Self::Positive(r1), Self::Positive(r2)) => r1.is_disjoint(r2), + (Self::Negative(r1), Self::Negative(r2)) => r1 == &VS::empty() && r2 == &VS::empty(), + // If the positive term is a subset of the negative term, it lies fully in the region that the negative + // term excludes. + (Self::Positive(r1), Self::Negative(r2)) => r1.subset_of(r2), + // Inversely, if there is a region outside the negative, they overlap in this region. + (Self::Negative(r1), Self::Positive(r2)) => r2.subset_of(r1), + } + } + /// Compute the union of two terms. /// If at least one term is negative, the union is also negative. pub(crate) fn union(&self, other: &Self) -> Self { - (self.negate().intersection(&other.negate())).negate() + match (self, other) { + (Self::Positive(r1), Self::Positive(r2)) => Self::Positive(r1.union(r2)), + (Self::Positive(r1), Self::Negative(r2)) => { + Self::Negative(r1.union(&r2.complement()).complement()) + } + (Self::Negative(r1), Self::Positive(r2)) => { + Self::Negative(r1.complement().union(r2).complement()) + } + (Self::Negative(r1), Self::Negative(r2)) => Self::Negative(r1.intersection(r2)), + } } /// Indicate if this term is a subset of another term. @@ -200,7 +232,6 @@ pub mod tests { crate::range::tests::strategy().prop_map(Term::Negative), ] } - proptest! { // Testing relation -------------------------------- @@ -217,5 +248,13 @@ pub mod tests { } } + /// Ensure that we don't wrongly convert between positive and negative ranges + #[test] + fn positive_negative(term1 in strategy(), term2 in strategy()) { + let intersection_positive = term1.is_positive() || term2.is_positive(); + let union_positive = term1.is_positive() && term2.is_positive(); + assert_eq!(term1.intersection(&term2).is_positive(), intersection_positive); + assert_eq!(term1.union(&term2).is_positive(), union_positive); + } } } diff --git a/src/version_set.rs b/src/version_set.rs index 501ec700..fae22948 100644 --- a/src/version_set.rs +++ b/src/version_set.rs @@ -57,4 +57,14 @@ pub trait VersionSet: Debug + Display + Clone + Eq { .intersection(&other.complement()) .complement() } + + /// Whether the range have no overlapping segmets + fn is_disjoint(&self, other: &Self) -> bool { + self.intersection(other) == Self::empty() + } + + /// Whether all range of `self` are contained in `other` + fn subset_of(&self, other: &Self) -> bool { + self == &self.intersection(other) + } }