Skip to content

Commit

Permalink
Specialize range operations for better performance
Browse files Browse the repository at this point in the history
  • Loading branch information
konstin committed Mar 6, 2024
1 parent 5dc5312 commit c0e306f
Show file tree
Hide file tree
Showing 4 changed files with 265 additions and 9 deletions.
2 changes: 1 addition & 1 deletion src/internal/partial_solution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ impl<P: Package, VS: VersionSet> PackageAssignments<P, VS> {
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);
Expand Down
215 changes: 211 additions & 4 deletions src/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,8 +328,10 @@ fn within_bounds<V: PartialOrd>(v: &V, segment: &Interval<V>) -> Ordering {
Ordering::Greater
}

/// A valid segment is one where at least one version fits between start and end
fn valid_segment<T: PartialOrd>(start: &Bound<T>, end: &Bound<T>) -> bool {
match (start, end) {
// Singleton interval are allowed
(Included(s), Included(e)) => s <= e,
(Included(s), Excluded(e)) => s < e,
(Excluded(s), Included(e)) => s < e,
Expand All @@ -338,6 +340,47 @@ fn valid_segment<T: PartialOrd>(start: &Bound<T>, end: &Bound<T>) -> bool {
}
}

/// The end of one interval is before the start of the next one, so they can't be concatenated
/// into a single interval. The `union` method calling with both intervals and then the intervals
/// switched. If either is true, the intervals are separate in the union and if both are false, they
/// are merged.
/// ```text
/// True for these two:
/// |----|
/// |-----|
/// ^ end ^ start
/// False for these two:
/// |----|
/// |-----|
/// Here it depends: If they both exclude the position they share, there is a version in between
/// them that blocks concatenation
/// |----|
/// |-----|
/// ```
fn end_before_start_with_gap<V: PartialOrd>(end: &Bound<V>, start: &Bound<V>) -> bool {
match (end, start) {
(_, Unbounded) => false,
(Unbounded, _) => false,
(Included(left), Included(right)) => left < right,
(Included(left), Excluded(right)) => left < right,
(Excluded(left), Included(right)) => left < right,
(Excluded(left), Excluded(right)) => left <= right,
}
}

/// The end of one interval is before the start of the next one, meaning they don't share any
/// version.
fn end_before_start<V: PartialOrd>(end: Bound<&V>, start: Bound<&V>) -> bool {
match (end, start) {
(_, Unbounded) => false,
(Unbounded, _) => false,
(Included(left), Included(right)) => left < right,
(Included(left), Excluded(right)) => left <= right,
(Excluded(left), Included(right)) => left <= right,
(Excluded(left), Excluded(right)) => left <= right,
}
}

/// Group adjacent versions locations.
///
/// ```text
Expand Down Expand Up @@ -371,10 +414,70 @@ fn group_adjacent_locations(
impl<V: Ord + Clone> Range<V> {
/// Computes the union of this `Range` and another.
pub fn union(&self, other: &Self) -> Self {
self.complement()
.intersection(&other.complement())
.complement()
.check_invariants()
let mut output: SmallVec<Interval<V>> = SmallVec::empty();
let mut accumulator: Option<(&Bound<_>, &Bound<_>)> = None;
let mut left_iter = self.segments.iter().peekable();
let mut right_iter = other.segments.iter().peekable();
loop {
let smaller_interval = match (left_iter.peek(), right_iter.peek()) {
(Some((left_start, left_end)), Some((right_start, right_end))) => {
let left_start_is_smaller = match (left_start, right_start) {
(Unbounded, _) => true,
(_, Unbounded) => false,
(Included(l), Included(r)) => l <= r,
(Excluded(l), Excluded(r)) => l <= r,
(Included(l), Excluded(r)) => l <= r,
(Excluded(l), Included(r)) => l < r,
};

if left_start_is_smaller {
left_iter.next();
(left_start, left_end)
} else {
right_iter.next();
(right_start, right_end)
}
}
(Some((left_start, left_end)), None) => {
left_iter.next();
(left_start, left_end)
}
(None, Some((right_start, right_end))) => {
right_iter.next();
(right_start, right_end)
}
(None, None) => break,
};

if let Some(accumulator_) = accumulator {
if end_before_start_with_gap(accumulator_.1, smaller_interval.0) {
output.push((accumulator_.0.clone(), accumulator_.1.clone()));
accumulator = Some(smaller_interval);
} else {
let accumulator_end = match (accumulator_.1, smaller_interval.1) {
(_, Unbounded) | (Unbounded, _) => &Unbounded,
(Included(l), Excluded(r) | Included(r)) if l == r => accumulator_.1,
(Excluded(l) | Included(l), Included(r)) if l == r => smaller_interval.1,
(Included(l) | Excluded(l), Included(r) | Excluded(r)) => {
if l > r {
accumulator_.1
} else {
smaller_interval.1
}
}
};
accumulator = Some((accumulator_.0, accumulator_end));
}
} else {
accumulator = Some(smaller_interval)
}
}

if let Some(accumulator) = accumulator {
output.push((accumulator.0.clone(), accumulator.1.clone()));
}

Self { segments: output }.check_invariants()
}

/// Computes the intersection of two sets of versions.
Expand Down Expand Up @@ -444,6 +547,92 @@ impl<V: Ord + Clone> Range<V> {
Self { segments: output }.check_invariants()
}

/// Return true is there can be no `V` so that `V` is contained in both `self` and `other`.
///
/// Note that we don't know that set of all existing `V`s here, so we only check if the segments
/// are disjoint, not if no version is contained in both.
pub fn is_disjoint(&self, other: &Self) -> bool {
let end_before_start = |end, start| match (end, start) {
(_, Unbounded) => false,
(Unbounded, _) => false,
(Included(left), Included(right)) => left < right,
(Included(left), Excluded(right)) => left <= right,
(Excluded(left), Included(right)) => left <= right,
(Excluded(left), Excluded(right)) => left <= right,
};

// The operation is symmetric
let mut left_iter = self.segments.iter().peekable();
let mut right_iter = other.segments.iter().peekable();

while let (Some(left), Some(right)) = (left_iter.peek(), right_iter.peek()) {
if end_before_start(left.end_bound(), right.start_bound()) {
left_iter.next();
} else if end_before_start(right.end_bound(), left.start_bound()) {
right_iter.next();
} else {
return false;
}
}

// The remaining element(s) can't intersect anymore
true
}

/// Return true if any `V` that is contained in `self` is also contained in `other`.
///
/// Note that we don't know that set of all existing `V`s here, so we only check if all
/// segments `self` are contained in a segment of `other`.
fn subset_of(&self, other: &Self) -> bool {
let mut containing_iter = other.segments.iter();
let mut subset_iter = self.segments.iter();
let Some(mut containing_elem) = containing_iter.next() else {
// As long as we have subset elements, we need containing elements
return subset_iter.next().is_none();
};

for subset_elem in subset_iter {
// Check if the current containing element ends before the subset element.
// There needs to be another containing element for our subset element in this case.
while end_before_start(containing_elem.end_bound(), subset_elem.start_bound()) {
if let Some(containing_elem_) = containing_iter.next() {
containing_elem = containing_elem_;
} else {
return false;
};
}

let start_contained = match (containing_elem.start_bound(), subset_elem.start_bound()) {
(Unbounded, _) => true,
(Included(_) | Excluded(_), Unbounded) => false,
// This is the only case where the subset is bound is "wider" than containing bound ...
(Excluded(left), Included(right)) => left < right,
// ... while in the other cases they can share the point
(Included(left), Included(right)) => left <= right,
(Included(left), Excluded(right)) => left <= right,
(Excluded(left), Excluded(right)) => left <= right,
};

let end_contained = match (subset_elem.end_bound(), containing_elem.end_bound()) {
(_, Unbounded) => true,
(Unbounded, Included(_) | Excluded(_)) => false,
// This is the only case where the subset is bound is "wider" than containing bound ...
(Included(left), Excluded(right)) => left < right,
// ... while in the other cases they can share the point
(Included(left), Included(right)) => left <= right,
(Excluded(left), Included(right)) => left <= right,
(Excluded(left), Excluded(right)) => left <= right,
};

if !(start_contained && end_contained) {
// This subset element is not contained
return false;
}
}

true
}

/// Returns a simpler Range that contains the same versions
///
/// For every one of the Versions provided in versions the existing range and
Expand Down Expand Up @@ -538,6 +727,14 @@ impl<T: Debug + Display + Clone + Eq + Ord> VersionSet for Range<T> {
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 ######################################################################
Expand Down Expand Up @@ -749,6 +946,16 @@ pub mod tests {
assert_eq!(r1.union(&r2).contains(&version), r1.contains(&version) || r2.contains(&version));
}

#[test]
fn union_through_intersection(r1 in strategy(), r2 in strategy()) {
let union_def = r1
.complement()
.intersection(&r2.complement())
.complement()
.check_invariants();
assert_eq!(r1.union(&r2), union_def);
}

// Testing contains --------------------------------

#[test]
Expand Down
47 changes: 43 additions & 4 deletions src/term.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<VS: VersionSet> {
/// For example, "1.0.0 <= v < 2.0.0" is a positive expression
Expand Down Expand Up @@ -84,7 +91,8 @@ impl<VS: VersionSet> Term<VS> {
/// Set operations with terms.
impl<VS: VersionSet> Term<VS> {
/// 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)),
Expand All @@ -98,10 +106,34 @@ impl<VS: VersionSet> Term<VS> {
}
}

/// 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.
Expand Down Expand Up @@ -200,7 +232,6 @@ pub mod tests {
crate::range::tests::strategy().prop_map(Term::Negative),
]
}

proptest! {

// Testing relation --------------------------------
Expand All @@ -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);
}
}
}
10 changes: 10 additions & 0 deletions src/version_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

0 comments on commit c0e306f

Please sign in to comment.