From a632cb40eee6c4cb5a787ee4873e148a3d207ea9 Mon Sep 17 00:00:00 2001 From: Alex Tokarev Date: Sat, 21 Nov 2020 13:50:30 +0300 Subject: [PATCH 01/22] ci: check crate publishing works before release (#69) --- .github/workflows/cargo_publish_dry_run.yml | 40 +++++++++++++++++++++ .github/workflows/ci.yml | 2 +- 2 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/cargo_publish_dry_run.yml diff --git a/.github/workflows/cargo_publish_dry_run.yml b/.github/workflows/cargo_publish_dry_run.yml new file mode 100644 index 00000000..c23c162e --- /dev/null +++ b/.github/workflows/cargo_publish_dry_run.yml @@ -0,0 +1,40 @@ +# Runs `cargo publish --dry-run` before another release + +name: Check crate publishing works +on: + pull_request: + branches: [ release ] + +env: + CARGO_TERM_COLOR: always + +jobs: + cargo_publish_dry_run: + name: Publishing works + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: Install stable Rust + uses: actions-rs/toolchain@v1 + with: + toolchain: stable + profile: minimal + + - name: Get Cargo version + id: cargo_version + run: echo "::set-output name=version::$(cargo -V | tr -d ' ')" + shell: bash + + - name: Download cache + uses: actions/cache@v2 + with: + path: | + ~/.cargo/registry/index/ + ~/.cargo/registry/cache/ + ~/.cargo/git/db/ + target/ + key: ${{ runner.os }}-${{ steps.cargo_version.outputs.version }}-${{ hashFiles('Cargo.toml') }} + restore-keys: ${{ runner.os }}-${{ steps.cargo_version.outputs.version }} + + - name: Run `cargo publish --dry-run` + run: cargo publish --dry-run diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fade925a..bf7df723 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,7 +2,7 @@ name: CI on: pull_request: push: - branches: [ master, dev ] + branches: [ release, dev ] schedule: [ cron: "0 6 * * 4" ] env: From 4b14c6fed3515e7d39e2c208f4ea1dd5a5e28b26 Mon Sep 17 00:00:00 2001 From: Alex Tokarev Date: Sat, 21 Nov 2020 13:57:16 +0300 Subject: [PATCH 02/22] docs: fix repository link (#71) --- Cargo.toml | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 37a4e88b..544ac050 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,11 +3,15 @@ [package] name = "pubgrub" version = "0.2.0" -authors = ["Matthieu Pizenberg ", "Alex Tokarev ", "Jacob Finkelman "] +authors = [ + "Matthieu Pizenberg ", + "Alex Tokarev ", + "Jacob Finkelman ", +] edition = "2018" description = "PubGrub version solving algorithm" readme = "README.md" -repository = "https://github.com/mpizenberg/pubgrub-rs" +repository = "https://github.com/pubgrub-rs/pubgrub" license = "MPL-2.0" keywords = ["dependency", "pubgrub", "semver", "solver", "version"] categories = ["algorithms"] @@ -22,8 +26,8 @@ serde = { version = "1.0", features = ["derive"], optional = true } [dev-dependencies] proptest = "0.10.1" -ron="0.6" -varisat="0.2.2" +ron = "0.6" +varisat = "0.2.2" criterion = "0.3" [[bench]] From 09110976dcb46b25d223668c0d0aa974b0e329e9 Mon Sep 17 00:00:00 2001 From: Alex Tokarev Date: Sat, 21 Nov 2020 20:14:41 +0300 Subject: [PATCH 03/22] docs(changelog): fix the link to v0.1.0..v0.2.0 diff (#70) * docs(changelog): fix the link to v0.1.0..v0.2.0 diff * docs(changelog): put '-diff' after tag * docs(changelog): change diff tag to match the text --- CHANGELOG.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c2dda56..a2cb50a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,9 @@ All notable changes to this project will be documented in this file. -## Unreleased [(diff)][diff-unreleased] +## Unreleased [(diff)][unreleased-diff] -## [0.2.0] - 2020-11-19 - [(diff with 0.1.0)][diff-0.2.0] +## [0.2.0] - 2020-11-19 - [(diff with 0.1.0)][0.1.0-diff] This release brings many important improvements to PubGrub. The gist of it is: @@ -78,7 +78,9 @@ The gist of it is: #### Changed -- CI workflow was improved (`./github/workflows/`), including a check for [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) and [Clippy ](https://github.com/rust-lang/rust-clippy) for source code linting. +- CI workflow was improved (`./github/workflows/`), including a check for + [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) and + [Clippy](https://github.com/rust-lang/rust-clippy) for source code linting. - Using SPDX license identifiers instead of MPL-2.0 classic file headers. - `State.incompatibilities` is now wrapped inside a `Rc`. - `DecisionLevel(u32)` is used in place of `usize` for partial solution decision levels. @@ -132,6 +134,6 @@ The gist of it is: - `.github/workflows/` CI to automatically build, test and document on push and pull requests. [0.2.0]: https://github.com/pubgrub-rs/pubgrub/releases/tag/v0.2.0 +[0.1.0-diff]: https://github.com/pubgrub-rs/pubgrub/compare/v0.1.0...v0.2.0 [0.1.0]: https://github.com/pubgrub-rs/pubgrub/releases/tag/v0.1.0 -[diff-unreleased]: https://github.com/pubgrub-rs/pubgrub/compare/release...dev -[diff-0.2.0]: https://github.com/mpizenberg/elm-pointer-events/compare/v0.1.0...v0.2.0 +[unreleased-diff]: https://github.com/pubgrub-rs/pubgrub/compare/release...dev From 4ad4c940e291ed69820a1a6a05546903ec614d27 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Sat, 16 Jan 2021 06:22:38 -0500 Subject: [PATCH 04/22] perf: make our own smallvec with no unsafe (#72) Great perf improvements! --- src/internal/mod.rs | 1 + src/internal/small_vec.rs | 84 +++++++++++++++++++++++++++++++++++++++ src/range.rs | 64 ++++++++++++++--------------- 3 files changed, 118 insertions(+), 31 deletions(-) create mode 100644 src/internal/small_vec.rs diff --git a/src/internal/mod.rs b/src/internal/mod.rs index 2c5b9d3e..fe5ba236 100644 --- a/src/internal/mod.rs +++ b/src/internal/mod.rs @@ -7,3 +7,4 @@ pub mod core; pub mod incompatibility; pub mod memory; pub mod partial_solution; +pub mod small_vec; diff --git a/src/internal/small_vec.rs b/src/internal/small_vec.rs new file mode 100644 index 00000000..98c0dd1e --- /dev/null +++ b/src/internal/small_vec.rs @@ -0,0 +1,84 @@ +use std::fmt; + +#[derive(Clone)] +pub enum SmallVec { + Empty, + One([T; 1]), + Two([T; 2]), + Flexible(Vec), +} + +impl SmallVec { + pub fn empty() -> Self { + Self::Empty + } + + pub fn one(t: T) -> Self { + Self::One([t]) + } + + pub fn as_slice(&self) -> &[T] { + match self { + Self::Empty => &[], + Self::One(v) => v, + Self::Two(v) => v, + Self::Flexible(v) => v, + } + } + + pub fn push(&mut self, new: T) { + *self = match std::mem::take(self) { + Self::Empty => Self::One([new]), + Self::One([v1]) => Self::Two([v1, new]), + Self::Two([v1, v2]) => Self::Flexible(vec![v1, v2, new]), + Self::Flexible(mut v) => { + v.push(new); + Self::Flexible(v) + } + } + } + + pub fn iter(&self) -> impl Iterator { + self.as_slice().iter() + } +} + +impl Default for SmallVec { + fn default() -> Self { + Self::Empty + } +} + +impl Eq for SmallVec {} + +impl PartialEq> for SmallVec { + fn eq(&self, other: &SmallVec) -> bool { + self.as_slice() == other.as_slice() + } +} + +impl fmt::Debug for SmallVec { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.as_slice().fmt(f) + } +} + +#[cfg(feature = "serde")] +impl serde::Serialize for SmallVec { + fn serialize(&self, s: S) -> Result { + serde::Serialize::serialize(self.as_slice(), s) + } +} + +#[cfg(feature = "serde")] +impl<'de, T: serde::Deserialize<'de>> serde::Deserialize<'de> for SmallVec { + fn deserialize>(d: D) -> Result { + let items: Vec = serde::Deserialize::deserialize(d)?; + + let mut v = Self::empty(); + for item in items { + v.push(item); + } + Ok(v) + } +} diff --git a/src/range.rs b/src/range.rs index 25d9403c..cc4ea432 100644 --- a/src/range.rs +++ b/src/range.rs @@ -17,6 +17,7 @@ use std::cmp::Ordering; use std::fmt; +use crate::internal::small_vec::SmallVec; use crate::version::Version; /// A Range is a set of versions. @@ -24,7 +25,7 @@ use crate::version::Version; #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "serde", serde(transparent))] pub struct Range { - segments: Vec>, + segments: SmallVec>, } type Interval = (V, Option); @@ -34,7 +35,7 @@ impl Range { /// Empty set of versions. pub fn none() -> Self { Self { - segments: Vec::new(), + segments: SmallVec::empty(), } } @@ -47,14 +48,14 @@ impl Range { pub fn exact(v: impl Into) -> Self { let v = v.into(); Self { - segments: vec![(v.clone(), Some(v.bump()))], + segments: SmallVec::one((v.clone(), Some(v.bump()))), } } /// Set of all versions higher or equal to some version. pub fn higher_than(v: impl Into) -> Self { Self { - segments: vec![(v.into(), None)], + segments: SmallVec::one((v.into(), None)), } } @@ -65,7 +66,7 @@ impl Range { Self::none() } else { Self { - segments: vec![(V::lowest(), Some(v))], + segments: SmallVec::one((V::lowest(), Some(v))), } } } @@ -78,7 +79,7 @@ impl Range { let v2 = v2.into(); if v1 < v2 { Self { - segments: vec![(v1, Some(v2))], + segments: SmallVec::one((v1, Some(v2))), } } else { Self::none() @@ -109,13 +110,9 @@ impl Range { // First high bound is not +∞ Some((v1, Some(v2))) => { if v1 == &V::lowest() { - Self { - segments: Self::negate_segments(v2.clone(), &self.segments[1..]), - } + Self::negate_segments(v2.clone(), &self.segments.as_slice()[1..]) } else { - Self { - segments: Self::negate_segments(V::lowest(), &self.segments), - } + Self::negate_segments(V::lowest(), &self.segments.as_slice()) } } } @@ -125,8 +122,8 @@ impl Range { /// For example: /// [ (v1, None) ] => [ (start, Some(v1)) ] /// [ (v1, Some(v2)) ] => [ (start, Some(v1)), (v2, None) ] - fn negate_segments(start: V, segments: &[Interval]) -> Vec> { - let mut complement_segments = Vec::with_capacity(1 + segments.len()); + fn negate_segments(start: V, segments: &[Interval]) -> Range { + let mut complement_segments = SmallVec::empty(); let mut start = Some(start); for (v1, some_v2) in segments.iter() { // start.unwrap() is fine because `segments` is not exposed, @@ -137,7 +134,10 @@ impl Range { if let Some(last) = start { complement_segments.push((last, None)); } - complement_segments + + Self { + segments: complement_segments, + } } // Union and intersection ################################################## @@ -149,16 +149,9 @@ impl Range { /// Compute the intersection of two sets of versions. pub fn intersection(&self, other: &Self) -> Self { - Self { - segments: Self::intersection_segments(&self.segments, &other.segments), - } - } - - /// Helper function performing intersection of two interval segments. - fn intersection_segments(s1: &[Interval], s2: &[Interval]) -> Vec> { - let mut segments = Vec::with_capacity(s1.len().min(s2.len())); - let mut left_iter = s1.iter(); - let mut right_iter = s2.iter(); + let mut segments = SmallVec::empty(); + let mut left_iter = self.segments.iter(); + let mut right_iter = other.segments.iter(); let mut left = left_iter.next(); let mut right = right_iter.next(); loop { @@ -190,13 +183,17 @@ impl Range { left = left_iter.next(); } Ordering::Equal => { - segments.extend(left_iter.cloned()); + for l in left_iter.cloned() { + segments.push(l) + } break; } Ordering::Greater => { let start = l1.max(r1).to_owned(); segments.push((start, Some(l2.to_owned()))); - segments.extend(left_iter.cloned()); + for l in left_iter.cloned() { + segments.push(l) + } break; } }, @@ -207,13 +204,17 @@ impl Range { right = right_iter.next(); } Ordering::Equal => { - segments.extend(right_iter.cloned()); + for r in right_iter.cloned() { + segments.push(r) + } break; } Ordering::Greater => { let start = l1.max(r1).to_owned(); segments.push((start, Some(r2.to_owned()))); - segments.extend(right_iter.cloned()); + for r in right_iter.cloned() { + segments.push(r) + } break; } }, @@ -231,7 +232,8 @@ impl Range { } } } - segments + + Self { segments } } } @@ -308,7 +310,7 @@ pub mod tests { vec.sort_unstable(); vec.dedup(); let mut pair_iter = vec.chunks_exact(2); - let mut segments = Vec::with_capacity(vec.len() / 2 + 1); + let mut segments = SmallVec::empty(); while let Some([v1, v2]) = pair_iter.next() { segments.push((NumberVersion(*v1), Some(NumberVersion(*v2)))); } From 6bf945683a277e6fe36d07775a4bbcaa6fc6b806 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Sat, 23 Jan 2021 13:11:40 -0500 Subject: [PATCH 05/22] perf: make our own smallmap with no unsafe (#73) --- src/internal/incompatibility.rs | 120 +++++++------------- src/internal/mod.rs | 1 + src/internal/small_map.rs | 195 ++++++++++++++++++++++++++++++++ 3 files changed, 234 insertions(+), 82 deletions(-) create mode 100644 src/internal/small_map.rs diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 219c6ba7..7fc52e45 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -6,12 +6,12 @@ use std::collections::HashSet as Set; use std::fmt; +use crate::internal::small_map::SmallMap; use crate::package::Package; use crate::range::Range; use crate::report::{DefaultStringReporter, DerivationTree, Derived, External}; use crate::solver::DependencyConstraints; use crate::term::{self, Term}; -use crate::type_aliases::Map; use crate::version::Version; /// An incompatibility is a set of terms for different packages @@ -33,7 +33,7 @@ use crate::version::Version; pub struct Incompatibility { /// TODO: remove pub. pub id: usize, - package_terms: Map>, + package_terms: SmallMap>, kind: Kind, } @@ -74,14 +74,12 @@ pub enum Relation { impl Incompatibility { /// Create the initial "not Root" incompatibility. pub fn not_root(id: usize, package: P, version: V) -> Self { - let mut package_terms = Map::with_capacity_and_hasher(1, Default::default()); - package_terms.insert( - package.clone(), - Term::Negative(Range::exact(version.clone())), - ); Self { id, - package_terms, + package_terms: SmallMap::One([( + package.clone(), + Term::Negative(Range::exact(version.clone())), + )]), kind: Kind::NotRoot(package, version), } } @@ -93,11 +91,9 @@ impl Incompatibility { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), }; - let mut package_terms = Map::with_capacity_and_hasher(1, Default::default()); - package_terms.insert(package.clone(), term); Self { id, - package_terms, + package_terms: SmallMap::One([(package.clone(), term)]), kind: Kind::NoVersions(package, range), } } @@ -107,11 +103,9 @@ impl Incompatibility { /// because its list of dependencies is unavailable. pub fn unavailable_dependencies(id: usize, package: P, version: V) -> Self { let range = Range::exact(version); - let mut package_terms = Map::with_capacity_and_hasher(1, Default::default()); - package_terms.insert(package.clone(), Term::Positive(range.clone())); Self { id, - package_terms, + package_terms: SmallMap::One([(package.clone(), Term::Positive(range.clone()))]), kind: Kind::UnavailableDependencies(package, range), } } @@ -133,54 +127,18 @@ impl Incompatibility { /// Build an incompatibility from a given dependency. fn from_dependency(id: usize, package: P, version: V, dep: (&P, &Range)) -> Self { - let mut package_terms = Map::with_capacity_and_hasher(2, Default::default()); let range1 = Range::exact(version); - package_terms.insert(package.clone(), Term::Positive(range1.clone())); let (p2, range2) = dep; - package_terms.insert(p2.clone(), Term::Negative(range2.clone())); Self { id, - package_terms, + package_terms: SmallMap::Two([ + (package.clone(), Term::Positive(range1.clone())), + (p2.clone(), Term::Negative(range2.clone())), + ]), kind: Kind::FromDependencyOf(package, range1, p2.clone(), range2.clone()), } } - /// Perform the intersection of terms in two incompatibilities. - fn intersection(i1: &Map>, i2: &Map>) -> Map> { - Self::merge(i1, i2, |t1, t2| Some(t1.intersection(t2))) - } - - /// Merge two hash maps. - /// - /// When a key is common to both, - /// apply the provided function to both values. - /// If the result is None, remove that key from the merged map, - /// otherwise add the content of the Some(_). - fn merge Option>( - map_1: &Map, - map_2: &Map, - f: F, - ) -> Map { - let mut merged_map = map_1.clone(); - merged_map.reserve(map_2.len()); - let mut to_delete = Vec::new(); - for (key, val_2) in map_2.iter() { - match merged_map.get_mut(key) { - None => { - merged_map.insert(key.clone(), val_2.clone()); - } - Some(val_1) => match f(val_1, val_2) { - None => to_delete.push(key), - Some(merged_value) => *val_1 = merged_value, - }, - } - } - for key in to_delete.iter() { - merged_map.remove(key); - } - merged_map - } - /// Add this incompatibility into the set of all incompatibilities. /// /// Pub collapses identical dependencies from adjacent package versions @@ -210,12 +168,16 @@ impl Incompatibility { /// Prior cause of two incompatibilities using the rule of resolution. pub fn prior_cause(id: usize, incompat: &Self, satisfier_cause: &Self, package: &P) -> Self { let kind = Kind::DerivedFrom(incompat.id, satisfier_cause.id); - let mut incompat1 = incompat.package_terms.clone(); - let mut incompat2 = satisfier_cause.package_terms.clone(); - let t1 = incompat1.remove(package).unwrap(); - let t2 = incompat2.remove(package).unwrap(); - let mut package_terms = Self::intersection(&incompat1, &incompat2); - let term = t1.union(&t2); + let mut package_terms = incompat.package_terms.clone(); + let t1 = package_terms.remove(package).unwrap(); + package_terms.merge( + satisfier_cause + .package_terms + .iter() + .filter(|(p, _)| p != &package), + |t1, t2| Some(t1.intersection(t2)), + ); + let term = t1.union(satisfier_cause.package_terms.get(package).unwrap()); if term != Term::any() { package_terms.insert(package.clone(), term); } @@ -255,7 +217,7 @@ impl Incompatibility { /// Check if an incompatibility should mark the end of the algorithm /// because it satisfies the root package. pub fn is_terminal(&self, root_package: &P, root_version: &V) -> bool { - if self.package_terms.is_empty() { + if self.package_terms.len() == 0 { true } else if self.package_terms.len() > 1 { false @@ -296,7 +258,7 @@ impl Incompatibility { let cause1 = store[*id1].build_derivation_tree(shared_ids, store); let cause2 = store[*id2].build_derivation_tree(shared_ids, store); let derived = Derived { - terms: self.package_terms.clone(), + terms: self.package_terms.as_map(), shared_id: shared_ids.get(&self.id).cloned(), cause1: Box::new(cause1), cause2: Box::new(cause2), @@ -329,26 +291,18 @@ impl fmt::Display for Incompatibility { write!( f, "{}", - DefaultStringReporter::string_terms(&self.package_terms) + DefaultStringReporter::string_terms(&self.package_terms.as_map()) ) } } -impl IntoIterator for Incompatibility { - type Item = (P, Term); - type IntoIter = std::collections::hash_map::IntoIter>; - - fn into_iter(self) -> Self::IntoIter { - self.package_terms.into_iter() - } -} - // TESTS ####################################################################### #[cfg(test)] pub mod tests { use super::*; use crate::term::tests::strategy as term_strat; + use crate::type_aliases::Map; use proptest::prelude::*; proptest! { @@ -362,22 +316,24 @@ pub mod tests { /// { p1: t1, p3: t3 } #[test] fn rule_of_resolution(t1 in term_strat(), t2 in term_strat(), t3 in term_strat()) { - let mut i1 = Map::default(); - i1.insert("p1", t1.clone()); - i1.insert("p2", t2.negate()); - let i1 = Incompatibility { id: 0, package_terms: i1, kind: Kind::DerivedFrom(0,0) }; - - let mut i2 = Map::default(); - i2.insert("p2", t2.clone()); - i2.insert("p3", t3.clone()); - let i2 = Incompatibility { id: 0, package_terms: i2, kind: Kind::DerivedFrom(0,0) }; + let i1 = Incompatibility { + id: 0, + package_terms: SmallMap::Two([("p1", t1.clone()), ("p2", t2.negate())]), + kind: Kind::DerivedFrom(0,0) + }; + + let i2 = Incompatibility { + id: 0, + package_terms: SmallMap::Two([("p2", t2.clone()), ("p3", t3.clone())]), + kind: Kind::DerivedFrom(0,0) + }; let mut i3 = Map::default(); i3.insert("p1", t1); i3.insert("p3", t3); let i_resolution = Incompatibility::prior_cause(0, &i1, &i2, &"p2"); - assert_eq!(i_resolution.package_terms, i3); + assert_eq!(i_resolution.package_terms.as_map(), i3); } } diff --git a/src/internal/mod.rs b/src/internal/mod.rs index fe5ba236..56e6baeb 100644 --- a/src/internal/mod.rs +++ b/src/internal/mod.rs @@ -7,4 +7,5 @@ pub mod core; pub mod incompatibility; pub mod memory; pub mod partial_solution; +pub mod small_map; pub mod small_vec; diff --git a/src/internal/small_map.rs b/src/internal/small_map.rs new file mode 100644 index 00000000..a1fe5f9e --- /dev/null +++ b/src/internal/small_map.rs @@ -0,0 +1,195 @@ +use crate::type_aliases::Map; +use std::hash::Hash; + +#[derive(Debug, Clone)] +pub enum SmallMap { + Empty, + One([(K, V); 1]), + Two([(K, V); 2]), + Flexible(Map), +} + +impl SmallMap { + pub fn get(&self, key: &K) -> Option<&V> { + match self { + Self::Empty => None, + Self::One([(k, v)]) if k == key => Some(v), + Self::One(_) => None, + Self::Two([(k1, v1), _]) if key == k1 => Some(v1), + Self::Two([_, (k2, v2)]) if key == k2 => Some(v2), + Self::Two(_) => None, + Self::Flexible(data) => data.get(key), + } + } + + pub fn get_mut(&mut self, key: &K) -> Option<&mut V> { + match self { + Self::Empty => None, + Self::One([(k, v)]) if k == key => Some(v), + Self::One(_) => None, + Self::Two([(k1, v1), _]) if key == k1 => Some(v1), + Self::Two([_, (k2, v2)]) if key == k2 => Some(v2), + Self::Two(_) => None, + Self::Flexible(data) => data.get_mut(key), + } + } + + pub fn remove(&mut self, key: &K) -> Option { + let out; + *self = match std::mem::take(self) { + Self::Empty => { + out = None; + Self::Empty + } + Self::One([(k, v)]) => { + if key == &k { + out = Some(v); + Self::Empty + } else { + out = None; + Self::One([(k, v)]) + } + } + Self::Two([(k1, v1), (k2, v2)]) => { + if key == &k1 { + out = Some(v1); + Self::One([(k2, v2)]) + } else if key == &k2 { + out = Some(v2); + Self::One([(k1, v1)]) + } else { + out = None; + Self::Two([(k1, v1), (k2, v2)]) + } + } + Self::Flexible(mut data) => { + out = data.remove(key); + Self::Flexible(data) + } + }; + out + } + + pub fn insert(&mut self, key: K, value: V) { + *self = match std::mem::take(self) { + Self::Empty => Self::One([(key, value)]), + Self::One([(k, v)]) => { + if key == k { + Self::One([(k, value)]) + } else { + Self::Two([(k, v), (key, value)]) + } + } + Self::Two([(k1, v1), (k2, v2)]) => { + if key == k1 { + Self::Two([(k1, value), (k2, v2)]) + } else if key == k2 { + Self::Two([(k1, v1), (k2, value)]) + } else { + let mut data: Map = Map::with_capacity_and_hasher(3, Default::default()); + data.insert(key, value); + data.insert(k1, v1); + data.insert(k2, v2); + Self::Flexible(data) + } + } + Self::Flexible(mut data) => { + data.insert(key, value); + Self::Flexible(data) + } + }; + } +} + +impl SmallMap { + /// Merge two hash maps. + /// + /// When a key is common to both, + /// apply the provided function to both values. + /// If the result is None, remove that key from the merged map, + /// otherwise add the content of the Some(_). + pub fn merge<'a>( + &'a mut self, + map_2: impl Iterator, + f: impl Fn(&V, &V) -> Option, + ) { + for (key, val_2) in map_2 { + match self.get_mut(key) { + None => { + self.insert(key.clone(), val_2.clone()); + } + Some(val_1) => match f(val_1, val_2) { + None => { + self.remove(key); + } + Some(merged_value) => *val_1 = merged_value, + }, + } + } + } +} + +impl Default for SmallMap { + fn default() -> Self { + Self::Empty + } +} + +impl SmallMap { + pub fn len(&self) -> usize { + match self { + Self::Empty => 0, + Self::One(_) => 1, + Self::Two(_) => 2, + Self::Flexible(data) => data.len(), + } + } +} + +impl SmallMap { + pub 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>), +} + +impl<'a, K: 'a, V: 'a> Iterator for IterSmallMap<'a, K, V> { + type Item = (&'a K, &'a V); + + fn next(&mut self) -> Option { + match self { + IterSmallMap::Inline(inner) => inner.next().map(|(k, v)| (k, v)), + IterSmallMap::Map(inner) => inner.next(), + } + } +} + +impl SmallMap { + pub fn iter(&self) -> impl Iterator { + match self { + Self::Empty => IterSmallMap::Inline([].iter()), + Self::One(data) => IterSmallMap::Inline(data.iter()), + Self::Two(data) => IterSmallMap::Inline(data.iter()), + Self::Flexible(data) => IterSmallMap::Map(data.iter()), + } + } +} From f8432fd84a67bc901f96939cad85ea4e3a306517 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 3 Feb 2021 11:54:46 -0500 Subject: [PATCH 06/22] test: Str benchmarks (#75) * test: benchmark files that are str and SemanticVersion * test: check that benchmarks are working * test: use the elm benchmark --- benches/large_case.rs | 46 ++++++++++++++++++++++++++++--------------- tests/proptest.rs | 40 +++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 16 deletions(-) diff --git a/benches/large_case.rs b/benches/large_case.rs index ad9f486d..476228c8 100644 --- a/benches/large_case.rs +++ b/benches/large_case.rs @@ -4,8 +4,26 @@ use std::time::Duration; extern crate criterion; use self::criterion::*; +use pubgrub::package::Package; use pubgrub::solver::{resolve, OfflineDependencyProvider}; -use pubgrub::version::NumberVersion; +use pubgrub::version::{NumberVersion, SemanticVersion, Version}; +use serde::de::Deserialize; +use std::hash::Hash; + +fn bench<'a, P: Package + Deserialize<'a>, V: Version + Hash + Deserialize<'a>>( + b: &mut Bencher, + case: &'a str, +) { + let dependency_provider: OfflineDependencyProvider = ron::de::from_str(&case).unwrap(); + + b.iter(|| { + for p in dependency_provider.packages() { + for n in dependency_provider.versions(p).unwrap() { + let _ = resolve(&dependency_provider, p.clone(), n.clone()); + } + } + }); +} fn bench_nested(c: &mut Criterion) { let mut group = c.benchmark_group("large_cases"); @@ -13,21 +31,17 @@ fn bench_nested(c: &mut Criterion) { for case in std::fs::read_dir("test-examples").unwrap() { let case = case.unwrap().path(); - - group.bench_function( - format!("{}", case.file_name().unwrap().to_string_lossy()), - |b| { - let s = std::fs::read_to_string(&case).unwrap(); - let dependency_provider: OfflineDependencyProvider = - ron::de::from_str(&s).unwrap(); - - b.iter(|| { - for &n in dependency_provider.versions(&0).unwrap() { - let _ = resolve(&dependency_provider, 0, n); - } - }); - }, - ); + let name = case.file_name().unwrap().to_string_lossy(); + let data = std::fs::read_to_string(&case).unwrap(); + if name.ends_with("u16_NumberVersion.ron") { + group.bench_function(name, |b| { + bench::(b, &data); + }); + } else if name.ends_with("str_SemanticVersion.ron") { + group.bench_function(name, |b| { + bench::<&str, SemanticVersion>(b, &data); + }); + } } group.finish(); diff --git a/tests/proptest.rs b/tests/proptest.rs index 7aa94a29..d2da3730 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -478,3 +478,43 @@ proptest! { } } } + +#[cfg(feature = "serde")] +#[test] +fn large_case() { + for case in std::fs::read_dir("test-examples").unwrap() { + let case = case.unwrap().path(); + let name = case.file_name().unwrap().to_string_lossy(); + eprintln!("{}", name); + let data = std::fs::read_to_string(&case).unwrap(); + if name.ends_with("u16_NumberVersion.ron") { + let dependency_provider: OfflineDependencyProvider = + ron::de::from_str(&data).unwrap(); + let mut sat = SatResolve::new(&dependency_provider); + for p in dependency_provider.packages() { + for n in dependency_provider.versions(p).unwrap() { + if let Ok(s) = resolve(&dependency_provider, p.clone(), n.clone()) { + assert!(sat.sat_is_valid_solution(&s)); + } else { + assert!(!sat.sat_resolve(p, &n)); + } + } + } + } else if name.ends_with("str_SemanticVersion.ron") { + let dependency_provider: OfflineDependencyProvider< + &str, + pubgrub::version::SemanticVersion, + > = ron::de::from_str(&data).unwrap(); + let mut sat = SatResolve::new(&dependency_provider); + for p in dependency_provider.packages() { + for n in dependency_provider.versions(p).unwrap() { + if let Ok(s) = resolve(&dependency_provider, p.clone(), n.clone()) { + assert!(sat.sat_is_valid_solution(&s)); + } else { + assert!(!sat.sat_resolve(p, &n)); + } + } + } + } + } +} From 4470dcbc3d3788a458e6898aa83978734fe67aad Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Sun, 14 Feb 2021 15:16:57 -0500 Subject: [PATCH 07/22] perf: reuse the same buffer in unit_propagation (#74) --- src/internal/core.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index e67f3792..d961343d 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -32,6 +32,11 @@ pub struct State { /// NOT the position in the [incompatibilities](State::incompatibilities) vec. /// TODO: remove pub. pub incompatibility_store: Vec>, + + /// 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: Vec

, } impl State { @@ -45,6 +50,7 @@ impl State { incompatibilities: Rc::new(vec![not_root_incompat.clone()]), partial_solution: PartialSolution::empty(), incompatibility_store: vec![not_root_incompat], + unit_propagation_buffer: vec![], } } @@ -63,9 +69,9 @@ impl State { /// Unit propagation is the core mechanism of the solving algorithm. /// CF pub fn unit_propagation(&mut self, package: P) -> Result<(), PubGrubError> { - let mut current_package = package.clone(); - let mut changed = vec![package]; - loop { + self.unit_propagation_buffer.clear(); + self.unit_propagation_buffer.push(package); + while let Some(current_package) = self.unit_propagation_buffer.pop() { // Iterate over incompatibilities in reverse order // to evaluate first the newest incompatibilities. for incompat in Rc::clone(&self.incompatibilities).iter().rev() { @@ -78,13 +84,14 @@ impl State { // we must perform conflict resolution. Relation::Satisfied => { let (package_almost, root_cause) = self.conflict_resolution(&incompat)?; - changed = vec![package_almost.clone()]; + self.unit_propagation_buffer.clear(); + self.unit_propagation_buffer.push(package_almost.clone()); // Add to the partial solution with incompat as cause. self.partial_solution .add_derivation(package_almost, root_cause); } Relation::AlmostSatisfied(package_almost) => { - changed.push(package_almost.clone()); + self.unit_propagation_buffer.push(package_almost.clone()); // Add (not term) to the partial solution with incompat as cause. self.partial_solution .add_derivation(package_almost, incompat.clone()); @@ -92,12 +99,8 @@ impl State { _ => {} } } - // If there are no more changed packages, unit propagation is done. - match changed.pop() { - None => break, - Some(current) => current_package = current, - } } + // If there are no more changed packages, unit propagation is done. Ok(()) } From 9c7d0fbcb91433d46fd9e7cbc12e4a64bae585e9 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 22 Jan 2021 16:49:53 -0500 Subject: [PATCH 08/22] refactor: do less work in backtrack --- src/internal/memory.rs | 5 +++++ src/internal/partial_solution.rs | 32 +++++++++++++++++--------------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/internal/memory.rs b/src/internal/memory.rs index 090e51e9..9773c5c6 100644 --- a/src/internal/memory.rs +++ b/src/internal/memory.rs @@ -38,6 +38,11 @@ impl Memory { } } + /// Clears the memory. + pub fn clear(&mut self) { + self.assignments.clear(); + } + /// Retrieve intersection of terms in memory related to package. pub fn term_intersection_for_package(&mut self, package: &P) -> Option<&Term> { self.assignments diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 5ede989f..23de5059 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -100,24 +100,26 @@ impl PartialSolution { /// Backtrack the partial solution to a given decision level. pub fn backtrack(&mut self, decision_level: DecisionLevel) { - // TODO: improve with dichotomic search. let pos = self .history - .iter() - .rposition(|dated_assignment| dated_assignment.decision_level == decision_level) - .unwrap_or(self.history.len() - 1); - *self = Self::from_assignments( - std::mem::take(&mut self.history) - .into_iter() - .take(pos + 1) - .map(|dated_assignment| dated_assignment.assignment), - ); - } + .binary_search_by(|probe| { + probe + .decision_level + .cmp(&decision_level) + // `binary_search_by` does not guarantee which element to return when more + // then one match. By all ways claiming that it does not match we ensure we + // get the last one. + .then(std::cmp::Ordering::Less) + }) + .unwrap_or_else(|x| x); - fn from_assignments(assignments: impl Iterator>) -> Self { - let mut partial_solution = Self::empty(); - assignments.for_each(|a| partial_solution.add_assignment(a)); - partial_solution + self.history.truncate(pos); + self.decision_level = self.history.last().expect("no history left").decision_level; + self.memory.clear(); + let mem = &mut self.memory; + self.history + .iter() + .for_each(|da| mem.add_assignment(&da.assignment)); } /// Extract potential packages for the next iteration of unit propagation. From 47aee9a96f751f7d2a5b393bc6ffeab3c07d8bc6 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 1 Mar 2021 13:43:52 -0500 Subject: [PATCH 09/22] refactor: use arena to have type safe ids (#77) * refactor: use id-arena to have type safe ids * refactor: make our own arena with no unsafe for smaller ids * docs: explain manual Clone impl for arena Id * docs: short explanation of what an arena is * docs: for IncompId * refactor: extract duplicate satisfier_cause_terms * refactor: tiny factorization in unit_propagation * refactor: add _id to current_incompat in conflict_resolution * docs: fix http link in arena module * refactor: fix typo statisfier -> satisfier * refactor: don't allocate a vec in from_dependencies * refactor: introduce alloc_iter in arena Co-authored-by: Matthieu Pizenberg --- src/internal/arena.rs | 122 +++++++++++++++++++++++++++ src/internal/assignment.rs | 8 +- src/internal/core.rs | 138 +++++++++++++++++++------------ src/internal/incompatibility.rs | 97 +++++++++------------- src/internal/memory.rs | 15 +++- src/internal/mod.rs | 1 + src/internal/partial_solution.rs | 59 +++++++++---- src/solver.rs | 32 +++---- 8 files changed, 323 insertions(+), 149 deletions(-) create mode 100644 src/internal/arena.rs diff --git a/src/internal/arena.rs b/src/internal/arena.rs new file mode 100644 index 00000000..75d04c82 --- /dev/null +++ b/src/internal/arena.rs @@ -0,0 +1,122 @@ +use std::{ + fmt, + hash::{Hash, Hasher}, + marker::PhantomData, + ops::{Index, Range}, +}; + +/// The index of a value allocated in an arena that holds `T`s. +/// +/// The Clone, Copy and other traits are defined manually because +/// deriving them adds some additional constraints on the `T` generic type +/// that we actually don't need since it is phantom. +/// +/// +pub struct Id { + raw: u32, + _ty: PhantomData T>, +} + +impl Clone for Id { + fn clone(&self) -> Self { + *self + } +} + +impl Copy for Id {} + +impl PartialEq for Id { + fn eq(&self, other: &Id) -> bool { + self.raw == other.raw + } +} + +impl Eq for Id {} + +impl Hash for Id { + fn hash(&self, state: &mut H) { + self.raw.hash(state) + } +} + +impl fmt::Debug for Id { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut type_name = std::any::type_name::(); + if let Some(id) = type_name.rfind(':') { + type_name = &type_name[id + 1..] + } + write!(f, "Id::<{}>({})", type_name, self.raw) + } +} + +impl Id { + pub fn into_raw(self) -> usize { + self.raw as usize + } + fn from(n: u32) -> Self { + Self { + raw: n as u32, + _ty: PhantomData, + } + } + pub fn range_to_iter(range: Range) -> impl Iterator { + let start = range.start.raw; + let end = range.end.raw; + (start..end).map(Self::from) + } +} + +/// Yet another index-based arena. +/// +/// 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. +/// They are all dropped at once when the arena is dropped. +#[derive(Clone, PartialEq, Eq)] +pub struct Arena { + data: Vec, +} + +impl fmt::Debug for Arena { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + fmt.debug_struct("Arena") + .field("len", &self.data.len()) + .field("data", &self.data) + .finish() + } +} + +impl Arena { + pub fn new() -> Arena { + Arena { data: Vec::new() } + } + + pub fn alloc(&mut self, value: T) -> Id { + let raw = self.data.len(); + self.data.push(value); + Id::from(raw as u32) + } + + pub fn alloc_iter>(&mut self, values: I) -> Range> { + let start = Id::from(self.data.len() as u32); + values.for_each(|v| { + self.alloc(v); + }); + let end = Id::from(self.data.len() as u32); + Range { start, end } + } +} + +impl Index> for Arena { + type Output = T; + fn index(&self, id: Id) -> &T { + &self.data[id.raw as usize] + } +} + +impl Index>> for Arena { + type Output = [T]; + fn index(&self, id: Range>) -> &[T] { + &self.data[(id.start.raw as usize)..(id.end.raw as usize)] + } +} diff --git a/src/internal/assignment.rs b/src/internal/assignment.rs index ac8f1270..c811782f 100644 --- a/src/internal/assignment.rs +++ b/src/internal/assignment.rs @@ -3,6 +3,8 @@ //! Assignments are the building blocks of a PubGrub partial solution. //! (partial solution = the current state of the solution we are building in the algorithm). +use crate::internal::arena::Arena; +use crate::internal::incompatibility::IncompId; use crate::internal::incompatibility::Incompatibility; use crate::package::Package; use crate::term::Term; @@ -25,7 +27,7 @@ pub enum Assignment { /// The package corresponding to the derivation. package: P, /// Incompatibility cause of the derivation. - cause: Incompatibility, + cause: IncompId, }, } @@ -41,10 +43,10 @@ impl Assignment { /// Retrieve the current assignment as a [Term]. /// If this is decision, it returns a positive term with that exact version. /// Otherwise, if this is a derivation, just returns its term. - pub fn as_term(&self) -> Term { + pub fn as_term(&self, store: &Arena>) -> Term { match &self { Self::Decision { version, .. } => Term::exact(version.clone()), - Self::Derivation { package, cause } => cause.get(&package).unwrap().negate(), + Self::Derivation { package, cause } => store[*cause].get(&package).unwrap().negate(), } } } diff --git a/src/internal/core.rs b/src/internal/core.rs index d961343d..4e852d4e 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -6,11 +6,14 @@ use std::{collections::HashSet as Set, rc::Rc}; use crate::error::PubGrubError; +use crate::internal::arena::Arena; use crate::internal::assignment::Assignment::{Decision, Derivation}; +use crate::internal::incompatibility::IncompId; use crate::internal::incompatibility::{Incompatibility, Relation}; use crate::internal::partial_solution::{DecisionLevel, PartialSolution}; use crate::package::Package; use crate::report::DerivationTree; +use crate::solver::DependencyConstraints; use crate::version::Version; /// Current state of the PubGrub algorithm. @@ -20,18 +23,14 @@ pub struct State { root_version: V, /// TODO: remove pub. - pub incompatibilities: Rc>>, + pub incompatibilities: Rc>>, /// Partial solution. /// TODO: remove pub. pub partial_solution: PartialSolution, /// The store is the reference storage for all incompatibilities. - /// The id field in one incompatibility refers - /// to the position in the [incompatibility_store](State::incompatibility_store) vec, - /// NOT the position in the [incompatibilities](State::incompatibilities) vec. - /// TODO: remove pub. - pub incompatibility_store: Vec>, + pub incompatibility_store: Arena>, /// This is a stack of work to be done in `unit_propagation`. /// It can definitely be a local variable to that method, but @@ -42,23 +41,48 @@ pub struct State { impl State { /// Initialization of PubGrub state. pub fn init(root_package: P, root_version: V) -> Self { - let not_root_incompat = - Incompatibility::not_root(0, root_package.clone(), root_version.clone()); + let mut incompatibility_store = Arena::new(); + let not_root_id = incompatibility_store.alloc(Incompatibility::not_root( + root_package.clone(), + root_version.clone(), + )); Self { root_package, root_version, - incompatibilities: Rc::new(vec![not_root_incompat.clone()]), + incompatibilities: Rc::new(vec![not_root_id]), partial_solution: PartialSolution::empty(), - incompatibility_store: vec![not_root_incompat], + incompatibility_store, unit_propagation_buffer: vec![], } } /// Add an incompatibility to the state. - pub fn add_incompatibility Incompatibility>(&mut self, gen_incompat: F) { - let incompat = gen_incompat(self.incompatibility_store.len()); - self.incompatibility_store.push(incompat.clone()); - incompat.merge_into(Rc::make_mut(&mut self.incompatibilities)); + pub fn add_incompatibility(&mut self, incompat: Incompatibility) { + Incompatibility::merge_into( + self.incompatibility_store.alloc(incompat), + Rc::make_mut(&mut self.incompatibilities), + ); + } + + /// Add an incompatibility to the state. + pub fn add_incompatibility_from_dependencies( + &mut self, + package: P, + version: V, + deps: &DependencyConstraints, + ) -> std::ops::Range> { + // Create incompatibilities and allocate them in the store. + let new_incompats_id_range = self + .incompatibility_store + .alloc_iter(deps.iter().map(|dep| { + Incompatibility::from_dependency(package.clone(), version.clone(), dep) + })); + // Merge the newly created incompatibilities with the older ones. + let incompatibilities = Rc::make_mut(&mut self.incompatibilities); + for id in IncompId::range_to_iter(new_incompats_id_range.clone()) { + Incompatibility::merge_into(id, incompatibilities); + } + new_incompats_id_range } /// Check if an incompatibility is terminal. @@ -74,27 +98,34 @@ impl State { while let Some(current_package) = self.unit_propagation_buffer.pop() { // Iterate over incompatibilities in reverse order // to evaluate first the newest incompatibilities. - for incompat in Rc::clone(&self.incompatibilities).iter().rev() { + for &incompat_id in Rc::clone(&self.incompatibilities).iter().rev() { + let current_incompat = &self.incompatibility_store[incompat_id]; // We only care about that incompatibility if it contains the current package. - if incompat.get(¤t_package) == None { + if current_incompat.get(¤t_package).is_none() { continue; } - match self.partial_solution.relation(&incompat) { + match self.partial_solution.relation(current_incompat) { // If the partial solution satisfies the incompatibility // we must perform conflict resolution. Relation::Satisfied => { - let (package_almost, root_cause) = self.conflict_resolution(&incompat)?; + let (package_almost, root_cause) = self.conflict_resolution(incompat_id)?; self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package_almost.clone()); // Add to the partial solution with incompat as cause. - self.partial_solution - .add_derivation(package_almost, root_cause); + self.partial_solution.add_derivation( + package_almost, + root_cause, + &self.incompatibility_store, + ); } Relation::AlmostSatisfied(package_almost) => { self.unit_propagation_buffer.push(package_almost.clone()); // Add (not term) to the partial solution with incompat as cause. - self.partial_solution - .add_derivation(package_almost, incompat.clone()); + self.partial_solution.add_derivation( + package_almost, + incompat_id, + &self.incompatibility_store, + ); } _ => {} } @@ -108,46 +139,49 @@ impl State { /// CF fn conflict_resolution( &mut self, - incompatibility: &Incompatibility, - ) -> Result<(P, Incompatibility), PubGrubError> { - let mut current_incompat = incompatibility.clone(); + incompatibility: IncompId, + ) -> Result<(P, IncompId), PubGrubError> { + let mut current_incompat_id = incompatibility; let mut current_incompat_changed = false; loop { - if current_incompat.is_terminal(&self.root_package, &self.root_version) { + if self.incompatibility_store[current_incompat_id] + .is_terminal(&self.root_package, &self.root_version) + { return Err(PubGrubError::NoSolution( - self.build_derivation_tree(¤t_incompat), + self.build_derivation_tree(current_incompat_id), )); } else { let (satisfier, satisfier_level, previous_satisfier_level) = self .partial_solution - .find_satisfier_and_previous_satisfier_level(¤t_incompat); + .find_satisfier_and_previous_satisfier_level( + &self.incompatibility_store[current_incompat_id], + &self.incompatibility_store, + ); match satisfier { Decision { package, .. } => { self.backtrack( - current_incompat.clone(), + current_incompat_id, current_incompat_changed, previous_satisfier_level, ); - return Ok((package, current_incompat)); + return Ok((package, current_incompat_id)); } Derivation { cause, package } => { if previous_satisfier_level != satisfier_level { self.backtrack( - current_incompat.clone(), + current_incompat_id, current_incompat_changed, previous_satisfier_level, ); - return Ok((package, current_incompat)); + return Ok((package, current_incompat_id)); } else { - let id = self.incompatibility_store.len(); let prior_cause = Incompatibility::prior_cause( - id, - ¤t_incompat, - &cause, + current_incompat_id, + cause, &package, + &self.incompatibility_store, ); - self.incompatibility_store.push(prior_cause.clone()); - current_incompat = prior_cause; + current_incompat_id = self.incompatibility_store.alloc(prior_cause); current_incompat_changed = true; } } @@ -159,36 +193,36 @@ impl State { /// Backtracking. fn backtrack( &mut self, - incompat: Incompatibility, + incompat: IncompId, incompat_changed: bool, decision_level: DecisionLevel, ) { - self.partial_solution.backtrack(decision_level); + self.partial_solution + .backtrack(decision_level, &self.incompatibility_store); if incompat_changed { - incompat.merge_into(Rc::make_mut(&mut self.incompatibilities)); + Incompatibility::merge_into(incompat, Rc::make_mut(&mut self.incompatibilities)); } } // Error reporting ######################################################### - fn build_derivation_tree(&self, incompat: &Incompatibility) -> DerivationTree { + fn build_derivation_tree(&self, incompat: IncompId) -> DerivationTree { let shared_ids = self.find_shared_ids(incompat); - incompat.build_derivation_tree(&shared_ids, self.incompatibility_store.as_slice()) + Incompatibility::build_derivation_tree(incompat, &shared_ids, &self.incompatibility_store) } - fn find_shared_ids(&self, incompat: &Incompatibility) -> Set { + fn find_shared_ids(&self, incompat: IncompId) -> Set> { let mut all_ids = Set::new(); let mut shared_ids = Set::new(); - let mut stack = Vec::new(); - stack.push(incompat); + let mut stack = vec![incompat]; while let Some(i) = stack.pop() { - if let Some((id1, id2)) = i.causes() { - if all_ids.contains(&i.id) { - shared_ids.insert(i.id); + if let Some((id1, id2)) = self.incompatibility_store[i].causes() { + if all_ids.contains(&i) { + shared_ids.insert(i); } else { - all_ids.insert(i.id); - stack.push(&self.incompatibility_store[id1]); - stack.push(&self.incompatibility_store[id2]); + all_ids.insert(i); + stack.push(id1); + stack.push(id2); } } } diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 7fc52e45..5bb4c577 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -6,11 +6,11 @@ use std::collections::HashSet as Set; use std::fmt; +use crate::internal::arena::{Arena, Id}; use crate::internal::small_map::SmallMap; use crate::package::Package; use crate::range::Range; use crate::report::{DefaultStringReporter, DerivationTree, Derived, External}; -use crate::solver::DependencyConstraints; use crate::term::{self, Term}; use crate::version::Version; @@ -31,12 +31,13 @@ use crate::version::Version; /// [PubGrub documentation](https://github.com/dart-lang/pub/blob/master/doc/solver.md#incompatibility). #[derive(Debug, Clone)] pub struct Incompatibility { - /// TODO: remove pub. - pub id: usize, package_terms: SmallMap>, kind: Kind, } +/// Type alias of unique identifiers for incompatibilities. +pub type IncompId = Id>; + #[derive(Debug, Clone)] enum Kind { /// Initial incompatibility aiming at picking the root package for the first decision. @@ -48,7 +49,7 @@ enum Kind { /// Incompatibility coming from the dependencies of a given package. FromDependencyOf(P, Range, P, Range), /// Derived from two causes. Stores cause ids. - DerivedFrom(usize, usize), + DerivedFrom(IncompId, IncompId), } /// A type alias for a pair of [Package] and a corresponding [Term]. @@ -73,9 +74,8 @@ pub enum Relation { impl Incompatibility { /// Create the initial "not Root" incompatibility. - pub fn not_root(id: usize, package: P, version: V) -> Self { + pub fn not_root(package: P, version: V) -> Self { Self { - id, package_terms: SmallMap::One([( package.clone(), Term::Negative(Range::exact(version.clone())), @@ -86,13 +86,12 @@ impl Incompatibility { /// Create an incompatibility to remember /// that a given range does not contain any version. - pub fn no_versions(id: usize, package: P, term: Term) -> Self { + pub fn no_versions(package: P, term: Term) -> Self { let range = match &term { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), }; Self { - id, package_terms: SmallMap::One([(package.clone(), term)]), kind: Kind::NoVersions(package, range), } @@ -101,36 +100,19 @@ impl Incompatibility { /// Create an incompatibility to remember /// that a package version is not selectable /// because its list of dependencies is unavailable. - pub fn unavailable_dependencies(id: usize, package: P, version: V) -> Self { + pub fn unavailable_dependencies(package: P, version: V) -> Self { let range = Range::exact(version); Self { - id, package_terms: SmallMap::One([(package.clone(), Term::Positive(range.clone()))]), kind: Kind::UnavailableDependencies(package, range), } } - /// Generate a list of incompatibilities from direct dependencies of a package. - pub fn from_dependencies( - start_id: usize, - package: P, - version: V, - deps: &DependencyConstraints, - ) -> Vec { - deps.iter() - .enumerate() - .map(|(i, dep)| { - Self::from_dependency(start_id + i, package.clone(), version.clone(), dep) - }) - .collect() - } - /// Build an incompatibility from a given dependency. - fn from_dependency(id: usize, package: P, version: V, dep: (&P, &Range)) -> Self { + pub fn from_dependency(package: P, version: V, dep: (&P, &Range)) -> Self { let range1 = Range::exact(version); let (p2, range2) = dep; Self { - id, package_terms: SmallMap::Two([ (package.clone(), Term::Positive(range1.clone())), (p2.clone(), Term::Negative(range2.clone())), @@ -161,28 +143,30 @@ impl Incompatibility { /// It may not be trivial since those incompatibilities /// may already have derived others. /// Maybe this should not be pursued. - pub fn merge_into(self, incompatibilities: &mut Vec) { - incompatibilities.push(self); + pub fn merge_into(id: Id, incompatibilities: &mut Vec>) { + incompatibilities.push(id); } /// Prior cause of two incompatibilities using the rule of resolution. - pub fn prior_cause(id: usize, incompat: &Self, satisfier_cause: &Self, package: &P) -> Self { - let kind = Kind::DerivedFrom(incompat.id, satisfier_cause.id); - let mut package_terms = incompat.package_terms.clone(); + pub fn prior_cause( + incompat: Id, + satisfier_cause: Id, + package: &P, + incompatibility_store: &Arena, + ) -> Self { + let kind = Kind::DerivedFrom(incompat, satisfier_cause); + let mut package_terms = incompatibility_store[incompat].package_terms.clone(); let t1 = package_terms.remove(package).unwrap(); + let satisfier_cause_terms = &incompatibility_store[satisfier_cause].package_terms; package_terms.merge( - satisfier_cause - .package_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.package_terms.get(package).unwrap()); + let term = t1.union(satisfier_cause_terms.get(package).unwrap()); if term != Term::any() { package_terms.insert(package.clone(), term); } Self { - id, package_terms, kind, } @@ -240,7 +224,7 @@ impl Incompatibility { // Reporting ############################################################### /// Retrieve parent causes if of type DerivedFrom. - pub fn causes(&self) -> Option<(usize, usize)> { + pub fn causes(&self) -> Option<(Id, Id)> { match self.kind { Kind::DerivedFrom(id1, id2) => Some((id1, id2)), _ => None, @@ -249,17 +233,17 @@ impl Incompatibility { /// Build a derivation tree for error reporting. pub fn build_derivation_tree( - &self, - shared_ids: &Set, - store: &[Self], + self_id: Id, + shared_ids: &Set>, + store: &Arena, ) -> DerivationTree { - match &self.kind { + match &store[self_id].kind { Kind::DerivedFrom(id1, id2) => { - let cause1 = store[*id1].build_derivation_tree(shared_ids, store); - let cause2 = store[*id2].build_derivation_tree(shared_ids, store); + let cause1 = Self::build_derivation_tree(*id1, shared_ids, store); + let cause2 = Self::build_derivation_tree(*id2, shared_ids, store); let derived = Derived { - terms: self.package_terms.as_map(), - shared_id: shared_ids.get(&self.id).cloned(), + terms: store[self_id].package_terms.as_map(), + shared_id: shared_ids.get(&self_id).map(|id| id.into_raw()), cause1: Box::new(cause1), cause2: Box::new(cause2), }; @@ -316,23 +300,22 @@ pub mod tests { /// { p1: t1, p3: t3 } #[test] fn rule_of_resolution(t1 in term_strat(), t2 in term_strat(), t3 in term_strat()) { - let i1 = Incompatibility { - id: 0, + let mut store = Arena::new(); + let i1 = store.alloc(Incompatibility { package_terms: SmallMap::Two([("p1", t1.clone()), ("p2", t2.negate())]), - kind: Kind::DerivedFrom(0,0) - }; + kind: Kind::UnavailableDependencies("0", Range::any()) + }); - let i2 = Incompatibility { - id: 0, - package_terms: SmallMap::Two([("p2", t2.clone()), ("p3", t3.clone())]), - kind: Kind::DerivedFrom(0,0) - }; + let i2 = store.alloc(Incompatibility { + package_terms: SmallMap::Two([("p2", t2), ("p3", t3.clone())]), + kind: Kind::UnavailableDependencies("0", Range::any()) + }); let mut i3 = Map::default(); i3.insert("p1", t1); i3.insert("p3", t3); - let i_resolution = Incompatibility::prior_cause(0, &i1, &i2, &"p2"); + let i_resolution = Incompatibility::prior_cause(i1, i2, &"p2", &store); assert_eq!(i_resolution.package_terms.as_map(), i3); } diff --git a/src/internal/memory.rs b/src/internal/memory.rs index 9773c5c6..a54d47f1 100644 --- a/src/internal/memory.rs +++ b/src/internal/memory.rs @@ -3,7 +3,9 @@ //! A Memory acts like a structured partial solution //! where terms are regrouped by package in a [Map](crate::type_aliases::Map). +use crate::internal::arena::Arena; use crate::internal::assignment::Assignment::{self, Decision, Derivation}; +use crate::internal::incompatibility::Incompatibility; use crate::package::Package; use crate::range::Range; use crate::term::Term; @@ -51,12 +53,17 @@ impl Memory { } /// Building step of a Memory from a given assignment. - pub fn add_assignment(&mut self, assignment: &Assignment) { + pub fn add_assignment( + &mut self, + assignment: &Assignment, + store: &Arena>, + ) { match assignment { Decision { package, version } => self.add_decision(package.clone(), version.clone()), - Derivation { package, cause } => { - self.add_derivation(package.clone(), cause.get(&package).unwrap().negate()) - } + Derivation { package, cause } => self.add_derivation( + package.clone(), + store[*cause].get(&package).unwrap().negate(), + ), } } diff --git a/src/internal/mod.rs b/src/internal/mod.rs index 56e6baeb..e8db3d46 100644 --- a/src/internal/mod.rs +++ b/src/internal/mod.rs @@ -2,6 +2,7 @@ //! Non exposed modules. +pub mod arena; pub mod assignment; pub mod core; pub mod incompatibility; diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 23de5059..3854da9e 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -3,7 +3,9 @@ //! The partial solution is the current state //! of the solution being built by the algorithm. +use crate::internal::arena::Arena; use crate::internal::assignment::Assignment::{self, Decision, Derivation}; +use crate::internal::incompatibility::IncompId; use crate::internal::incompatibility::{Incompatibility, Relation}; use crate::internal::memory::Memory; use crate::package::Package; @@ -64,12 +66,16 @@ impl PartialSolution { } } - fn add_assignment(&mut self, assignment: Assignment) { + fn add_assignment( + &mut self, + assignment: Assignment, + store: &Arena>, + ) { self.decision_level = match assignment { Decision { .. } => self.decision_level + DecisionLevel(1), Derivation { .. } => self.decision_level, }; - self.memory.add_assignment(&assignment); + self.memory.add_assignment(&assignment, store); self.history.push(DatedAssignment { decision_level: self.decision_level, assignment, @@ -77,13 +83,18 @@ impl PartialSolution { } /// Add a decision to the partial solution. - pub fn add_decision(&mut self, package: P, version: V) { - self.add_assignment(Decision { package, version }); + pub fn add_decision(&mut self, package: P, version: V, store: &Arena>) { + self.add_assignment(Decision { package, version }, store); } /// Add a derivation to the partial solution. - pub fn add_derivation(&mut self, package: P, cause: Incompatibility) { - self.add_assignment(Derivation { package, cause }); + pub fn add_derivation( + &mut self, + package: P, + cause: IncompId, + store: &Arena>, + ) { + self.add_assignment(Derivation { package, cause }, store); } /// If a partial solution has, for every positive derivation, @@ -99,7 +110,11 @@ impl PartialSolution { } /// Backtrack the partial solution to a given decision level. - pub fn backtrack(&mut self, decision_level: DecisionLevel) { + pub fn backtrack( + &mut self, + decision_level: DecisionLevel, + store: &Arena>, + ) { let pos = self .history .binary_search_by(|probe| { @@ -119,7 +134,7 @@ impl PartialSolution { let mem = &mut self.memory; self.history .iter() - .for_each(|da| mem.add_assignment(&da.assignment)); + .for_each(|da| mem.add_assignment(&da.assignment, store)); } /// Extract potential packages for the next iteration of unit propagation. @@ -142,7 +157,8 @@ impl PartialSolution { &mut self, package: P, version: V, - new_incompatibilities: &[Incompatibility], + new_incompatibilities: std::ops::Range>, + store: &Arena>, ) { let not_satisfied = |incompat: &Incompatibility| { incompat.relation(|p| { @@ -156,8 +172,8 @@ impl PartialSolution { // Check none of the dependencies (new_incompatibilities) // would create a conflict (be satisfied). - if new_incompatibilities.iter().all(not_satisfied) { - self.add_decision(package, version); + if store[new_incompatibilities].iter().all(not_satisfied) { + self.add_decision(package, version, store); } } @@ -170,14 +186,15 @@ impl PartialSolution { pub fn find_satisfier_and_previous_satisfier_level( &self, incompat: &Incompatibility, + store: &Arena>, ) -> (Assignment, DecisionLevel, DecisionLevel) { let SatisfierAndPreviousHistory { satisfier, previous_history, - } = Self::find_satisfier(incompat, self.history.as_slice()) + } = Self::find_satisfier(incompat, self.history.as_slice(), store) .expect("We should always find a satisfier if called in the right context."); let previous_satisfier_level = - Self::find_previous_satisfier(incompat, &satisfier.assignment, previous_history); + Self::find_previous_satisfier(incompat, &satisfier.assignment, previous_history, store); ( satisfier.assignment, satisfier.decision_level, @@ -191,8 +208,14 @@ impl PartialSolution { fn find_satisfier<'a>( incompat: &Incompatibility, history: &'a [DatedAssignment], + store: &Arena>, ) -> Option> { - Self::find_satisfier_helper(incompat, Self::new_accum_satisfied_from(incompat), history) + Self::find_satisfier_helper( + incompat, + Self::new_accum_satisfied_from(incompat), + history, + store, + ) } /// Earliest assignment in the partial solution before satisfier @@ -202,15 +225,16 @@ impl PartialSolution { incompat: &Incompatibility, satisfier: &Assignment, previous_assignments: &'a [DatedAssignment], + store: &Arena>, ) -> DecisionLevel { let package = satisfier.package().clone(); let incompat_term = incompat.get(&package).expect("This should exist"); - let satisfier_term = satisfier.as_term(); + let satisfier_term = satisfier.as_term(store); let is_satisfied = satisfier_term.subset_of(incompat_term); let mut accum_satisfied = Self::new_accum_satisfied_from(incompat); accum_satisfied.insert(package, (is_satisfied, satisfier_term)); // Search previous satisfier. - Self::find_satisfier_helper(incompat, accum_satisfied, previous_assignments).map_or( + Self::find_satisfier_helper(incompat, accum_satisfied, previous_assignments, store).map_or( DecisionLevel(1), |satisfier_and_previous_history| { satisfier_and_previous_history @@ -235,6 +259,7 @@ impl PartialSolution { incompat: &Incompatibility, accum_satisfied: Map)>, all_assignments: &'a [DatedAssignment], + store: &Arena>, ) -> Option> { let mut accum_satisfied = accum_satisfied; for (idx, dated_assignment) in all_assignments.iter().enumerate() { @@ -250,7 +275,7 @@ impl PartialSolution { Some(x) => x, }; // Check if that incompat term is satisfied by our accumulated terms intersection. - *accum_term = accum_term.intersection(&dated_assignment.assignment.as_term()); + *accum_term = accum_term.intersection(&dated_assignment.assignment.as_term(store)); *is_satisfied = accum_term.subset_of(incompat_term); // Check if we have found the satisfier // (all booleans in accum_satisfied are true). diff --git a/src/solver.rs b/src/solver.rs index 56138a0f..b032b903 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -119,9 +119,7 @@ pub fn resolve( .term_intersection_for_package(&next) .expect("a package was chosen but we don't have a term.") .clone(); - state.add_incompatibility(|id| { - Incompatibility::no_versions(id, next.clone(), term.clone()) - }); + state.add_incompatibility(Incompatibility::no_versions(next.clone(), term.clone())); continue; } Some(x) => x, @@ -153,9 +151,10 @@ pub fn resolve( source: err, })? { Dependencies::Unknown => { - state.add_incompatibility(|id| { - Incompatibility::unavailable_dependencies(id, p.clone(), v.clone()) - }); + state.add_incompatibility(Incompatibility::unavailable_dependencies( + p.clone(), + v.clone(), + )); continue; } Dependencies::Known(x) => { @@ -177,14 +176,10 @@ pub fn resolve( }; // Add that package and version if the dependencies are not problematic. - let start_id = state.incompatibility_store.len(); let dep_incompats = - Incompatibility::from_dependencies(start_id, p.clone(), v.clone(), &dependencies); + state.add_incompatibility_from_dependencies(p.clone(), v.clone(), &dependencies); - for incompat in dep_incompats.iter() { - state.add_incompatibility(|_| incompat.clone()); - } - if dep_incompats + if state.incompatibility_store[dep_incompats.clone()] .iter() .any(|incompat| state.is_terminal(incompat)) { @@ -194,13 +189,18 @@ pub fn resolve( "Root package depends on itself at a different version?".into(), )); } - state - .partial_solution - .add_version(p.clone(), v, &dep_incompats); + state.partial_solution.add_version( + p.clone(), + 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. - state.partial_solution.add_decision(next.clone(), v); + state + .partial_solution + .add_decision(next.clone(), v, &state.incompatibility_store); } } } From f6679139937dd9dea4381ceb6a29f32b7e8b2a8d Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 30 Mar 2021 11:32:55 -0400 Subject: [PATCH 10/22] refactor: inline find_satisfier_helper and try to simplify (#79) * refactor: remove SatisfierAndPreviousHistory * refactor: remove add_assignment * refactor: keep track of when each package was satisfied * refactor: inline find_satisfier_helper and try to simplify * docs: add a note about find_satisfier returned map * refactor: impl From instead of Into for clippy Co-authored-by: Matthieu Pizenberg --- src/internal/core.rs | 2 +- src/internal/incompatibility.rs | 5 + src/internal/memory.rs | 4 +- src/internal/partial_solution.rs | 175 +++++++++++++++---------------- src/solver.rs | 4 +- src/version.rs | 12 +-- 6 files changed, 97 insertions(+), 105 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 4e852d4e..98293081 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -157,7 +157,7 @@ impl State { &self.incompatibility_store[current_incompat_id], &self.incompatibility_store, ); - match satisfier { + match satisfier.clone() { Decision { package, .. } => { self.backtrack( current_incompat_id, diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 5bb4c577..dfcccf30 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -221,6 +221,11 @@ impl Incompatibility { self.package_terms.iter() } + // The number of packages. + pub fn len(&self) -> usize { + self.package_terms.len() + } + // Reporting ############################################################### /// Retrieve parent causes if of type DerivedFrom. diff --git a/src/internal/memory.rs b/src/internal/memory.rs index a54d47f1..5d3acde3 100644 --- a/src/internal/memory.rs +++ b/src/internal/memory.rs @@ -68,7 +68,7 @@ impl Memory { } /// Add a decision to a Memory. - fn add_decision(&mut self, package: P, version: V) { + pub fn add_decision(&mut self, package: P, version: V) { // Check that add_decision is never used in the wrong context. if cfg!(debug_assertions) { match self.assignments.get_mut(&package) { @@ -87,7 +87,7 @@ impl Memory { } /// Add a derivation to a Memory. - fn add_derivation(&mut self, package: P, term: Term) { + pub fn add_derivation(&mut self, package: P, term: Term) { use std::collections::hash_map::Entry; match self.assignments.entry(package) { Entry::Occupied(mut o) => match o.get_mut() { diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 3854da9e..8be4b805 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -37,11 +37,6 @@ pub struct DatedAssignment { assignment: Assignment, } -pub struct SatisfierAndPreviousHistory<'a, P: Package, V: Version> { - satisfier: DatedAssignment, - previous_history: &'a [DatedAssignment], -} - /// The partial solution is the current state /// of the solution being built by the algorithm. /// It is composed of a succession of assignments, @@ -66,27 +61,16 @@ impl PartialSolution { } } - fn add_assignment( - &mut self, - assignment: Assignment, - store: &Arena>, - ) { - self.decision_level = match assignment { - Decision { .. } => self.decision_level + DecisionLevel(1), - Derivation { .. } => self.decision_level, - }; - self.memory.add_assignment(&assignment, store); + /// Add a decision to the partial solution. + pub fn add_decision(&mut self, package: P, version: V) { + self.decision_level = self.decision_level + DecisionLevel(1); + self.memory.add_decision(package.clone(), version.clone()); self.history.push(DatedAssignment { decision_level: self.decision_level, - assignment, + assignment: Decision { package, version }, }); } - /// Add a decision to the partial solution. - pub fn add_decision(&mut self, package: P, version: V, store: &Arena>) { - self.add_assignment(Decision { package, version }, store); - } - /// Add a derivation to the partial solution. pub fn add_derivation( &mut self, @@ -94,7 +78,14 @@ impl PartialSolution { cause: IncompId, store: &Arena>, ) { - self.add_assignment(Derivation { package, cause }, store); + self.memory.add_derivation( + package.clone(), + store[cause].get(&package).unwrap().negate(), + ); + self.history.push(DatedAssignment { + decision_level: self.decision_level, + assignment: Derivation { package, cause }, + }); } /// If a partial solution has, for every positive derivation, @@ -173,7 +164,7 @@ impl PartialSolution { // Check none of the dependencies (new_incompatibilities) // would create a conflict (be satisfied). if store[new_incompatibilities].iter().all(not_satisfied) { - self.add_decision(package, version, store); + self.add_decision(package, version); } } @@ -187,16 +178,24 @@ impl PartialSolution { &self, incompat: &Incompatibility, store: &Arena>, - ) -> (Assignment, DecisionLevel, DecisionLevel) { - let SatisfierAndPreviousHistory { - satisfier, - previous_history, - } = Self::find_satisfier(incompat, self.history.as_slice(), store) - .expect("We should always find a satisfier if called in the right context."); - let previous_satisfier_level = - Self::find_previous_satisfier(incompat, &satisfier.assignment, previous_history, store); + ) -> (&Assignment, DecisionLevel, DecisionLevel) { + let satisfier_map = Self::find_satisfier(incompat, self.history.as_slice(), store); + assert_eq!( + satisfier_map.len(), + incompat.len(), + "We should always find a satisfier if called in the right context." + ); + let &satisfier_idx = satisfier_map.values().max().unwrap(); + let satisfier = &self.history[satisfier_idx]; + let previous_satisfier_level = Self::find_previous_satisfier( + incompat, + &satisfier.assignment, + satisfier_map, + &self.history[0..=satisfier_idx], + store, + ); ( - satisfier.assignment, + &satisfier.assignment, satisfier.decision_level, previous_satisfier_level, ) @@ -204,18 +203,47 @@ impl PartialSolution { /// A satisfier is the earliest assignment in partial solution such that the incompatibility /// is satisfied by the partial solution up to and including that assignment. - /// Also returns all assignments earlier than the satisfier. + /// + /// Returns a map indicating for each package term, when that was first satisfied in history. + /// If we effectively found a satisfier, the returned map must be the same size that incompat. fn find_satisfier<'a>( incompat: &Incompatibility, history: &'a [DatedAssignment], store: &Arena>, - ) -> Option> { - Self::find_satisfier_helper( - incompat, - Self::new_accum_satisfied_from(incompat), - history, - store, - ) + ) -> Map { + let mut accum_satisfied: Map> = incompat + .iter() + .map(|(p, _)| (p.clone(), Term::any())) + .collect(); + let mut satisfied = Map::with_capacity_and_hasher(incompat.len(), Default::default()); + for (idx, dated_assignment) in history.iter().enumerate() { + let package = dated_assignment.assignment.package(); + if satisfied.contains_key(package) { + continue; // If that package term is already satisfied, no need to check. + } + let incompat_term = match incompat.get(package) { + // We only care about packages related to the incompatibility. + None => continue, + Some(i) => i, + }; + let accum_term = match accum_satisfied.get_mut(package) { + // We only care about packages related to the accum_satisfied. + None => continue, + Some(i) => i, + }; + + // Check if that incompat term is satisfied by our accumulated terms intersection. + *accum_term = accum_term.intersection(&dated_assignment.assignment.as_term(store)); + // Check if we have found the satisfier + // (that all terms are satisfied). + if accum_term.subset_of(incompat_term) { + satisfied.insert(package.clone(), idx); + if satisfied.len() == incompat.len() { + break; + } + } + } + satisfied } /// Earliest assignment in the partial solution before satisfier @@ -224,68 +252,29 @@ impl PartialSolution { fn find_previous_satisfier<'a>( incompat: &Incompatibility, satisfier: &Assignment, + mut satisfier_map: Map, previous_assignments: &'a [DatedAssignment], store: &Arena>, ) -> DecisionLevel { let package = satisfier.package().clone(); - let incompat_term = incompat.get(&package).expect("This should exist"); - let satisfier_term = satisfier.as_term(store); - let is_satisfied = satisfier_term.subset_of(incompat_term); - let mut accum_satisfied = Self::new_accum_satisfied_from(incompat); - accum_satisfied.insert(package, (is_satisfied, satisfier_term)); + let mut accum_term = satisfier.as_term(store); + let incompat_term = incompat.get(&package).expect("package not in satisfier"); // Search previous satisfier. - Self::find_satisfier_helper(incompat, accum_satisfied, previous_assignments, store).map_or( - DecisionLevel(1), - |satisfier_and_previous_history| { - satisfier_and_previous_history - .satisfier - .decision_level - .max(DecisionLevel(1)) - }, - ) - } - - fn new_accum_satisfied_from(incompat: &Incompatibility) -> Map)> { - incompat - .iter() - .map(|(p, _)| (p.clone(), (false, Term::any()))) - .collect() - } - - /// Iterate over the assignments (oldest must be first) - /// until we find the first one such that the set of all assignments until this one (included) - /// satisfies the given incompatibility. - pub fn find_satisfier_helper<'a>( - incompat: &Incompatibility, - accum_satisfied: Map)>, - all_assignments: &'a [DatedAssignment], - store: &Arena>, - ) -> Option> { - let mut accum_satisfied = accum_satisfied; - for (idx, dated_assignment) in all_assignments.iter().enumerate() { - let package = dated_assignment.assignment.package(); - let incompat_term = match incompat.get(package) { + for (idx, dated_assignment) in previous_assignments.iter().enumerate() { + if dated_assignment.assignment.package() != &package { // We only care about packages related to the incompatibility. - None => continue, - Some(i) => i, - }; - let (is_satisfied, accum_term) = match accum_satisfied.get_mut(package) { - None => panic!("A package in incompat should always exist in accum_satisfied"), - Some((true, _)) => continue, // If that package term is already satisfied, no need to check. - Some(x) => x, - }; + continue; + } // Check if that incompat term is satisfied by our accumulated terms intersection. - *accum_term = accum_term.intersection(&dated_assignment.assignment.as_term(store)); - *is_satisfied = accum_term.subset_of(incompat_term); + accum_term = accum_term.intersection(&dated_assignment.assignment.as_term(store)); // Check if we have found the satisfier - // (all booleans in accum_satisfied are true). - if *is_satisfied && accum_satisfied.iter().all(|(_, (satisfied, _))| *satisfied) { - return Some(SatisfierAndPreviousHistory { - satisfier: dated_assignment.clone(), - previous_history: &all_assignments[0..idx], - }); + if accum_term.subset_of(incompat_term) { + satisfier_map.insert(package.clone(), idx); + break; } } - None + previous_assignments[*satisfier_map.values().max().unwrap()] + .decision_level + .max(DecisionLevel(1)) } } diff --git a/src/solver.rs b/src/solver.rs index b032b903..490d0ee1 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -198,9 +198,7 @@ pub fn resolve( } else { // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied // terms and can add the decision directly. - state - .partial_solution - .add_decision(next.clone(), v, &state.incompatibility_store); + state.partial_solution.add_decision(next.clone(), v); } } } diff --git a/src/version.rs b/src/version.rs index 33ebf085..c7d749ee 100644 --- a/src/version.rs +++ b/src/version.rs @@ -81,9 +81,9 @@ impl From<(u32, u32, u32)> for SemanticVersion { } // Convert a version into a tuple (major, minor, patch). -impl Into<(u32, u32, u32)> for SemanticVersion { - fn into(self) -> (u32, u32, u32) { - (self.major, self.minor, self.patch) +impl From for (u32, u32, u32) { + fn from(v: SemanticVersion) -> Self { + (v.major, v.minor, v.patch) } } @@ -238,9 +238,9 @@ impl From for NumberVersion { } // Convert a version into an usize. -impl Into for NumberVersion { - fn into(self) -> u32 { - self.0 +impl From for u32 { + fn from(version: NumberVersion) -> Self { + version.0 } } From b7087ba7571cf3823152fbc0b84887cbbdca3c7c Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Sat, 17 Apr 2021 14:27:48 -0400 Subject: [PATCH 11/22] refactor: restart the unit propagation loop on Backtrack (#81) --- src/internal/core.rs | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 98293081..825bb074 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -3,7 +3,7 @@ //! Core model and functions //! to write a functional PubGrub algorithm. -use std::{collections::HashSet as Set, rc::Rc}; +use std::collections::HashSet as Set; use crate::error::PubGrubError; use crate::internal::arena::Arena; @@ -23,7 +23,7 @@ pub struct State { root_version: V, /// TODO: remove pub. - pub incompatibilities: Rc>>, + pub incompatibilities: Vec>, /// Partial solution. /// TODO: remove pub. @@ -49,7 +49,7 @@ impl State { Self { root_package, root_version, - incompatibilities: Rc::new(vec![not_root_id]), + incompatibilities: vec![not_root_id], partial_solution: PartialSolution::empty(), incompatibility_store, unit_propagation_buffer: vec![], @@ -60,7 +60,7 @@ impl State { pub fn add_incompatibility(&mut self, incompat: Incompatibility) { Incompatibility::merge_into( self.incompatibility_store.alloc(incompat), - Rc::make_mut(&mut self.incompatibilities), + &mut self.incompatibilities, ); } @@ -78,9 +78,8 @@ impl State { Incompatibility::from_dependency(package.clone(), version.clone(), dep) })); // Merge the newly created incompatibilities with the older ones. - let incompatibilities = Rc::make_mut(&mut self.incompatibilities); for id in IncompId::range_to_iter(new_incompats_id_range.clone()) { - Incompatibility::merge_into(id, incompatibilities); + Incompatibility::merge_into(id, &mut self.incompatibilities); } new_incompats_id_range } @@ -98,7 +97,8 @@ impl State { while let Some(current_package) = self.unit_propagation_buffer.pop() { // Iterate over incompatibilities in reverse order // to evaluate first the newest incompatibilities. - for &incompat_id in Rc::clone(&self.incompatibilities).iter().rev() { + let mut conflict_id = None; + for &incompat_id in self.incompatibilities.iter().rev() { let current_incompat = &self.incompatibility_store[incompat_id]; // We only care about that incompatibility if it contains the current package. if current_incompat.get(¤t_package).is_none() { @@ -108,15 +108,8 @@ impl State { // If the partial solution satisfies the incompatibility // we must perform conflict resolution. Relation::Satisfied => { - let (package_almost, root_cause) = self.conflict_resolution(incompat_id)?; - self.unit_propagation_buffer.clear(); - self.unit_propagation_buffer.push(package_almost.clone()); - // Add to the partial solution with incompat as cause. - self.partial_solution.add_derivation( - package_almost, - root_cause, - &self.incompatibility_store, - ); + conflict_id = Some(incompat_id); + break; } Relation::AlmostSatisfied(package_almost) => { self.unit_propagation_buffer.push(package_almost.clone()); @@ -130,6 +123,17 @@ impl State { _ => {} } } + if let Some(incompat_id) = conflict_id { + let (package_almost, root_cause) = self.conflict_resolution(incompat_id)?; + self.unit_propagation_buffer.clear(); + self.unit_propagation_buffer.push(package_almost.clone()); + // Add to the partial solution with incompat as cause. + self.partial_solution.add_derivation( + package_almost, + root_cause, + &self.incompatibility_store, + ); + } } // If there are no more changed packages, unit propagation is done. Ok(()) @@ -200,7 +204,7 @@ impl State { self.partial_solution .backtrack(decision_level, &self.incompatibility_store); if incompat_changed { - Incompatibility::merge_into(incompat, Rc::make_mut(&mut self.incompatibilities)); + Incompatibility::merge_into(incompat, &mut self.incompatibilities); } } From 6be0fbf434d548a0d90f3b85bceb4c9c1b1a1711 Mon Sep 17 00:00:00 2001 From: Marcin Puc <5671049+tranzystorek-io@users.noreply.github.com> Date: Thu, 29 Apr 2021 20:24:09 +0200 Subject: [PATCH 12/22] refactor: add minor code clean-up (#83) * refactor: add minor code clean-up * style: adjust code to project convention * style: unmerge std imports in SmallVec * style: rename option args to `maybe_*` --- src/internal/partial_solution.rs | 2 +- src/internal/small_vec.rs | 27 +++++++++++++++++++++++---- src/range.rs | 30 +++++++++++++----------------- src/report.rs | 9 +++++---- src/term.rs | 2 +- tests/sat_dependency_provider.rs | 6 +++--- 6 files changed, 46 insertions(+), 30 deletions(-) diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 8be4b805..523ad02a 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -179,7 +179,7 @@ impl PartialSolution { incompat: &Incompatibility, store: &Arena>, ) -> (&Assignment, DecisionLevel, DecisionLevel) { - let satisfier_map = Self::find_satisfier(incompat, self.history.as_slice(), store); + let satisfier_map = Self::find_satisfier(incompat, &self.history, store); assert_eq!( satisfier_map.len(), incompat.len(), diff --git a/src/internal/small_vec.rs b/src/internal/small_vec.rs index 98c0dd1e..7d97195b 100644 --- a/src/internal/small_vec.rs +++ b/src/internal/small_vec.rs @@ -1,4 +1,5 @@ use std::fmt; +use std::ops::Deref; #[derive(Clone)] pub enum SmallVec { @@ -38,7 +39,7 @@ impl SmallVec { } } - pub fn iter(&self) -> impl Iterator { + pub fn iter(&self) -> std::slice::Iter<'_, T> { self.as_slice().iter() } } @@ -49,10 +50,28 @@ impl Default for SmallVec { } } -impl Eq for SmallVec {} +impl Deref for SmallVec { + type Target = [T]; -impl PartialEq> for SmallVec { - fn eq(&self, other: &SmallVec) -> bool { + fn deref(&self) -> &Self::Target { + self.as_slice() + } +} + +impl<'a, T> IntoIterator for &'a SmallVec { + type Item = &'a T; + + type IntoIter = std::slice::Iter<'a, T>; + + fn into_iter(self) -> Self::IntoIter { + self.iter() + } +} + +impl Eq for SmallVec {} + +impl PartialEq for SmallVec { + fn eq(&self, other: &Self) -> bool { self.as_slice() == other.as_slice() } } diff --git a/src/range.rs b/src/range.rs index cc4ea432..8de8b3ff 100644 --- a/src/range.rs +++ b/src/range.rs @@ -93,7 +93,7 @@ impl Range { /// Compute the complement set of versions. pub fn negate(&self) -> Self { - match self.segments.as_slice().first() { + match self.segments.first() { None => Self::any(), // Complement of ∅ is * // First high bound is +∞ @@ -110,9 +110,9 @@ impl Range { // First high bound is not +∞ Some((v1, Some(v2))) => { if v1 == &V::lowest() { - Self::negate_segments(v2.clone(), &self.segments.as_slice()[1..]) + Self::negate_segments(v2.clone(), &self.segments[1..]) } else { - Self::negate_segments(V::lowest(), &self.segments.as_slice()) + Self::negate_segments(V::lowest(), &self.segments) } } } @@ -125,11 +125,11 @@ impl Range { fn negate_segments(start: V, segments: &[Interval]) -> Range { let mut complement_segments = SmallVec::empty(); let mut start = Some(start); - for (v1, some_v2) in segments.iter() { + for (v1, maybe_v2) in segments { // start.unwrap() is fine because `segments` is not exposed, // and our usage guaranties that only the last segment may contain a None. complement_segments.push((start.unwrap(), Some(v1.to_owned()))); - start = some_v2.to_owned(); + start = maybe_v2.to_owned(); } if let Some(last) = start { complement_segments.push((last, None)); @@ -144,7 +144,7 @@ impl Range { /// Compute the union of two sets of versions. pub fn union(&self, other: &Self) -> Self { - (self.negate().intersection(&other.negate())).negate() + self.negate().intersection(&other.negate()).negate() } /// Compute the intersection of two sets of versions. @@ -241,8 +241,8 @@ impl Range { impl Range { /// Check if a range contains a given version. pub fn contains(&self, version: &V) -> bool { - for (v1, some_v2) in self.segments.iter() { - match some_v2 { + for (v1, maybe_v2) in &self.segments { + match maybe_v2 { None => return v1 <= version, Some(v2) => { if version < v1 { @@ -258,11 +258,7 @@ impl Range { /// Return the lowest version in the range (if there is one). pub fn lowest_version(&self) -> Option { - self.segments - .as_slice() - .first() - .map(|(start, _)| start) - .cloned() + self.segments.first().map(|(start, _)| start).cloned() } } @@ -288,10 +284,10 @@ impl fmt::Display for Range { } } -fn interval_to_string(interval: &Interval) -> String { - match interval { - (start, Some(end)) => format!("[ {}, {} [", start, end), - (start, None) => format!("[ {}, ∞ [", start), +fn interval_to_string((start, maybe_end): &Interval) -> String { + match maybe_end { + Some(end) => format!("[ {}, {} [", start, end), + None => format!("[ {}, ∞ [", start), } } diff --git a/src/report.rs b/src/report.rs index f9197e70..09815821 100644 --- a/src/report.rs +++ b/src/report.rs @@ -4,6 +4,7 @@ //! dependency solving failed. use std::fmt; +use std::ops::{Deref, DerefMut}; use crate::package::Package; use crate::range::Range; @@ -75,7 +76,7 @@ impl DerivationTree { match self { DerivationTree::External(_) => {} DerivationTree::Derived(derived) => { - match (&mut *derived.cause1, &mut *derived.cause2) { + match (derived.cause1.deref_mut(), derived.cause2.deref_mut()) { (DerivationTree::External(External::NoVersions(p, r)), ref mut cause2) => { cause2.collapse_no_versions(); *self = cause2 @@ -208,7 +209,7 @@ impl DefaultStringReporter { } fn build_recursive_helper(&mut self, current: &Derived) { - match (&*current.cause1, &*current.cause2) { + match (current.cause1.deref(), current.cause2.deref()) { (DerivationTree::External(external1), DerivationTree::External(external2)) => { // Simplest case, we just combine two external incompatibilities. self.lines.push(Self::explain_both_external( @@ -309,7 +310,7 @@ impl DefaultStringReporter { external: &External, current_terms: &Map>, ) { - match (&*derived.cause1, &*derived.cause2) { + match (derived.cause1.deref(), derived.cause2.deref()) { // If the derived cause has itself one external prior cause, // we can chain the external explanations. (DerivationTree::Derived(prior_derived), DerivationTree::External(prior_external)) => { @@ -478,7 +479,7 @@ impl Reporter for DefaultStringReporter { DerivationTree::Derived(derived) => { let mut reporter = Self::new(); reporter.build_recursive(derived); - reporter.lines[..].join("\n") + reporter.lines.join("\n") } } } diff --git a/src/term.rs b/src/term.rs index 63c4da7d..bc038acf 100644 --- a/src/term.rs +++ b/src/term.rs @@ -149,7 +149,7 @@ impl<'a, V: 'a + Version> Term { /// Check if a set of terms satisfies or contradicts a given term. /// Otherwise the relation is inconclusive. pub(crate) fn relation_with(&self, other_terms_intersection: &Term) -> Relation { - let full_intersection = self.intersection(other_terms_intersection.as_ref()); + let full_intersection = self.intersection(other_terms_intersection); if &full_intersection == other_terms_intersection { Relation::Satisfied } else if full_intersection == Self::empty() { diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index 425759a2..fa964ca8 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -34,8 +34,8 @@ fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Va // https://www.it.uu.se/research/group/astra/ModRef10/papers/Alan%20M.%20Frisch%20and%20Paul%20A.%20Giannoros.%20SAT%20Encodings%20of%20the%20At-Most-k%20Constraint%20-%20ModRef%202010.pdf let bits: Vec = solver.new_var_iter(log_bits(vars.len())).collect(); for (i, p) in vars.iter().enumerate() { - for b in 0..bits.len() { - solver.add_clause(&[p.negative(), bits[b].lit(((1 << b) & i) > 0)]); + for (j, &bit) in bits.iter().enumerate() { + solver.add_clause(&[p.negative(), bit.lit(((1 << j) & i) > 0)]); } } } @@ -79,7 +79,7 @@ impl SatResolve { Dependencies::Unknown => panic!(), Dependencies::Known(d) => d, }; - for (p1, range) in deps.iter() { + for (p1, range) in &deps { let empty_vec = vec![]; let mut matches: Vec = all_versions_by_p .get(&p1) From ee7b08471c2bfcd706d4e07b61cee617d8f3f09f Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Fri, 7 May 2021 15:01:30 -0400 Subject: [PATCH 13/22] perf: index incompatibilities by package (#84) * refactor: dont copy the term for Contradicted * perf: index incompatibilities by package * refactor: move merge_into into core * refactor: rename merge_into to merge_incompatibility Co-authored-by: Matthieu Pizenberg --- src/internal/core.rs | 53 +++++++++++++++++++++++--------- src/internal/incompatibility.rs | 37 +++------------------- src/internal/partial_solution.rs | 2 +- 3 files changed, 44 insertions(+), 48 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 825bb074..1274ecbf 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -14,6 +14,7 @@ use crate::internal::partial_solution::{DecisionLevel, PartialSolution}; use crate::package::Package; use crate::report::DerivationTree; use crate::solver::DependencyConstraints; +use crate::type_aliases::Map; use crate::version::Version; /// Current state of the PubGrub algorithm. @@ -22,8 +23,7 @@ pub struct State { root_package: P, root_version: V, - /// TODO: remove pub. - pub incompatibilities: Vec>, + incompatibilities: Map>>, /// Partial solution. /// TODO: remove pub. @@ -46,10 +46,12 @@ impl State { root_package.clone(), root_version.clone(), )); + let mut incompatibilities = Map::default(); + incompatibilities.insert(root_package.clone(), vec![not_root_id]); Self { root_package, root_version, - incompatibilities: vec![not_root_id], + incompatibilities, partial_solution: PartialSolution::empty(), incompatibility_store, unit_propagation_buffer: vec![], @@ -58,10 +60,8 @@ impl State { /// Add an incompatibility to the state. pub fn add_incompatibility(&mut self, incompat: Incompatibility) { - Incompatibility::merge_into( - self.incompatibility_store.alloc(incompat), - &mut self.incompatibilities, - ); + let id = self.incompatibility_store.alloc(incompat); + self.merge_incompatibility(id); } /// Add an incompatibility to the state. @@ -79,7 +79,7 @@ impl State { })); // Merge the newly created incompatibilities with the older ones. for id in IncompId::range_to_iter(new_incompats_id_range.clone()) { - Incompatibility::merge_into(id, &mut self.incompatibilities); + self.merge_incompatibility(id); } new_incompats_id_range } @@ -98,12 +98,9 @@ impl State { // Iterate over incompatibilities in reverse order // to evaluate first the newest incompatibilities. let mut conflict_id = None; - for &incompat_id in self.incompatibilities.iter().rev() { + // We only care about incompatibilities if it contains the current package. + for &incompat_id in self.incompatibilities[¤t_package].iter().rev() { let current_incompat = &self.incompatibility_store[incompat_id]; - // We only care about that incompatibility if it contains the current package. - if current_incompat.get(¤t_package).is_none() { - continue; - } match self.partial_solution.relation(current_incompat) { // If the partial solution satisfies the incompatibility // we must perform conflict resolution. @@ -204,7 +201,35 @@ impl State { self.partial_solution .backtrack(decision_level, &self.incompatibility_store); if incompat_changed { - Incompatibility::merge_into(incompat, &mut self.incompatibilities); + self.merge_incompatibility(incompat); + } + } + + /// Add this incompatibility into the set of all incompatibilities. + /// + /// Pub collapses identical dependencies from adjacent package versions + /// into individual incompatibilities. + /// This substantially reduces the total number of incompatibilities + /// and makes it much easier for Pub to reason about multiple versions of packages at once. + /// + /// For example, rather than representing + /// foo 1.0.0 depends on bar ^1.0.0 and + /// foo 1.1.0 depends on bar ^1.0.0 + /// as two separate incompatibilities, + /// they are collapsed together into the single incompatibility {foo ^1.0.0, not bar ^1.0.0} + /// (provided that no other version of foo exists between 1.0.0 and 2.0.0). + /// We could collapse them into { foo (1.0.0 ∪ 1.1.0), not bar ^1.0.0 } + /// without having to check the existence of other versions though. + /// + /// Here we do the simple stupid thing of just growing the Vec. + /// It may not be trivial since those incompatibilities + /// may already have derived others. + fn merge_incompatibility(&mut self, id: IncompId) { + for (pkg, _term) in self.incompatibility_store[id].iter() { + self.incompatibilities + .entry(pkg.clone()) + .or_default() + .push(id); } } diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index dfcccf30..d8f1318f 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -52,19 +52,16 @@ enum Kind { DerivedFrom(IncompId, IncompId), } -/// A type alias for a pair of [Package] and a corresponding [Term]. -pub type PackageTerm = (P, Term); - /// A Relation describes how a set of terms can be compared to an incompatibility. /// Typically, the set of terms comes from the partial solution. #[derive(Eq, PartialEq)] -pub enum Relation { +pub enum Relation { /// We say that a set of terms S satisfies an incompatibility I /// if S satisfies every term in I. Satisfied, /// We say that S contradicts I /// if S contradicts at least one term in I. - Contradicted(PackageTerm), + Contradicted(P), /// 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), @@ -121,32 +118,6 @@ impl Incompatibility { } } - /// Add this incompatibility into the set of all incompatibilities. - /// - /// Pub collapses identical dependencies from adjacent package versions - /// into individual incompatibilities. - /// This substantially reduces the total number of incompatibilities - /// and makes it much easier for Pub to reason about multiple versions of packages at once. - /// - /// For example, rather than representing - /// foo 1.0.0 depends on bar ^1.0.0 and - /// foo 1.1.0 depends on bar ^1.0.0 - /// as two separate incompatibilities, - /// they are collapsed together into the single incompatibility {foo ^1.0.0, not bar ^1.0.0} - /// (provided that no other version of foo exists between 1.0.0 and 2.0.0). - /// We could collapse them into { foo (1.0.0 ∪ 1.1.0), not bar ^1.0.0 } - /// without having to check the existence of other versions though. - /// And it would even keep the same [Kind]: [FromDependencyOf](Kind::FromDependencyOf) foo. - /// - /// Here we do the simple stupid thing of just growing the Vec. - /// TODO: improve this. - /// It may not be trivial since those incompatibilities - /// may already have derived others. - /// Maybe this should not be pursued. - pub fn merge_into(id: Id, incompatibilities: &mut Vec>) { - incompatibilities.push(id); - } - /// Prior cause of two incompatibilities using the rule of resolution. pub fn prior_cause( incompat: Id, @@ -173,13 +144,13 @@ impl Incompatibility { } /// CF definition of Relation enum. - pub fn relation(&self, mut terms: impl FnMut(&P) -> Option>) -> Relation { + pub fn relation(&self, mut terms: impl FnMut(&P) -> Option>) -> Relation

{ let mut relation = Relation::Satisfied; 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(), incompat_term.clone())); + return Relation::Contradicted(package.clone()); } None | Some(term::Relation::Inconclusive) => { // If a package is not present, the intersection is the same as [Term::any]. diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 523ad02a..c4e7d747 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -169,7 +169,7 @@ impl PartialSolution { } /// Check if the terms in the partial solution satisfy the incompatibility. - pub fn relation(&mut self, incompat: &Incompatibility) -> Relation { + pub fn relation(&mut self, incompat: &Incompatibility) -> Relation

{ incompat.relation(|package| self.memory.term_intersection_for_package(package).cloned()) } From 9b2518e5fd358f0ba26f076ed5e6045424351ebb Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 11 May 2021 14:27:59 -0400 Subject: [PATCH 14/22] perf: remove not_intersected_yet (#87) --- src/internal/incompatibility.rs | 54 +++++++++++++++-------------- src/internal/memory.rs | 58 +++++++++----------------------- src/internal/partial_solution.rs | 13 +++---- 3 files changed, 50 insertions(+), 75 deletions(-) diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index d8f1318f..a7b403a7 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -143,32 +143,6 @@ impl Incompatibility { } } - /// CF definition of Relation enum. - pub fn relation(&self, mut terms: impl FnMut(&P) -> Option>) -> Relation

{ - let mut relation = Relation::Satisfied; - 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()); - } - None | Some(term::Relation::Inconclusive) => { - // If a package is not present, the intersection is the same as [Term::any]. - // According to the rules of satisfactions, the relation would be inconclusive. - // It could also be satisfied if the incompatibility term was also [Term::any], - // but we systematically remove those from incompatibilities - // so we're safe on that front. - if relation == Relation::Satisfied { - relation = Relation::AlmostSatisfied(package.clone()); - } else { - relation = Relation::Inconclusive; - } - } - } - } - relation - } - /// Check if an incompatibility should mark the end of the algorithm /// because it satisfies the root package. pub fn is_terminal(&self, root_package: &P, root_version: &V) -> bool { @@ -246,6 +220,34 @@ impl Incompatibility { } } +impl<'a, P: Package, V: Version + 'a> Incompatibility { + /// CF definition of Relation enum. + pub fn relation(&self, terms: impl Fn(&P) -> Option<&'a Term>) -> Relation

{ + let mut relation = Relation::Satisfied; + 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()); + } + None | Some(term::Relation::Inconclusive) => { + // If a package is not present, the intersection is the same as [Term::any]. + // According to the rules of satisfactions, the relation would be inconclusive. + // It could also be satisfied if the incompatibility term was also [Term::any], + // but we systematically remove those from incompatibilities + // so we're safe on that front. + if relation == Relation::Satisfied { + relation = Relation::AlmostSatisfied(package.clone()); + } else { + relation = Relation::Inconclusive; + } + } + } + } + relation + } +} + impl fmt::Display for Incompatibility { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( diff --git a/src/internal/memory.rs b/src/internal/memory.rs index 5d3acde3..0d43f534 100644 --- a/src/internal/memory.rs +++ b/src/internal/memory.rs @@ -26,10 +26,7 @@ pub struct Memory { #[derive(Clone)] enum PackageAssignments { Decision((V, Term)), - Derivations { - intersected: Term, - not_intersected_yet: Vec>, - }, + Derivations(Term), } impl Memory { @@ -46,9 +43,9 @@ impl Memory { } /// Retrieve intersection of terms in memory related to package. - pub fn term_intersection_for_package(&mut self, package: &P) -> Option<&Term> { + pub fn term_intersection_for_package(&self, package: &P) -> Option<&Term> { self.assignments - .get_mut(package) + .get(package) .map(|pa| pa.assignment_intersection()) } @@ -93,18 +90,12 @@ impl Memory { Entry::Occupied(mut o) => match o.get_mut() { // Check that add_derivation is never called in the wrong context. PackageAssignments::Decision(_) => debug_assert!(false), - PackageAssignments::Derivations { - intersected: _, - not_intersected_yet, - } => { - not_intersected_yet.push(term); + PackageAssignments::Derivations(t) => { + *t = t.intersection(&term); } }, Entry::Vacant(v) => { - v.insert(PackageAssignments::Derivations { - intersected: term, - not_intersected_yet: Vec::new(), - }); + v.insert(PackageAssignments::Derivations(term)); } } } @@ -115,9 +106,9 @@ impl Memory { /// selected version (no "decision") /// and if it contains at least one positive derivation term /// in the partial solution. - pub fn potential_packages(&mut self) -> impl Iterator)> { + pub fn potential_packages(&self) -> impl Iterator)> { self.assignments - .iter_mut() + .iter() .filter_map(|(p, pa)| pa.potential_package_filter(p)) } @@ -131,13 +122,8 @@ impl Memory { PackageAssignments::Decision((v, _)) => { solution.insert(p.clone(), v.clone()); } - PackageAssignments::Derivations { - intersected, - not_intersected_yet, - } => { - if intersected.is_positive() - || not_intersected_yet.iter().any(|t| t.is_positive()) - { + PackageAssignments::Derivations(intersected) => { + if intersected.is_positive() { return None; } } @@ -149,20 +135,10 @@ impl Memory { impl PackageAssignments { /// Returns intersection of all assignments (decision included). - /// Mutates itself to store the intersection result. - fn assignment_intersection(&mut self) -> &Term { + fn assignment_intersection(&self) -> &Term { match self { PackageAssignments::Decision((_, term)) => term, - PackageAssignments::Derivations { - intersected, - not_intersected_yet, - } => { - for derivation in not_intersected_yet.iter() { - *intersected = intersected.intersection(&derivation); - } - not_intersected_yet.clear(); - intersected - } + PackageAssignments::Derivations(term) => term, } } @@ -171,17 +147,13 @@ impl PackageAssignments { /// and if it contains at least one positive derivation term /// in the partial solution. fn potential_package_filter<'a, P: Package>( - &'a mut self, + &'a self, package: &'a P, ) -> Option<(&'a P, &'a Range)> { match self { PackageAssignments::Decision(_) => None, - PackageAssignments::Derivations { - intersected, - not_intersected_yet, - } => { - if intersected.is_positive() || not_intersected_yet.iter().any(|t| t.is_positive()) - { + PackageAssignments::Derivations(intersected) => { + if intersected.is_positive() { Some((package, self.assignment_intersection().unwrap_positive())) } else { None diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index c4e7d747..5fc9bf3a 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -96,7 +96,7 @@ impl PartialSolution { } /// Compute, cache and retrieve the intersection of all terms for this package. - pub fn term_intersection_for_package(&mut self, package: &P) -> Option<&Term> { + pub fn term_intersection_for_package(&self, package: &P) -> Option<&Term> { self.memory.term_intersection_for_package(package) } @@ -130,7 +130,7 @@ impl PartialSolution { /// Extract potential packages for the next iteration of unit propagation. /// Return `None` if there is no suitable package anymore, which stops the algorithm. - pub fn potential_packages(&mut self) -> Option)>> { + pub fn potential_packages(&self) -> Option)>> { let mut iter = self.memory.potential_packages().peekable(); if iter.peek().is_some() { Some(iter) @@ -151,12 +151,13 @@ impl PartialSolution { new_incompatibilities: std::ops::Range>, store: &Arena>, ) { + let exact = &Term::exact(version.clone()); let not_satisfied = |incompat: &Incompatibility| { incompat.relation(|p| { if p == &package { - Some(Term::exact(version.clone())) + Some(exact) } else { - self.memory.term_intersection_for_package(p).cloned() + self.memory.term_intersection_for_package(p) } }) != Relation::Satisfied }; @@ -169,8 +170,8 @@ impl PartialSolution { } /// Check if the terms in the partial solution satisfy the incompatibility. - pub fn relation(&mut self, incompat: &Incompatibility) -> Relation

{ - incompat.relation(|package| self.memory.term_intersection_for_package(package).cloned()) + pub fn relation(&self, incompat: &Incompatibility) -> Relation

{ + incompat.relation(|package| self.memory.term_intersection_for_package(package)) } /// Find satisfier and previous satisfier decision level. From 54851df7d344529ea5fa149131078f47f340d2c6 Mon Sep 17 00:00:00 2001 From: Louis Pilfold Date: Sat, 15 May 2021 01:48:28 +0100 Subject: [PATCH 15/22] docs: correct typo (#89) --- src/solver.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/solver.rs b/src/solver.rs index 490d0ee1..ff395140 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -344,7 +344,7 @@ impl OfflineDependencyProvider { .or_default() = package_deps; } - /// Lists packages that have bean saved. + /// Lists packages that have been saved. pub fn packages(&self) -> impl Iterator { self.dependencies.keys() } From 3f2c1b9868afef968927a289b8e62cff84b5cc0a Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Sat, 22 May 2021 10:33:52 -0400 Subject: [PATCH 16/22] perf: don't reuse the same incompatibility repeatedly (#88) * perf: don't reuse the same incompatibility repeatedly * refactor: rename used_incompatibilities > contradicted_incompatibilities Co-authored-by: Matthieu Pizenberg --- src/internal/core.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/internal/core.rs b/src/internal/core.rs index 1274ecbf..99186114 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -25,6 +25,10 @@ pub struct State { incompatibilities: Map>>, + /// Store the ids of incompatibilities that are already contradicted + /// and will stay that way until the next conflict and backtrack is operated. + contradicted_incompatibilities: rustc_hash::FxHashSet>, + /// Partial solution. /// TODO: remove pub. pub partial_solution: PartialSolution, @@ -52,6 +56,7 @@ impl State { root_package, root_version, incompatibilities, + contradicted_incompatibilities: rustc_hash::FxHashSet::default(), partial_solution: PartialSolution::empty(), incompatibility_store, unit_propagation_buffer: vec![], @@ -100,6 +105,9 @@ impl State { let mut conflict_id = None; // We only care about incompatibilities if it contains the current package. for &incompat_id in self.incompatibilities[¤t_package].iter().rev() { + if self.contradicted_incompatibilities.contains(&incompat_id) { + continue; + } let current_incompat = &self.incompatibility_store[incompat_id]; match self.partial_solution.relation(current_incompat) { // If the partial solution satisfies the incompatibility @@ -116,6 +124,11 @@ impl State { incompat_id, &self.incompatibility_store, ); + // With the partial solution updated, the incompatibility is now contradicted. + self.contradicted_incompatibilities.insert(incompat_id); + } + Relation::Contradicted(_) => { + self.contradicted_incompatibilities.insert(incompat_id); } _ => {} } @@ -130,6 +143,9 @@ impl State { root_cause, &self.incompatibility_store, ); + // After conflict resolution and the partial solution update, + // the root cause incompatibility is now contradicted. + self.contradicted_incompatibilities.insert(root_cause); } } // If there are no more changed packages, unit propagation is done. @@ -200,6 +216,7 @@ impl State { ) { self.partial_solution .backtrack(decision_level, &self.incompatibility_store); + self.contradicted_incompatibilities.clear(); if incompat_changed { self.merge_incompatibility(incompat); } From 7eeee70a0c662fb2f5bf6c0c278f1e41fdcef67e Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Tue, 1 Jun 2021 20:19:36 +0200 Subject: [PATCH 17/22] perf: use smallvec for unit propagation buffer (#90) --- src/internal/core.rs | 5 ++-- src/internal/small_vec.rs | 54 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 99186114..ee6d3fe6 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -11,6 +11,7 @@ use crate::internal::assignment::Assignment::{Decision, Derivation}; use crate::internal::incompatibility::IncompId; use crate::internal::incompatibility::{Incompatibility, Relation}; use crate::internal::partial_solution::{DecisionLevel, PartialSolution}; +use crate::internal::small_vec::SmallVec; use crate::package::Package; use crate::report::DerivationTree; use crate::solver::DependencyConstraints; @@ -39,7 +40,7 @@ pub struct State { /// 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: Vec

, + unit_propagation_buffer: SmallVec

, } impl State { @@ -59,7 +60,7 @@ impl State { contradicted_incompatibilities: rustc_hash::FxHashSet::default(), partial_solution: PartialSolution::empty(), incompatibility_store, - unit_propagation_buffer: vec![], + unit_propagation_buffer: SmallVec::Empty, } } diff --git a/src/internal/small_vec.rs b/src/internal/small_vec.rs index 7d97195b..2c3fe4f4 100644 --- a/src/internal/small_vec.rs +++ b/src/internal/small_vec.rs @@ -39,6 +39,32 @@ impl SmallVec { } } + pub fn pop(&mut self) -> Option { + match std::mem::take(self) { + Self::Empty => None, + Self::One([v1]) => { + *self = Self::Empty; + Some(v1) + } + Self::Two([v1, v2]) => { + *self = Self::One([v1]); + Some(v2) + } + Self::Flexible(mut v) => { + let out = v.pop(); + *self = Self::Flexible(v); + out + } + } + } + + pub fn clear(&mut self) { + if let Self::Flexible(mut v) = std::mem::take(self) { + v.clear(); + *self = Self::Flexible(v); + } // else: self already eq Empty from the take + } + pub fn iter(&self) -> std::slice::Iter<'_, T> { self.as_slice().iter() } @@ -101,3 +127,31 @@ impl<'de, T: serde::Deserialize<'de>> serde::Deserialize<'de> for SmallVec { Ok(v) } } + +// TESTS ####################################################################### + +#[cfg(test)] +pub mod tests { + use super::*; + use proptest::prelude::*; + + proptest! { + #[test] + fn push_and_pop(comands: Vec>) { + let mut v = vec![]; + let mut sv = SmallVec::Empty; + for comand in comands { + match comand { + Some(i) => { + v.push(i); + sv.push(i); + } + None => { + assert_eq!(v.pop(), sv.pop()); + } + } + assert_eq!(v.as_slice(), sv.as_slice()); + } + } + } +} From 2459aa45e6f0e5440bed6ed82a493ff3c4922153 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Thu, 3 Jun 2021 21:03:01 +0200 Subject: [PATCH 18/22] refactor: a tiny cleanup change (#93) --- src/solver.rs | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/solver.rs b/src/solver.rs index ff395140..62015911 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -112,24 +112,19 @@ pub fn resolve( next = decision.0.clone(); // Pick the next compatible version. + let term_intersection = state + .partial_solution + .term_intersection_for_package(&next) + .expect("a package was chosen but we don't have a term."); let v = match decision.1 { None => { - let term = state - .partial_solution - .term_intersection_for_package(&next) - .expect("a package was chosen but we don't have a term.") - .clone(); - state.add_incompatibility(Incompatibility::no_versions(next.clone(), term.clone())); + let inc = Incompatibility::no_versions(next.clone(), term_intersection.clone()); + state.add_incompatibility(inc); continue; } Some(x) => x, }; - if !state - .partial_solution - .term_intersection_for_package(&next) - .expect("a package was chosen but we don't have a term.") - .contains(&v) - { + if !term_intersection.contains(&v) { return Err(PubGrubError::ErrorChoosingPackageVersion( "choose_package_version picked an incompatible version".into(), )); @@ -179,6 +174,8 @@ pub fn resolve( let dep_incompats = state.add_incompatibility_from_dependencies(p.clone(), v.clone(), &dependencies); + // TODO: I don't think this check can actually happen. + // We might want to put it under #[cfg(debug_assertions)]. if state.incompatibility_store[dep_incompats.clone()] .iter() .any(|incompat| state.is_terminal(incompat)) From cd91684f733e0e1d3e4fe558e6614844159a6919 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sun, 6 Jun 2021 02:13:50 +0200 Subject: [PATCH 19/22] refactor: unify history and memory in partial_solution (#92) * refactor: unify history and memory in partial_solution * refactor: use retain * refactor: use partition_point * perf: use more SmallVec Co-authored-by: Jacob Finkelman --- src/internal/assignment.rs | 52 ---- src/internal/core.rs | 7 +- src/internal/incompatibility.rs | 7 +- src/internal/memory.rs | 164 ---------- src/internal/mod.rs | 2 - src/internal/partial_solution.rs | 513 +++++++++++++++++++++---------- 6 files changed, 363 insertions(+), 382 deletions(-) delete mode 100644 src/internal/assignment.rs delete mode 100644 src/internal/memory.rs diff --git a/src/internal/assignment.rs b/src/internal/assignment.rs deleted file mode 100644 index c811782f..00000000 --- a/src/internal/assignment.rs +++ /dev/null @@ -1,52 +0,0 @@ -// SPDX-License-Identifier: MPL-2.0 - -//! Assignments are the building blocks of a PubGrub partial solution. -//! (partial solution = the current state of the solution we are building in the algorithm). - -use crate::internal::arena::Arena; -use crate::internal::incompatibility::IncompId; -use crate::internal::incompatibility::Incompatibility; -use crate::package::Package; -use crate::term::Term; -use crate::version::Version; - -/// An assignment is either a decision: a chosen version for a package, -/// or a derivation : a term specifying compatible versions for a package. -/// We also record the incompatibility at the origin of a derivation, called its cause. -#[derive(Clone)] -pub enum Assignment { - /// The decision. - Decision { - /// The package corresponding to the decision. - package: P, - /// The decided version. - version: V, - }, - /// The derivation. - Derivation { - /// The package corresponding to the derivation. - package: P, - /// Incompatibility cause of the derivation. - cause: IncompId, - }, -} - -impl Assignment { - /// Return the package for this assignment - pub fn package(&self) -> &P { - match self { - Self::Decision { package, .. } => package, - Self::Derivation { package, .. } => package, - } - } - - /// Retrieve the current assignment as a [Term]. - /// If this is decision, it returns a positive term with that exact version. - /// Otherwise, if this is a derivation, just returns its term. - pub fn as_term(&self, store: &Arena>) -> Term { - match &self { - Self::Decision { version, .. } => Term::exact(version.clone()), - Self::Derivation { package, cause } => store[*cause].get(&package).unwrap().negate(), - } - } -} diff --git a/src/internal/core.rs b/src/internal/core.rs index ee6d3fe6..8d9f15b2 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -7,9 +7,8 @@ use std::collections::HashSet as Set; use crate::error::PubGrubError; use crate::internal::arena::Arena; -use crate::internal::assignment::Assignment::{Decision, Derivation}; -use crate::internal::incompatibility::IncompId; -use crate::internal::incompatibility::{Incompatibility, Relation}; +use crate::internal::incompatibility::{IncompId, Incompatibility, Relation}; +use crate::internal::partial_solution::Assignment::{Decision, Derivation}; use crate::internal::partial_solution::{DecisionLevel, PartialSolution}; use crate::internal::small_vec::SmallVec; use crate::package::Package; @@ -175,7 +174,7 @@ impl State { &self.incompatibility_store[current_incompat_id], &self.incompatibility_store, ); - match satisfier.clone() { + match satisfier { Decision { package, .. } => { self.backtrack( current_incompat_id, diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index a7b403a7..acf900b8 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -54,7 +54,7 @@ enum Kind { /// A Relation describes how a set of terms can be compared to an incompatibility. /// Typically, the set of terms comes from the partial solution. -#[derive(Eq, PartialEq)] +#[derive(Eq, PartialEq, Debug)] pub enum Relation { /// We say that a set of terms S satisfies an incompatibility I /// if S satisfies every term in I. @@ -166,11 +166,6 @@ impl Incompatibility { self.package_terms.iter() } - // The number of packages. - pub fn len(&self) -> usize { - self.package_terms.len() - } - // Reporting ############################################################### /// Retrieve parent causes if of type DerivedFrom. diff --git a/src/internal/memory.rs b/src/internal/memory.rs deleted file mode 100644 index 0d43f534..00000000 --- a/src/internal/memory.rs +++ /dev/null @@ -1,164 +0,0 @@ -// SPDX-License-Identifier: MPL-2.0 - -//! A Memory acts like a structured partial solution -//! where terms are regrouped by package in a [Map](crate::type_aliases::Map). - -use crate::internal::arena::Arena; -use crate::internal::assignment::Assignment::{self, Decision, Derivation}; -use crate::internal::incompatibility::Incompatibility; -use crate::package::Package; -use crate::range::Range; -use crate::term::Term; -use crate::type_aliases::{Map, SelectedDependencies}; -use crate::version::Version; - -/// A memory is the set of all assignments in the partial solution, -/// organized by package instead of historically ordered. -/// -/// Contrary to PartialSolution, Memory does not store derivations causes, only the terms. -#[derive(Clone)] -pub struct Memory { - assignments: Map>, -} - -/// A package memory contains the potential decision and derivations -/// that have already been made for a given package. -#[derive(Clone)] -enum PackageAssignments { - Decision((V, Term)), - Derivations(Term), -} - -impl Memory { - /// Initialize an empty memory. - pub fn empty() -> Self { - Self { - assignments: Map::default(), - } - } - - /// Clears the memory. - pub fn clear(&mut self) { - self.assignments.clear(); - } - - /// Retrieve intersection of terms in memory related to package. - pub fn term_intersection_for_package(&self, package: &P) -> Option<&Term> { - self.assignments - .get(package) - .map(|pa| pa.assignment_intersection()) - } - - /// Building step of a Memory from a given assignment. - pub fn add_assignment( - &mut self, - assignment: &Assignment, - store: &Arena>, - ) { - match assignment { - Decision { package, version } => self.add_decision(package.clone(), version.clone()), - Derivation { package, cause } => self.add_derivation( - package.clone(), - store[*cause].get(&package).unwrap().negate(), - ), - } - } - - /// Add a decision to a Memory. - pub fn add_decision(&mut self, package: P, version: V) { - // Check that add_decision is never used in the wrong context. - if cfg!(debug_assertions) { - match self.assignments.get_mut(&package) { - None => panic!("Assignments must already exist"), - // Cannot be called when a decision has already been taken. - Some(PackageAssignments::Decision(_)) => panic!("Already existing decision"), - // Cannot be called if the versions is not contained in the terms intersection. - Some(pa) => debug_assert!(pa.assignment_intersection().contains(&version)), - } - } - - self.assignments.insert( - package, - PackageAssignments::Decision((version.clone(), Term::exact(version))), - ); - } - - /// Add a derivation to a Memory. - pub fn add_derivation(&mut self, package: P, term: Term) { - use std::collections::hash_map::Entry; - match self.assignments.entry(package) { - Entry::Occupied(mut o) => match o.get_mut() { - // Check that add_derivation is never called in the wrong context. - PackageAssignments::Decision(_) => debug_assert!(false), - PackageAssignments::Derivations(t) => { - *t = t.intersection(&term); - } - }, - Entry::Vacant(v) => { - v.insert(PackageAssignments::Derivations(term)); - } - } - } - - /// Extract all packages that may potentially be picked next - /// to continue solving package dependencies. - /// A package is a potential pick if there isn't an already - /// selected version (no "decision") - /// and if it contains at least one positive derivation term - /// in the partial solution. - pub fn potential_packages(&self) -> impl Iterator)> { - self.assignments - .iter() - .filter_map(|(p, pa)| pa.potential_package_filter(p)) - } - - /// 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 fn extract_solution(&self) -> Option> { - let mut solution = Map::default(); - for (p, pa) in &self.assignments { - match pa { - PackageAssignments::Decision((v, _)) => { - solution.insert(p.clone(), v.clone()); - } - PackageAssignments::Derivations(intersected) => { - if intersected.is_positive() { - return None; - } - } - } - } - Some(solution) - } -} - -impl PackageAssignments { - /// Returns intersection of all assignments (decision included). - fn assignment_intersection(&self) -> &Term { - match self { - PackageAssignments::Decision((_, term)) => term, - PackageAssignments::Derivations(term) => term, - } - } - - /// A package is a potential pick if there isn't an already - /// 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 Range)> { - match self { - PackageAssignments::Decision(_) => None, - PackageAssignments::Derivations(intersected) => { - if intersected.is_positive() { - Some((package, self.assignment_intersection().unwrap_positive())) - } else { - None - } - } - } - } -} diff --git a/src/internal/mod.rs b/src/internal/mod.rs index e8db3d46..86d7e22e 100644 --- a/src/internal/mod.rs +++ b/src/internal/mod.rs @@ -3,10 +3,8 @@ //! Non exposed modules. pub mod arena; -pub mod assignment; pub mod core; pub mod incompatibility; -pub mod memory; pub mod partial_solution; pub mod small_map; pub mod small_vec; diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 5fc9bf3a..700dc199 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -1,103 +1,201 @@ // SPDX-License-Identifier: MPL-2.0 -//! The partial solution is the current state -//! of the solution being built by the algorithm. +//! A Memory acts like a structured partial solution +//! where terms are regrouped by package in a [Map](crate::type_aliases::Map). use crate::internal::arena::Arena; -use crate::internal::assignment::Assignment::{self, Decision, Derivation}; -use crate::internal::incompatibility::IncompId; -use crate::internal::incompatibility::{Incompatibility, Relation}; -use crate::internal::memory::Memory; +use crate::internal::incompatibility::{IncompId, Incompatibility, Relation}; +use crate::internal::small_map::SmallMap; use crate::package::Package; use crate::range::Range; use crate::term::Term; use crate::type_aliases::{Map, SelectedDependencies}; use crate::version::Version; -#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] -pub struct DecisionLevel(u32); +use super::small_vec::SmallVec; -impl std::ops::Add for DecisionLevel { - type Output = DecisionLevel; +#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] +pub struct DecisionLevel(pub u32); - fn add(self, other: DecisionLevel) -> DecisionLevel { - DecisionLevel(self.0 + other.0) +impl DecisionLevel { + pub fn increment(self) -> Self { + Self(self.0 + 1) } } -impl std::ops::SubAssign for DecisionLevel { - fn sub_assign(&mut self, other: DecisionLevel) { - self.0 -= other.0 - } +/// The partial solution contains all package assignments, +/// organized by package and historically ordered. +#[derive(Clone, Debug)] +pub struct PartialSolution { + next_global_index: u32, + current_decision_level: DecisionLevel, + package_assignments: Map>, } -#[derive(Clone)] -pub struct DatedAssignment { - decision_level: DecisionLevel, - assignment: Assignment, +/// Package assignments contain the potential decision and derivations +/// that have already been made for a given package, +/// as well as the intersection of terms by all of these. +#[derive(Clone, Debug)] +struct PackageAssignments { + smallest_decision_level: DecisionLevel, + highest_decision_level: DecisionLevel, + dated_derivations: SmallVec>, + assignments_intersection: AssignmentsIntersection, } -/// The partial solution is the current state -/// of the solution being built by the algorithm. -/// It is composed of a succession of assignments, -/// defined as either decisions or derivations. -#[derive(Clone)] -pub struct PartialSolution { +#[derive(Clone, Debug)] +pub struct DatedDerivation { + global_index: u32, decision_level: DecisionLevel, - /// Each assignment is stored with its decision level in the history. - /// The order in which assignments where added in the vec is kept, - /// so the oldest assignments are at the beginning of the vec. - history: Vec>, - memory: Memory, + cause: IncompId, +} + +#[derive(Clone, Debug)] +enum AssignmentsIntersection { + Decision((u32, V, Term)), + Derivations(Term), +} + +/// An assignment is either a decision: a chosen version for a package, +/// or a derivation : a term specifying compatible versions for a package. +/// We also record the incompatibility at the origin of a derivation, called its cause. +/// TODO: Remove. I only added that to not change the code in core.rs (except reference &). +#[derive(Clone, PartialEq, Debug)] +pub enum Assignment { + /// The decision. + Decision { + /// The package corresponding to the decision. + package: P, + /// The decided version. + version: V, + }, + /// The derivation. + Derivation { + /// The package corresponding to the derivation. + package: P, + /// Incompatibility cause of the derivation. + cause: IncompId, + }, } impl PartialSolution { - /// Initialize an empty partial solution. + /// Initialize an empty PartialSolution. pub fn empty() -> Self { Self { - decision_level: DecisionLevel(0), - history: Vec::new(), - memory: Memory::empty(), + next_global_index: 0, + current_decision_level: DecisionLevel(0), + package_assignments: Map::default(), } } - /// Add a decision to the partial solution. + /// Add a decision. pub fn add_decision(&mut self, package: P, version: V) { - self.decision_level = self.decision_level + DecisionLevel(1); - self.memory.add_decision(package.clone(), version.clone()); - self.history.push(DatedAssignment { - decision_level: self.decision_level, - assignment: Decision { package, version }, - }); + // Check that add_decision is never used in the wrong context. + if cfg!(debug_assertions) { + match self.package_assignments.get_mut(&package) { + None => panic!("Derivations must already exist"), + Some(pa) => match &pa.assignments_intersection { + // Cannot be called when a decision has already been taken. + AssignmentsIntersection::Decision(_) => panic!("Already existing decision"), + // Cannot be called if the versions is not contained in the terms intersection. + AssignmentsIntersection::Derivations(term) => { + debug_assert!(term.contains(&version)) + } + }, + } + } + self.current_decision_level = self.current_decision_level.increment(); + let mut pa = self + .package_assignments + .get_mut(&package) + .expect("Derivations must already exist"); + pa.highest_decision_level = self.current_decision_level; + pa.assignments_intersection = AssignmentsIntersection::Decision(( + self.next_global_index, + version.clone(), + Term::exact(version), + )); + self.next_global_index += 1; } - /// Add a derivation to the partial solution. + /// Add a derivation. pub fn add_derivation( &mut self, package: P, cause: IncompId, store: &Arena>, ) { - self.memory.add_derivation( - package.clone(), - store[cause].get(&package).unwrap().negate(), - ); - self.history.push(DatedAssignment { - decision_level: self.decision_level, - assignment: Derivation { package, cause }, - }); + use std::collections::hash_map::Entry; + let term = store[cause].get(&package).unwrap().negate(); + let dated_derivation = DatedDerivation { + global_index: self.next_global_index, + decision_level: self.current_decision_level, + cause, + }; + self.next_global_index += 1; + match self.package_assignments.entry(package) { + Entry::Occupied(mut occupied) => { + let mut pa = occupied.get_mut(); + pa.highest_decision_level = self.current_decision_level; + match &mut pa.assignments_intersection { + // Check that add_derivation is never called in the wrong context. + AssignmentsIntersection::Decision(_) => { + panic!("add_derivation should not be called after a decision") + } + AssignmentsIntersection::Derivations(t) => { + *t = t.intersection(&term); + } + } + pa.dated_derivations.push(dated_derivation); + } + Entry::Vacant(v) => { + v.insert(PackageAssignments { + smallest_decision_level: self.current_decision_level, + highest_decision_level: self.current_decision_level, + dated_derivations: SmallVec::One([dated_derivation]), + assignments_intersection: AssignmentsIntersection::Derivations(term), + }); + } + } + } + + /// Extract potential packages for the next iteration of unit propagation. + /// Return `None` if there is no suitable package anymore, which stops the algorithm. + /// A package is a potential pick if there isn't an already + /// selected version (no "decision") + /// and if it contains at least one positive derivation term + /// in the partial solution. + pub fn potential_packages(&self) -> Option)>> { + let mut iter = self + .package_assignments + .iter() + .filter_map(|(p, pa)| pa.assignments_intersection.potential_package_filter(p)) + .peekable(); + if iter.peek().is_some() { + Some(iter) + } else { + None + } } /// 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 fn extract_solution(&self) -> Option> { - self.memory.extract_solution() - } - - /// Compute, cache and retrieve the intersection of all terms for this package. - pub fn term_intersection_for_package(&self, package: &P) -> Option<&Term> { - self.memory.term_intersection_for_package(package) + let mut solution = Map::default(); + for (p, pa) in &self.package_assignments { + match &pa.assignments_intersection { + AssignmentsIntersection::Decision((_, v, _)) => { + solution.insert(p.clone(), v.clone()); + } + AssignmentsIntersection::Derivations(term) => { + if term.is_positive() { + return None; + } + } + } + } + Some(solution) } /// Backtrack the partial solution to a given decision level. @@ -106,37 +204,44 @@ impl PartialSolution { decision_level: DecisionLevel, store: &Arena>, ) { - let pos = self - .history - .binary_search_by(|probe| { - probe - .decision_level - .cmp(&decision_level) - // `binary_search_by` does not guarantee which element to return when more - // then one match. By all ways claiming that it does not match we ensure we - // get the last one. - .then(std::cmp::Ordering::Less) - }) - .unwrap_or_else(|x| x); - - self.history.truncate(pos); - self.decision_level = self.history.last().expect("no history left").decision_level; - self.memory.clear(); - let mem = &mut self.memory; - self.history - .iter() - .for_each(|da| mem.add_assignment(&da.assignment, store)); - } + self.current_decision_level = decision_level; + 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. + true + } else { + // smallest_decision_level <= decision_level < highest_decision_level + // + // Since decision_level < highest_decision_level, + // We can be certain that there will be no decision in this package assignments + // after backtracking, because such decision would have been the last + // assignment and it would have the "highest_decision_level". - /// Extract potential packages for the next iteration of unit propagation. - /// Return `None` if there is no suitable package anymore, which stops the algorithm. - pub fn potential_packages(&self) -> Option)>> { - let mut iter = self.memory.potential_packages().peekable(); - if iter.peek().is_some() { - Some(iter) - } else { - None - } + // Truncate the history. + while pa.dated_derivations.last().map(|dd| dd.decision_level) > Some(decision_level) + { + pa.dated_derivations.pop(); + } + debug_assert!(!pa.dated_derivations.is_empty()); + + // Update highest_decision_level. + pa.highest_decision_level = pa.dated_derivations.last().unwrap().decision_level; + + // Recompute the assignments intersection. + pa.assignments_intersection = AssignmentsIntersection::Derivations( + pa.dated_derivations + .iter() + .fold(Term::any(), |acc, dated_derivation| { + let term = store[dated_derivation.cause].get(&p).unwrap().negate(); + acc.intersection(&term) + }), + ); + true + } + }); } /// We can add the version to the partial solution as a decision @@ -151,13 +256,13 @@ impl PartialSolution { new_incompatibilities: std::ops::Range>, store: &Arena>, ) { - let exact = &Term::exact(version.clone()); + let exact = Term::exact(version.clone()); let not_satisfied = |incompat: &Incompatibility| { incompat.relation(|p| { if p == &package { - Some(exact) + Some(&exact) } else { - self.memory.term_intersection_for_package(p) + self.term_intersection_for_package(p) } }) != Relation::Satisfied }; @@ -171,7 +276,14 @@ impl PartialSolution { /// Check if the terms in the partial solution satisfy the incompatibility. pub fn relation(&self, incompat: &Incompatibility) -> Relation

{ - incompat.relation(|package| self.memory.term_intersection_for_package(package)) + incompat.relation(|package| self.term_intersection_for_package(package)) + } + + /// Retrieve intersection of terms related to package. + pub fn term_intersection_for_package(&self, package: &P) -> Option<&Term> { + self.package_assignments + .get(package) + .map(|pa| pa.assignments_intersection.term()) } /// Find satisfier and previous satisfier decision level. @@ -179,25 +291,39 @@ impl PartialSolution { &self, incompat: &Incompatibility, store: &Arena>, - ) -> (&Assignment, DecisionLevel, DecisionLevel) { - let satisfier_map = Self::find_satisfier(incompat, &self.history, store); - assert_eq!( - satisfier_map.len(), - incompat.len(), - "We should always find a satisfier if called in the right context." - ); - let &satisfier_idx = satisfier_map.values().max().unwrap(); - let satisfier = &self.history[satisfier_idx]; + ) -> (Assignment, DecisionLevel, DecisionLevel) { + let satisfied_map = Self::find_satisfier(incompat, &self.package_assignments, store); + let (satisfier_package, &(satisfier_index, _, 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.assignment, - satisfier_map, - &self.history[0..=satisfier_idx], + &satisfier_package, + satisfied_map, + &self.package_assignments, store, ); + let satisfier_pa = self.package_assignments.get(&satisfier_package).unwrap(); + let satisfier_assignment = if satisfier_index == satisfier_pa.dated_derivations.len() { + match &satisfier_pa.assignments_intersection { + AssignmentsIntersection::Decision((_, version, _)) => Assignment::Decision { + package: satisfier_package, + version: version.clone(), + }, + AssignmentsIntersection::Derivations(_) => panic!("Must be a decision"), + } + } else { + let dd = &satisfier_pa.dated_derivations[satisfier_index]; + Assignment::Derivation { + package: satisfier_package, + cause: dd.cause, + } + }; ( - &satisfier.assignment, - satisfier.decision_level, + satisfier_assignment, + satisfier_decision_level, previous_satisfier_level, ) } @@ -207,42 +333,57 @@ impl PartialSolution { /// /// Returns a map indicating for each package term, when that was first satisfied in history. /// If we effectively found a satisfier, the returned map must be the same size that incompat. - fn find_satisfier<'a>( + /// + /// Question: This is possible since we added a "global_index" to every dated_derivation. + /// 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. + fn find_satisfier( incompat: &Incompatibility, - history: &'a [DatedAssignment], + package_assignments: &Map>, store: &Arena>, - ) -> Map { - let mut accum_satisfied: Map> = incompat - .iter() - .map(|(p, _)| (p.clone(), Term::any())) - .collect(); - let mut satisfied = Map::with_capacity_and_hasher(incompat.len(), Default::default()); - for (idx, dated_assignment) in history.iter().enumerate() { - let package = dated_assignment.assignment.package(); - if satisfied.contains_key(package) { - continue; // If that package term is already satisfied, no need to check. - } - let incompat_term = match incompat.get(package) { - // We only care about packages related to the incompatibility. - None => continue, - Some(i) => i, - }; - let accum_term = match accum_satisfied.get_mut(package) { - // We only care about packages related to the accum_satisfied. - None => continue, - Some(i) => i, - }; - - // Check if that incompat term is satisfied by our accumulated terms intersection. - *accum_term = accum_term.intersection(&dated_assignment.assignment.as_term(store)); - // Check if we have found the satisfier - // (that all terms are satisfied). - if accum_term.subset_of(incompat_term) { - satisfied.insert(package.clone(), idx); - if satisfied.len() == incompat.len() { + ) -> SmallMap { + let mut satisfied = SmallMap::Empty; + for (package, incompat_term) in incompat.iter() { + let pa = package_assignments.get(package).expect("Must exist"); + // Term where we accumulate intersections until incompat_term is satisfied. + let mut accum_term = Term::any(); + // Indicate if we found a satisfier in the list of derivations, otherwise it will be the decision. + let mut derivation_satisfier_index = None; + for (idx, dated_derivation) in pa.dated_derivations.iter().enumerate() { + let this_term = store[dated_derivation.cause].get(package).unwrap().negate(); + accum_term = accum_term.intersection(&this_term); + if accum_term.subset_of(incompat_term) { + // We found the derivation causing satisfaction. + derivation_satisfier_index = Some(( + idx, + dated_derivation.global_index, + dated_derivation.decision_level, + )); break; } } + match derivation_satisfier_index { + Some(indices) => { + satisfied.insert(package.clone(), indices); + } + // If it wasn't found in the derivations, + // it must be the decision which is last (if called in the right context). + None => match pa.assignments_intersection { + AssignmentsIntersection::Decision((global_index, _, _)) => { + satisfied.insert( + package.clone(), + ( + pa.dated_derivations.len(), + global_index, + pa.highest_decision_level, + ), + ); + } + AssignmentsIntersection::Derivations(_) => { + panic!("This must be a decision") + } + }, + }; } satisfied } @@ -250,32 +391,96 @@ impl PartialSolution { /// Earliest assignment in the partial solution before satisfier /// such that incompatibility is satisfied by the partial solution up to /// and including that assignment plus satisfier. - fn find_previous_satisfier<'a>( + fn find_previous_satisfier( incompat: &Incompatibility, - satisfier: &Assignment, - mut satisfier_map: Map, - previous_assignments: &'a [DatedAssignment], + satisfier_package: &P, + mut satisfied_map: SmallMap, + package_assignments: &Map>, store: &Arena>, ) -> DecisionLevel { - let package = satisfier.package().clone(); - let mut accum_term = satisfier.as_term(store); - let incompat_term = incompat.get(&package).expect("package not in satisfier"); - // Search previous satisfier. - for (idx, dated_assignment) in previous_assignments.iter().enumerate() { - if dated_assignment.assignment.package() != &package { - // We only care about packages related to the incompatibility. - continue; - } + // First, let's retrieve the previous derivations and the initial accum_term. + let satisfier_pa = package_assignments.get(satisfier_package).unwrap(); + let (satisfier_index, ref mut _satisfier_global_idx, ref mut _satisfier_decision_level) = + satisfied_map.get_mut(satisfier_package).unwrap(); + // *satisfier_global_idx = 0; // Changes behavior + // *satisfier_decision_level = DecisionLevel(0); // Changes behavior + + let (previous_derivations, mut accum_term) = + if *satisfier_index == satisfier_pa.dated_derivations.len() { + match &satisfier_pa.assignments_intersection { + AssignmentsIntersection::Derivations(_) => panic!("must be a decision"), + AssignmentsIntersection::Decision((_, _, term)) => { + (satisfier_pa.dated_derivations.as_slice(), term.clone()) + } + } + } else { + let dd = &satisfier_pa.dated_derivations[*satisfier_index]; + ( + &satisfier_pa.dated_derivations[..=*satisfier_index], // [..satisfier_idx] to change behavior + store[dd.cause].get(satisfier_package).unwrap().negate(), + ) + }; + + let incompat_term = incompat + .get(satisfier_package) + .expect("satisfier package not in incompat"); + + for (idx, dated_derivation) in previous_derivations.iter().enumerate() { // Check if that incompat term is satisfied by our accumulated terms intersection. - accum_term = accum_term.intersection(&dated_assignment.assignment.as_term(store)); + let this_term = store[dated_derivation.cause] + .get(satisfier_package) + .unwrap() + .negate(); + accum_term = accum_term.intersection(&this_term); // Check if we have found the satisfier if accum_term.subset_of(incompat_term) { - satisfier_map.insert(package.clone(), idx); + satisfied_map.insert( + satisfier_package.clone(), + ( + idx, + dated_derivation.global_index, + dated_derivation.decision_level, + ), + ); break; } } - previous_assignments[*satisfier_map.values().max().unwrap()] - .decision_level - .max(DecisionLevel(1)) + + // Finally, let's identify the decision level of that previous satisfier. + let (_, &(_, _, decision_level)) = satisfied_map + .iter() + .max_by_key(|(_p, (_, global_index, _))| global_index) + .unwrap(); + decision_level.max(DecisionLevel(1)) + } +} + +impl AssignmentsIntersection { + /// Returns the term intersection of all assignments (decision included). + fn term(&self) -> &Term { + match self { + Self::Decision((_, _, term)) => term, + Self::Derivations(term) => term, + } + } + + /// A package is a potential pick if there isn't an already + /// 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 Range)> { + match self { + Self::Decision(_) => None, + Self::Derivations(term_intersection) => { + if term_intersection.is_positive() { + Some((package, term_intersection.unwrap_positive())) + } else { + None + } + } + } } } From 19312b521ba8442ecffc76cd6cd73bf875999270 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Sun, 13 Jun 2021 19:01:58 +0200 Subject: [PATCH 20/22] refactor: satisfier search result cleanup (#96) --- src/internal/core.rs | 47 +++++++++++-------------- src/internal/partial_solution.rs | 60 +++++++++++--------------------- 2 files changed, 40 insertions(+), 67 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 8d9f15b2..f923850a 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -8,7 +8,9 @@ use std::collections::HashSet as Set; use crate::error::PubGrubError; use crate::internal::arena::Arena; use crate::internal::incompatibility::{IncompId, Incompatibility, Relation}; -use crate::internal::partial_solution::Assignment::{Decision, Derivation}; +use crate::internal::partial_solution::SatisfierSearch::{ + DifferentDecisionLevels, SameDecisionLevels, +}; use crate::internal::partial_solution::{DecisionLevel, PartialSolution}; use crate::internal::small_vec::SmallVec; use crate::package::Package; @@ -168,14 +170,14 @@ impl State { self.build_derivation_tree(current_incompat_id), )); } else { - let (satisfier, satisfier_level, previous_satisfier_level) = self - .partial_solution - .find_satisfier_and_previous_satisfier_level( - &self.incompatibility_store[current_incompat_id], - &self.incompatibility_store, - ); - match satisfier { - Decision { package, .. } => { + let (package, satisfier_search_result) = self.partial_solution.satisfier_search( + &self.incompatibility_store[current_incompat_id], + &self.incompatibility_store, + ); + match satisfier_search_result { + DifferentDecisionLevels { + previous_satisfier_level, + } => { self.backtrack( current_incompat_id, current_incompat_changed, @@ -183,24 +185,15 @@ impl State { ); return Ok((package, current_incompat_id)); } - Derivation { cause, package } => { - if previous_satisfier_level != satisfier_level { - self.backtrack( - current_incompat_id, - current_incompat_changed, - previous_satisfier_level, - ); - return Ok((package, current_incompat_id)); - } else { - let prior_cause = Incompatibility::prior_cause( - current_incompat_id, - cause, - &package, - &self.incompatibility_store, - ); - current_incompat_id = self.incompatibility_store.alloc(prior_cause); - current_incompat_changed = true; - } + SameDecisionLevels { satisfier_cause } => { + let prior_cause = Incompatibility::prior_cause( + current_incompat_id, + satisfier_cause, + &package, + &self.incompatibility_store, + ); + current_incompat_id = self.incompatibility_store.alloc(prior_cause); + current_incompat_changed = true; } } } diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 700dc199..787d207e 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -56,25 +56,13 @@ enum AssignmentsIntersection { Derivations(Term), } -/// An assignment is either a decision: a chosen version for a package, -/// or a derivation : a term specifying compatible versions for a package. -/// We also record the incompatibility at the origin of a derivation, called its cause. -/// TODO: Remove. I only added that to not change the code in core.rs (except reference &). -#[derive(Clone, PartialEq, Debug)] -pub enum Assignment { - /// The decision. - Decision { - /// The package corresponding to the decision. - package: P, - /// The decided version. - version: V, +#[derive(Clone, Debug)] +pub enum SatisfierSearch { + DifferentDecisionLevels { + previous_satisfier_level: DecisionLevel, }, - /// The derivation. - Derivation { - /// The package corresponding to the derivation. - package: P, - /// Incompatibility cause of the derivation. - cause: IncompId, + SameDecisionLevels { + satisfier_cause: IncompId, }, } @@ -286,12 +274,12 @@ impl PartialSolution { .map(|pa| pa.assignments_intersection.term()) } - /// Find satisfier and previous satisfier decision level. - pub fn find_satisfier_and_previous_satisfier_level( + /// Figure out if the satisfier and previous satisfier are of different decision levels. + pub fn satisfier_search( &self, incompat: &Incompatibility, store: &Arena>, - ) -> (Assignment, DecisionLevel, DecisionLevel) { + ) -> (P, SatisfierSearch) { let satisfied_map = Self::find_satisfier(incompat, &self.package_assignments, store); let (satisfier_package, &(satisfier_index, _, satisfier_decision_level)) = satisfied_map .iter() @@ -305,27 +293,19 @@ impl PartialSolution { &self.package_assignments, store, ); - let satisfier_pa = self.package_assignments.get(&satisfier_package).unwrap(); - let satisfier_assignment = if satisfier_index == satisfier_pa.dated_derivations.len() { - match &satisfier_pa.assignments_intersection { - AssignmentsIntersection::Decision((_, version, _)) => Assignment::Decision { - package: satisfier_package, - version: version.clone(), - }, - AssignmentsIntersection::Derivations(_) => panic!("Must be a decision"), - } + if previous_satisfier_level < satisfier_decision_level { + let search_result = SatisfierSearch::DifferentDecisionLevels { + previous_satisfier_level, + }; + (satisfier_package, search_result) } else { + let satisfier_pa = self.package_assignments.get(&satisfier_package).unwrap(); let dd = &satisfier_pa.dated_derivations[satisfier_index]; - Assignment::Derivation { - package: satisfier_package, - cause: dd.cause, - } - }; - ( - satisfier_assignment, - satisfier_decision_level, - previous_satisfier_level, - ) + let search_result = SatisfierSearch::SameDecisionLevels { + satisfier_cause: dd.cause, + }; + (satisfier_package, search_result) + } } /// A satisfier is the earliest assignment in partial solution such that the incompatibility From 2ca8446740bc119c2add8862f6f2d3ff33c4e69b Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 14 Jun 2021 11:40:16 -0400 Subject: [PATCH 21/22] refactor: satisfier search in a helper method (#97) --- src/internal/partial_solution.rs | 136 +++++++++++++------------------ 1 file changed, 57 insertions(+), 79 deletions(-) diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 787d207e..ad454244 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -325,45 +325,10 @@ impl PartialSolution { let mut satisfied = SmallMap::Empty; for (package, incompat_term) in incompat.iter() { let pa = package_assignments.get(package).expect("Must exist"); - // Term where we accumulate intersections until incompat_term is satisfied. - let mut accum_term = Term::any(); - // Indicate if we found a satisfier in the list of derivations, otherwise it will be the decision. - let mut derivation_satisfier_index = None; - for (idx, dated_derivation) in pa.dated_derivations.iter().enumerate() { - let this_term = store[dated_derivation.cause].get(package).unwrap().negate(); - accum_term = accum_term.intersection(&this_term); - if accum_term.subset_of(incompat_term) { - // We found the derivation causing satisfaction. - derivation_satisfier_index = Some(( - idx, - dated_derivation.global_index, - dated_derivation.decision_level, - )); - break; - } - } - match derivation_satisfier_index { - Some(indices) => { - satisfied.insert(package.clone(), indices); - } - // If it wasn't found in the derivations, - // it must be the decision which is last (if called in the right context). - None => match pa.assignments_intersection { - AssignmentsIntersection::Decision((global_index, _, _)) => { - satisfied.insert( - package.clone(), - ( - pa.dated_derivations.len(), - global_index, - pa.highest_decision_level, - ), - ); - } - AssignmentsIntersection::Derivations(_) => { - panic!("This must be a decision") - } - }, - }; + satisfied.insert( + package.clone(), + pa.satisfier(package, incompat_term, Term::any(), store), + ); } satisfied } @@ -380,51 +345,26 @@ impl PartialSolution { ) -> 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, ref mut _satisfier_global_idx, ref mut _satisfier_decision_level) = - satisfied_map.get_mut(satisfier_package).unwrap(); - // *satisfier_global_idx = 0; // Changes behavior - // *satisfier_decision_level = DecisionLevel(0); // Changes behavior - - let (previous_derivations, mut accum_term) = - if *satisfier_index == satisfier_pa.dated_derivations.len() { - match &satisfier_pa.assignments_intersection { - AssignmentsIntersection::Derivations(_) => panic!("must be a decision"), - AssignmentsIntersection::Decision((_, _, term)) => { - (satisfier_pa.dated_derivations.as_slice(), term.clone()) - } - } - } else { - let dd = &satisfier_pa.dated_derivations[*satisfier_index]; - ( - &satisfier_pa.dated_derivations[..=*satisfier_index], // [..satisfier_idx] to change behavior - store[dd.cause].get(satisfier_package).unwrap().negate(), - ) - }; + let (satisfier_index, _gidx, _dl) = satisfied_map.get_mut(satisfier_package).unwrap(); + + let accum_term = if *satisfier_index == satisfier_pa.dated_derivations.len() { + 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 .get(satisfier_package) .expect("satisfier package not in incompat"); - for (idx, dated_derivation) in previous_derivations.iter().enumerate() { - // Check if that incompat term is satisfied by our accumulated terms intersection. - let this_term = store[dated_derivation.cause] - .get(satisfier_package) - .unwrap() - .negate(); - accum_term = accum_term.intersection(&this_term); - // Check if we have found the satisfier - if accum_term.subset_of(incompat_term) { - satisfied_map.insert( - satisfier_package.clone(), - ( - idx, - dated_derivation.global_index, - dated_derivation.decision_level, - ), - ); - break; - } - } + satisfied_map.insert( + satisfier_package.clone(), + satisfier_pa.satisfier(satisfier_package, incompat_term, accum_term, store), + ); // Finally, let's identify the decision level of that previous satisfier. let (_, &(_, _, decision_level)) = satisfied_map @@ -435,6 +375,44 @@ impl PartialSolution { } } +impl PackageAssignments { + fn satisfier( + &self, + package: &P, + incompat_term: &Term, + start_term: Term, + store: &Arena>, + ) -> (usize, u32, DecisionLevel) { + // Term where we accumulate intersections until incompat_term is satisfied. + let mut accum_term = start_term; + // 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() { + let this_term = store[dated_derivation.cause].get(package).unwrap().negate(); + accum_term = accum_term.intersection(&this_term); + if accum_term.subset_of(incompat_term) { + // We found the derivation causing satisfaction. + return ( + idx, + dated_derivation.global_index, + dated_derivation.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::Derivations(_) => { + panic!("This must be a decision") + } + } + } +} + impl AssignmentsIntersection { /// Returns the term intersection of all assignments (decision included). fn term(&self) -> &Term { From 57027b38a7a5e5bad2666d54e9e93e963fc7dca7 Mon Sep 17 00:00:00 2001 From: Matthieu Pizenberg Date: Wed, 30 Jun 2021 22:30:03 +0200 Subject: [PATCH 22/22] release: prepare the 0.2.1 release (#98) * docs: update version in Cargo.toml to 0.2.1 * docs: update changelog to 0.2.1 * refactor: make clippy happy * docs: put the speed improvements in changelog * docs: update release date of 0.2.1 --- CHANGELOG.md | 33 ++++++++++++++++++++++++++++++++- Cargo.toml | 2 +- src/report.rs | 3 +-- 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2cb50a9..51f58e14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,34 @@ All notable changes to this project will be documented in this file. ## Unreleased [(diff)][unreleased-diff] +## [0.2.1] - 2021-06-30 - [(diff with 0.2.0)][0.2.0-diff] + +This release is focused on performance improvements and code readability, without any change to the public API. + +The code tends to be simpler around tricky parts of the algorithm such as conflict resolution. +Some data structures have been rewritten (with no unsafe) to lower memory usage. +Depending on scenarios, version 0.2.1 is 3 to 8 times faster than 0.2.0. +As an example, solving all elm package versions existing went from 580ms to 175ms on my laptop. +While solving a specific subset of packages from crates.io went from 2.5s to 320ms on my laptop. + +Below are listed all the important changes in the internal parts of the API. + +#### Added + +- New `SmallVec` data structure (with no unsafe) using fixed size arrays for up to 2 entries. +- New `SmallMap` data structure (with no unsafe) using fixed size arrays for up to 2 entries. +- New `Arena` data structure (with no unsafe) backed by a `Vec` and indexed with `Id` where `T` is phantom data. + +#### Changed + +- Updated the `large_case` benchmark to run with both u16 and string package identifiers in registries. +- Use the new `Arena` for the incompatibility store, and use its `Id` identifiers to reference incompatibilities instead of full owned copies in the `incompatibilities` field of the solver `State`. +- Save satisfier indices of each package involved in an incompatibility when looking for its satisfier. This speeds up the search for the previous satisfier. +- Early unit propagation loop restart at the first conflict found instead of continuing evaluation for the current package. +- Index incompatibilities by package in a hash map instead of using a vec. +- Keep track of already contradicted incompatibilities in a `Set` until the next backtrack to speed up unit propagation. +- Unify `history` and `memory` in `partial_solution` under a unique hash map indexed by packages. This should speed up access to relevan terms in conflict resolution. + ## [0.2.0] - 2020-11-19 - [(diff with 0.1.0)][0.1.0-diff] This release brings many important improvements to PubGrub. @@ -133,7 +161,10 @@ The gist of it is: - `.gitignore` configured for a Rust project. - `.github/workflows/` CI to automatically build, test and document on push and pull requests. +[0.2.1]: https://github.com/pubgrub-rs/pubgrub/releases/tag/v0.2.1 [0.2.0]: https://github.com/pubgrub-rs/pubgrub/releases/tag/v0.2.0 -[0.1.0-diff]: https://github.com/pubgrub-rs/pubgrub/compare/v0.1.0...v0.2.0 [0.1.0]: https://github.com/pubgrub-rs/pubgrub/releases/tag/v0.1.0 + [unreleased-diff]: https://github.com/pubgrub-rs/pubgrub/compare/release...dev +[0.2.0-diff]: https://github.com/pubgrub-rs/pubgrub/compare/v0.2.0...v0.2.1 +[0.1.0-diff]: https://github.com/pubgrub-rs/pubgrub/compare/v0.1.0...v0.2.0 diff --git a/Cargo.toml b/Cargo.toml index 544ac050..1f6b32fb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ [package] name = "pubgrub" -version = "0.2.0" +version = "0.2.1" authors = [ "Matthieu Pizenberg ", "Alex Tokarev ", diff --git a/src/report.rs b/src/report.rs index 09815821..07dec364 100644 --- a/src/report.rs +++ b/src/report.rs @@ -263,12 +263,11 @@ impl DefaultStringReporter { // recursively call on the second node, // and finally conclude. (None, None) => { + self.build_recursive(derived1); if derived1.shared_id != None { - self.build_recursive(derived1); self.lines.push("".into()); self.build_recursive(current); } else { - self.build_recursive(derived1); self.add_line_ref(); let ref1 = self.ref_count; self.lines.push("".into());