Relax dependencies to iterator instead of a hashmap to avoid clones

Previously, `Dependencies::Available` was hardcoded to a hash map. By relaxing this to `impl IntoIterator<_> + Clone`, we avoid converting from uv's ordered `Vec<(_, _)>` to a `HashMap<_, _>` and avoid cloning. ## Design considerations We implement this using the return type `Dependencies<impl IntoIterator<Item = (DP::P, DP::VS)> + Clone, DP::M>`. This allows using `get_dependencies` without knowing the actual (which a generic bound would require) or adding another associated type to the dependency provider. The `impl` bound also captures all input lifetimes, keeping `p` and `v` borrowed. We could bind the iterator to only `&self` instead, but this would only move two clones (package and version) to the implementer. Co-authored-by: Jacob Finkelman <[email protected]> Co-authored-by: Zanie <[email protected]> Co-authored-by: Charlie Marsh <[email protected]>
pubgrub-rs · Jun 2, 2024 · 0fba094 · 0fba094
1 parent 1c05803
commit 0fba094
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 52 deletions.
diff --git a/examples/caching_dependency_provider.rs b/examples/caching_dependency_provider.rs
@@ -29,27 +29,37 @@ impl<DP: DependencyProvider<M = String>> DependencyProvider for CachingDependenc
         &self,
         package: &DP::P,
         version: &DP::V,
-    ) -> Result<Dependencies<DP::P, DP::VS, DP::M>, DP::Err> {
+    ) -> Result<Dependencies<impl IntoIterator<Item = (DP::P, DP::VS)> + Clone, Self::M>, DP::Err>
+    {
         let mut cache = self.cached_dependencies.borrow_mut();
         match cache.get_dependencies(package, version) {
             Ok(Dependencies::Unavailable(_)) => {
-                let dependencies = self.remote_dependencies.get_dependencies(package, version);
-                match dependencies {
-                    Ok(Dependencies::Available(dependencies)) => {
-                        cache.add_dependencies(
-                            package.clone(),
-                            version.clone(),
-                            dependencies.clone(),
-                        );
-                        Ok(Dependencies::Available(dependencies))
-                    }
-                    Ok(Dependencies::Unavailable(reason)) => Ok(Dependencies::Unavailable(reason)),
-                    error @ Err(_) => error,
-                }
+                // Code below to end the borrow.
+            }
+            Ok(dependencies) => {
+                return Ok(match dependencies {
+                    Dependencies::Unavailable(reason) => Dependencies::Unavailable(reason),
+                    Dependencies::Available(available) => Dependencies::Available(
+                        available.into_iter().collect::<Vec<(Self::P, Self::VS)>>(),
+                    ),
+                })
             }
-            Ok(dependencies) => Ok(dependencies),
             Err(_) => unreachable!(),
         }
+        let dependencies = self.remote_dependencies.get_dependencies(package, version);
+        match dependencies {
+            Ok(Dependencies::Available(dependencies)) => {
+                cache.add_dependencies(package.clone(), version.clone(), dependencies.clone());
+                Ok(Dependencies::Available(
+                    dependencies
+                        .clone()
+                        .into_iter()
+                        .collect::<Vec<(Self::P, Self::VS)>>(),
+                ))
+            }
+            Ok(Dependencies::Unavailable(reason)) => Ok(Dependencies::Unavailable(reason)),
+            Err(err) => Err(err),
+        }
     }
 
     fn choose_version(&self, package: &DP::P, range: &DP::VS) -> Result<Option<DP::V>, DP::Err> {

diff --git a/src/internal/core.rs b/src/internal/core.rs
@@ -16,7 +16,7 @@ use crate::internal::partial_solution::{DecisionLevel, PartialSolution};
 use crate::internal::small_vec::SmallVec;
 use crate::report::DerivationTree;
 use crate::solver::DependencyProvider;
-use crate::type_aliases::{DependencyConstraints, IncompDpId, Map};
+use crate::type_aliases::{IncompDpId, Map};
 use crate::version_set::VersionSet;
 
 /// Current state of the PubGrub algorithm.
@@ -84,12 +84,12 @@ impl<DP: DependencyProvider> State<DP> {
         &mut self,
         package: DP::P,
         version: DP::V,
-        deps: &DependencyConstraints<DP::P, DP::VS>,
+        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.iter().map(|dep| {
+                .alloc_iter(deps.into_iter().map(|dep| {
                     Incompatibility::from_dependency(
                         package.clone(),
                         <DP::VS as VersionSet>::singleton(version.clone()),

diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs
@@ -137,18 +137,18 @@ impl<P: Package, VS: VersionSet, M: Eq + Clone + Debug + Display> Incompatibilit
     }
 
     /// Build an incompatibility from a given dependency.
-    pub fn from_dependency(package: P, versions: VS, dep: (&P, &VS)) -> Self {
+    pub fn from_dependency(package: P, versions: VS, dep: (P, VS)) -> Self {
         let (p2, set2) = dep;
         Self {
-            package_terms: if set2 == &VS::empty() {
+            package_terms: if set2 == VS::empty() {
                 SmallMap::One([(package.clone(), Term::Positive(versions.clone()))])
             } else {
                 SmallMap::Two([
                     (package.clone(), Term::Positive(versions.clone())),
                     (p2.clone(), Term::Negative(set2.clone())),
                 ])
             },
-            kind: Kind::FromDependencyOf(package, versions, p2.clone(), set2.clone()),
+            kind: Kind::FromDependencyOf(package, versions, p2, set2),
         }
     }
 
@@ -190,7 +190,10 @@ impl<P: Package, VS: VersionSet, M: Eq + Clone + Debug + Display> Incompatibilit
                 .unwrap()
                 .unwrap_positive()
                 .union(other.get(p1).unwrap().unwrap_positive()), // It is safe to `simplify` here
-            (p2, dep_term.map_or(&VS::empty(), |v| v.unwrap_negative())),
+            (
+                p2.clone(),
+                dep_term.map_or(VS::empty(), |v| v.unwrap_negative().clone()),
+            ),
         ));
     }
 

diff --git a/src/lib.rs b/src/lib.rs
@@ -96,8 +96,8 @@
 //!         &self,
 //!         package: &String,
 //!         version: &SemanticVersion,
-//!     ) -> Result<Dependencies<String, SemVS, Self::M>, Infallible> {
-//!         unimplemented!()
+//!     ) -> Result<Dependencies<impl IntoIterator<Item = (String, SemVS)> + Clone, Self::M>, Infallible> {
+//!         Ok(Dependencies::Available([]))
 //!     }
 //!
 //!     type Err = Infallible;

diff --git a/src/solver.rs b/src/solver.rs
@@ -71,7 +71,7 @@ use crate::error::PubGrubError;
 use crate::internal::core::State;
 use crate::internal::incompatibility::Incompatibility;
 use crate::package::Package;
-use crate::type_aliases::{DependencyConstraints, Map, SelectedDependencies};
+use crate::type_aliases::{Map, SelectedDependencies};
 use crate::version_set::VersionSet;
 use log::{debug, info};
 
@@ -158,22 +158,22 @@ pub fn resolve<DP: DependencyProvider>(
                     ));
                     continue;
                 }
-                Dependencies::Available(x) if x.contains_key(p) => {
+                Dependencies::Available(x) if x.clone().into_iter().any(|(d, _)| &d == p) => {
                     return Err(PubGrubError::SelfDependency {
                         package: p.clone(),
-                        version: v,
+                        version: v.clone(),
                     });
                 }
                 Dependencies::Available(x) => x,
             };
 
             // Add that package and version if the dependencies are not problematic.
             let dep_incompats =
-                state.add_incompatibility_from_dependencies(p.clone(), v.clone(), &dependencies);
+                state.add_incompatibility_from_dependencies(p.clone(), v.clone(), dependencies);
 
             state.partial_solution.add_version(
                 p.clone(),
-                v,
+                v.clone(),
                 dep_incompats,
                 &state.incompatibility_store,
             );
@@ -189,11 +189,11 @@ pub fn resolve<DP: DependencyProvider>(
 /// An enum used by [DependencyProvider] that holds information about package dependencies.
 /// For each [Package] there is a set of versions allowed as a dependency.
 #[derive(Clone)]
-pub enum Dependencies<P: Package, VS: VersionSet, M: Eq + Clone + Debug + Display> {
+pub enum Dependencies<T, M: Eq + Clone + Debug + Display> {
     /// Package dependencies are unavailable with the reason why they are missing.
     Unavailable(M),
     /// Container for all available package versions.
-    Available(DependencyConstraints<P, VS>),
+    Available(T),
 }
 
 /// Trait that allows the algorithm to retrieve available packages and their dependencies.
@@ -280,7 +280,10 @@ pub trait DependencyProvider {
         &self,
         package: &Self::P,
         version: &Self::V,
-    ) -> Result<Dependencies<Self::P, Self::VS, Self::M>, Self::Err>;
+    ) -> Result<
+        Dependencies<impl IntoIterator<Item = (Self::P, Self::VS)> + Clone, Self::M>,
+        Self::Err,
+    >;
 
     /// This is called fairly regularly during the resolution,
     /// if it returns an Err then resolution will be terminated.
@@ -304,7 +307,7 @@ pub trait DependencyProvider {
 )]
 #[cfg_attr(feature = "serde", serde(transparent))]
 pub struct OfflineDependencyProvider<P: Package, VS: VersionSet> {
-    dependencies: Map<P, BTreeMap<VS::V, DependencyConstraints<P, VS>>>,
+    dependencies: Map<P, BTreeMap<VS::V, Map<P, VS>>>,
 }
 
 impl<P: Package, VS: VersionSet> OfflineDependencyProvider<P, VS> {
@@ -354,8 +357,8 @@ impl<P: Package, VS: VersionSet> OfflineDependencyProvider<P, VS> {
 
     /// Lists dependencies of a given package and version.
     /// Returns [None] if no information is available regarding that package and version pair.
-    fn dependencies(&self, package: &P, version: &VS::V) -> Option<DependencyConstraints<P, VS>> {
-        self.dependencies.get(package)?.get(version).cloned()
+    fn dependencies(&self, package: &P, version: &VS::V) -> Option<&Map<P, VS>> {
+        self.dependencies.get(package)?.get(version)
     }
 }
 
@@ -393,12 +396,15 @@ impl<P: Package, VS: VersionSet> DependencyProvider for OfflineDependencyProvide
         &self,
         package: &P,
         version: &VS::V,
-    ) -> Result<Dependencies<P, VS, Self::M>, Infallible> {
+    ) -> Result<
+        Dependencies<impl IntoIterator<Item = (Self::P, Self::VS)> + Clone, Self::M>,
+        Self::Err,
+    > {
         Ok(match self.dependencies(package, version) {
             None => {
                 Dependencies::Unavailable("its dependencies could not be determined".to_string())
             }
-            Some(dependencies) => Dependencies::Available(dependencies),
+            Some(dependencies) => Dependencies::Available(dependencies.clone()),
         })
     }
 }
diff --git a/src/type_aliases.rs b/src/type_aliases.rs
@@ -10,18 +10,10 @@ pub type Map<K, V> = rustc_hash::FxHashMap<K, V>;
 /// Set implementation used by the library.
 pub type Set<V> = rustc_hash::FxHashSet<V>;
 
-/// Concrete dependencies picked by the library during [resolve](crate::solver::resolve)
-/// from [DependencyConstraints].
+/// Concrete dependencies picked by the library during [resolve](crate::solver::resolve).
 pub type SelectedDependencies<DP> =
     Map<<DP as DependencyProvider>::P, <DP as DependencyProvider>::V>;
 
-/// Holds information about all possible versions a given package can accept.
-/// There is a difference in semantics between an empty map
-/// inside [DependencyConstraints] and [Dependencies::Unavailable](crate::solver::Dependencies::Unavailable):
-/// the former means the package has no dependency and it is a known fact,
-/// while the latter means they could not be fetched by the [DependencyProvider].
-pub type DependencyConstraints<P, VS> = Map<P, VS>;
-
 pub(crate) type IncompDpId<DP> = IncompId<
     <DP as DependencyProvider>::P,
     <DP as DependencyProvider>::VS,

diff --git a/tests/proptest.rs b/tests/proptest.rs
@@ -37,7 +37,7 @@ impl<P: Package, VS: VersionSet> DependencyProvider for OldestVersionsDependency
         &self,
         p: &P,
         v: &VS::V,
-    ) -> Result<Dependencies<P, VS, Self::M>, Infallible> {
+    ) -> Result<Dependencies<impl IntoIterator<Item = (P, VS)> + Clone, Self::M>, Infallible> {
         self.0.get_dependencies(p, v)
     }
 
@@ -90,7 +90,10 @@ impl<DP: DependencyProvider> DependencyProvider for TimeoutDependencyProvider<DP
         &self,
         p: &DP::P,
         v: &DP::V,
-    ) -> Result<Dependencies<DP::P, DP::VS, DP::M>, DP::Err> {
+    ) -> Result<
+        Dependencies<impl IntoIterator<Item = (DP::P, DP::VS)> + Clone + 'static, DP::M>,
+        DP::Err,
+    > {
         self.dp.get_dependencies(p, v)
     }
 
@@ -352,8 +355,8 @@ fn retain_dependencies<N: Package + Ord, VS: VersionSet>(
             smaller_dependency_provider.add_dependencies(
                 n.clone(),
                 v.clone(),
-                deps.iter().filter_map(|(dep, range)| {
-                    if !retain(n, v, dep) {
+                deps.into_iter().filter_map(|(dep, range)| {
+                    if !retain(n, v, &dep) {
                         None
                     } else {
                         Some((dep.clone(), range.clone()))

diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs
@@ -69,10 +69,10 @@ impl<P: Package, VS: VersionSet> SatResolve<P, VS> {
                 Dependencies::Unavailable(_) => panic!(),
                 Dependencies::Available(d) => d,
             };
-            for (p1, range) in &deps {
+            for (p1, range) in deps {
                 let empty_vec = vec![];
                 let mut matches: Vec<varisat::Lit> = all_versions_by_p
-                    .get(p1)
+                    .get(&p1)
                     .unwrap_or(&empty_vec)
                     .iter()
                     .filter(|(v1, _)| range.contains(v1))