Create an arena for package names

Author: Eh2406

benches/large_case.rs

@@ -4,11 +4,11 @@ use std::time::Duration;
 extern crate criterion;
 use self::criterion::*;
 
-use pubgrub::package::Package;
-use pubgrub::range::Range;
-use pubgrub::solver::{resolve, OfflineDependencyProvider};
-use pubgrub::version::SemanticVersion;
-use pubgrub::version_set::VersionSet;
+use pubgrub::Package;
+use pubgrub::Range;
+use pubgrub::SemanticVersion;
+use pubgrub::VersionSet;
+use pubgrub::{resolve, OfflineDependencyProvider};
 use serde::de::Deserialize;
 
 fn bench<'a, P: Package + Deserialize<'a>, VS: VersionSet + Deserialize<'a>>(

src/internal/arena.rs

@@ -1,8 +1,10 @@
 use std::fmt;
-use std::hash::{Hash, Hasher};
+use std::hash::{BuildHasherDefault, Hash, Hasher};
 use std::marker::PhantomData;
 use std::ops::{Index, Range};
 
+type FnvIndexSet<K> = indexmap::IndexSet<K, BuildHasherDefault<rustc_hash::FxHasher>>;
+
 /// The index of a value allocated in an arena that holds `T`s.
 ///
 /// The Clone, Copy and other traits are defined manually because
@@ -124,3 +126,44 @@ impl<T> Index<Range<Id<T>>> for Arena<T> {
         &self.data[(id.start.raw as usize)..(id.end.raw as usize)]
     }
 }
+
+/// Yet another index-based arena. This one de-duplicates entries by hashing.
+///
+/// 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.
+/// In this case the `Vec` is inside a `IndexSet` allowing fast lookup by value not just index.
+/// They are all dropped at once when the arena is dropped.
+#[derive(Clone, PartialEq, Eq)]
+pub struct HashArena<T: Hash + Eq> {
+    data: FnvIndexSet<T>,
+}
+
+impl<T: Hash + Eq + fmt::Debug> fmt::Debug for HashArena<T> {
+    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
+        fmt.debug_struct("Arena")
+            .field("len", &self.data.len())
+            .field("data", &self.data)
+            .finish()
+    }
+}
+
+impl<T: Hash + Eq> HashArena<T> {
+    pub fn new() -> Self {
+        HashArena {
+            data: FnvIndexSet::default(),
+        }
+    }
+
+    pub fn alloc(&mut self, value: T) -> Id<T> {
+        let (raw, _) = self.data.insert_full(value);
+        Id::from(raw as u32)
+    }
+}
+
+impl<T: Hash + Eq> Index<Id<T>> for HashArena<T> {
+    type Output = T;
+    fn index(&self, id: Id<T>) -> &T {
+        &self.data[id.raw as usize]
+    }
+}

src/internal/core.rs

@@ -19,14 +19,16 @@ use crate::solver::DependencyProvider;
 use crate::type_aliases::{IncompDpId, Map};
 use crate::version_set::VersionSet;
 
+use super::arena::{HashArena, Id};
+
 /// Current state of the PubGrub algorithm.
 #[derive(Clone)]
 pub struct State<DP: DependencyProvider> {
-    root_package: DP::P,
+    pub root_package: Id<DP::P>,
     root_version: DP::V,
 
     #[allow(clippy::type_complexity)]
-    incompatibilities: Map<DP::P, Vec<IncompDpId<DP>>>,
+    incompatibilities: Map<Id<DP::P>, Vec<IncompDpId<DP>>>,
 
     /// Store the ids of incompatibilities that are already contradicted.
     /// For each one keep track of the decision level when it was found to be contradicted.
@@ -36,7 +38,7 @@ pub struct State<DP: DependencyProvider> {
     /// All incompatibilities expressing dependencies,
     /// with common dependents merged.
     #[allow(clippy::type_complexity)]
-    merged_dependencies: Map<(DP::P, DP::P), SmallVec<IncompDpId<DP>>>,
+    merged_dependencies: Map<(Id<DP::P>, Id<DP::P>), SmallVec<IncompDpId<DP>>>,
 
     /// Partial solution.
     /// TODO: remove pub.
@@ -45,29 +47,35 @@ pub struct State<DP: DependencyProvider> {
     /// The store is the reference storage for all incompatibilities.
     pub incompatibility_store: Arena<Incompatibility<DP::P, DP::VS, DP::M>>,
 
+    /// The store is the reference storage for all incompatibilities.
+    pub package_store: HashArena<DP::P>,
+
     /// 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: SmallVec<DP::P>,
+    unit_propagation_buffer: SmallVec<Id<DP::P>>,
 }
 
 impl<DP: DependencyProvider> State<DP> {
     /// Initialization of PubGrub state.
     pub fn init(root_package: DP::P, root_version: DP::V) -> Self {
         let mut incompatibility_store = Arena::new();
+        let mut package_store = HashArena::new();
+        let root_package = package_store.alloc(root_package);
         let not_root_id = incompatibility_store.alloc(Incompatibility::not_root(
-            root_package.clone(),
+            root_package,
             root_version.clone(),
         ));
         let mut incompatibilities = Map::default();
-        incompatibilities.insert(root_package.clone(), vec![not_root_id]);
+        incompatibilities.insert(root_package, vec![not_root_id]);
         Self {
             root_package,
             root_version,
             incompatibilities,
             contradicted_incompatibilities: Map::default(),
             partial_solution: PartialSolution::empty(),
             incompatibility_store,
+            package_store,
             unit_propagation_buffer: SmallVec::Empty,
             merged_dependencies: Map::default(),
         }
@@ -82,18 +90,19 @@ impl<DP: DependencyProvider> State<DP> {
     /// Add an incompatibility to the state.
     pub fn add_incompatibility_from_dependencies(
         &mut self,
-        package: DP::P,
+        package: Id<DP::P>,
         version: DP::V,
         deps: impl IntoIterator<Item = (DP::P, DP::VS)>,
     ) -> std::ops::Range<IncompDpId<DP>> {
         // Create incompatibilities and allocate them in the store.
         let new_incompats_id_range =
             self.incompatibility_store
-                .alloc_iter(deps.into_iter().map(|dep| {
+                .alloc_iter(deps.into_iter().map(|(dep_p, dep_vs)| {
+                    let dep_pid = self.package_store.alloc(dep_p);
                     Incompatibility::from_dependency(
-                        package.clone(),
+                        package,
                         <DP::VS as VersionSet>::singleton(version.clone()),
-                        dep,
+                        (dep_pid, dep_vs),
                     )
                 }));
         // Merge the newly created incompatibilities with the older ones.
@@ -105,7 +114,7 @@ impl<DP: DependencyProvider> State<DP> {
 
     /// Unit propagation is the core mechanism of the solving algorithm.
     /// CF <https://github.com/dart-lang/pub/blob/master/doc/solver.md#unit-propagation>
-    pub fn unit_propagation(&mut self, package: DP::P) -> Result<(), NoSolutionError<DP>> {
+    pub fn unit_propagation(&mut self, package: Id<DP::P>) -> Result<(), NoSolutionError<DP>> {
         self.unit_propagation_buffer.clear();
         self.unit_propagation_buffer.push(package);
         while let Some(current_package) = self.unit_propagation_buffer.pop() {
@@ -126,7 +135,7 @@ impl<DP: DependencyProvider> State<DP> {
                     // we must perform conflict resolution.
                     Relation::Satisfied => {
                         log::info!(
-                            "Start conflict resolution because incompat satisfied:\n   {}",
+                            "Start conflict resolution because incompat satisfied:\n   {:?}",
                             current_incompat
                         );
                         conflict_id = Some(incompat_id);
@@ -139,7 +148,7 @@ impl<DP: DependencyProvider> State<DP> {
                         // In practice the most common pathology is adding the same package repeatedly.
                         // So we only check if it is duplicated with the last item.
                         if self.unit_propagation_buffer.last() != Some(&package_almost) {
-                            self.unit_propagation_buffer.push(package_almost.clone());
+                            self.unit_propagation_buffer.push(package_almost);
                         }
                         // Add (not term) to the partial solution with incompat as cause.
                         self.partial_solution.add_derivation(
@@ -165,7 +174,7 @@ impl<DP: DependencyProvider> State<DP> {
                             self.build_derivation_tree(terminal_incompat_id)
                         })?;
                 self.unit_propagation_buffer.clear();
-                self.unit_propagation_buffer.push(package_almost.clone());
+                self.unit_propagation_buffer.push(package_almost);
                 // Add to the partial solution with incompat as cause.
                 self.partial_solution.add_derivation(
                     package_almost,
@@ -188,12 +197,12 @@ impl<DP: DependencyProvider> State<DP> {
     fn conflict_resolution(
         &mut self,
         incompatibility: IncompDpId<DP>,
-    ) -> Result<(DP::P, IncompDpId<DP>), IncompDpId<DP>> {
+    ) -> Result<(Id<DP::P>, IncompDpId<DP>), IncompDpId<DP>> {
         let mut current_incompat_id = incompatibility;
         let mut current_incompat_changed = false;
         loop {
             if self.incompatibility_store[current_incompat_id]
-                .is_terminal(&self.root_package, &self.root_version)
+                .is_terminal(self.root_package, &self.root_version)
             {
                 return Err(current_incompat_id);
             } else {
@@ -205,7 +214,6 @@ impl<DP: DependencyProvider> State<DP> {
                     DifferentDecisionLevels {
                         previous_satisfier_level,
                     } => {
-                        let package = package.clone();
                         self.backtrack(
                             current_incompat_id,
                             current_incompat_changed,
@@ -221,7 +229,7 @@ impl<DP: DependencyProvider> State<DP> {
                             package,
                             &self.incompatibility_store,
                         );
-                        log::info!("prior cause: {}", prior_cause);
+                        log::info!("prior cause: {:?}", prior_cause);
                         current_incompat_id = self.incompatibility_store.alloc(prior_cause);
                         current_incompat_changed = true;
                     }
@@ -264,19 +272,16 @@ impl<DP: DependencyProvider> State<DP> {
     fn merge_incompatibility(&mut self, mut id: IncompDpId<DP>) {
         if let Some((p1, p2)) = self.incompatibility_store[id].as_dependency() {
             // If we are a dependency, there's a good chance we can be merged with a previous dependency
-            let deps_lookup = self
-                .merged_dependencies
-                .entry((p1.clone(), p2.clone()))
-                .or_default();
+            let deps_lookup = self.merged_dependencies.entry((p1, p2)).or_default();
             if let Some((past, merged)) = deps_lookup.as_mut_slice().iter_mut().find_map(|past| {
                 self.incompatibility_store[id]
                     .merge_dependents(&self.incompatibility_store[*past])
                     .map(|m| (past, m))
             }) {
                 let new = self.incompatibility_store.alloc(merged);
-                for (pkg, _) in self.incompatibility_store[new].iter() {
+                for (&pkg, _) in self.incompatibility_store[new].iter() {
                     self.incompatibilities
-                        .entry(pkg.clone())
+                        .entry(pkg)
                         .or_default()
                         .retain(|id| id != past);
                 }
@@ -286,14 +291,11 @@ impl<DP: DependencyProvider> State<DP> {
                 deps_lookup.push(id);
             }
         }
-        for (pkg, term) in self.incompatibility_store[id].iter() {
+        for (&pkg, term) in self.incompatibility_store[id].iter() {
             if cfg!(debug_assertions) {
                 assert_ne!(term, &crate::term::Term::any());
             }
-            self.incompatibilities
-                .entry(pkg.clone())
-                .or_default()
-                .push(id);
+            self.incompatibilities.entry(pkg).or_default().push(id);
         }
     }
 
@@ -328,6 +330,7 @@ impl<DP: DependencyProvider> State<DP> {
                 id,
                 &shared_ids,
                 &self.incompatibility_store,
+                &self.package_store,
                 &precomputed,
             );
             precomputed.insert(id, Arc::new(tree));