Auto merge of #14690 - x-hgg-x:resolver-perf-3, r=Eh2406

bors · bors · commit 8040c0051dad · 2024-10-15T18:08:23.000Z
fix(resolver): avoid cloning when iterating using RcVecIter ### What does this PR try to resolve? Follow up of Eh2406/pubgrub-crates-benchmark#6 (comment). This PR modifies the internal `RcVecIter` struct so that we can iterate on its items without cloning them. This improves performance of the resolver because eagerly cloning `Arc<T>` is not free. Comparison of performance using `solana-core = "=1.0.5"` in `Cargo.toml`: | branch | duration | |------------|----------| | master | 213s | | PR | 202s | r? Eh2406
diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
@@ -446,13 +446,13 @@ fn activate_deps_loop(
                     // conflict with us.
                     let mut has_past_conflicting_dep = just_here_for_the_error_messages;
                     if !has_past_conflicting_dep {
-                        if let Some(conflicting) = frame
-                            .remaining_siblings
-                            .clone()
-                            .filter_map(|(ref new_dep, _, _)| {
-                                past_conflicting_activations.conflicting(&resolver_ctx, new_dep)
-                            })
-                            .next()
+                        if let Some(conflicting) =
+                            frame
+                                .remaining_siblings
+                                .remaining()
+                                .find_map(|(ref new_dep, _, _)| {
+                                    past_conflicting_activations.conflicting(&resolver_ctx, new_dep)
+                                })
                         {
                             // If one of our deps is known unresolvable
                             // then we will not succeed.
@@ -757,7 +757,7 @@ impl RemainingCandidates {
         conflicting_prev_active: &mut ConflictMap,
         cx: &ResolverContext,
     ) -> Option<(Summary, bool)> {
-        for b in self.remaining.by_ref() {
+        for b in self.remaining.iter() {
             let b_id = b.package_id();
             // The `links` key in the manifest dictates that there's only one
             // package in a dependency graph, globally, with that particular
@@ -783,7 +783,7 @@ impl RemainingCandidates {
             // Here we throw out our candidate if it's *compatible*, yet not
             // equal, to all previously activated versions.
             if let Some((a, _)) = cx.activations.get(&b_id.as_activations_key()) {
-                if *a != b {
+                if a != b {
                     conflicting_prev_active
                         .entry(a.package_id())
                         .or_insert(ConflictReason::Semver);
@@ -796,7 +796,7 @@ impl RemainingCandidates {
             // necessarily return the item just yet. Instead we stash it away to
             // get returned later, and if we replaced something then that was
             // actually the candidate to try first so we return that.
-            if let Some(r) = mem::replace(&mut self.has_another, Some(b)) {
+            if let Some(r) = mem::replace(&mut self.has_another, Some(b.clone())) {
                 return Some((r, true));
             }
         }
diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs
@@ -5,7 +5,6 @@ use crate::util::interning::InternedString;
 use crate::util::GlobalContext;
 use std::cmp::Ordering;
 use std::collections::{BTreeMap, BTreeSet};
-use std::ops::Range;
 use std::rc::Rc;
 use std::time::{Duration, Instant};
 
@@ -181,13 +180,13 @@ impl DepsFrame {
     fn min_candidates(&self) -> usize {
         self.remaining_siblings
             .peek()
-            .map(|(_, (_, candidates, _))| candidates.len())
+            .map(|(_, candidates, _)| candidates.len())
             .unwrap_or(0)
     }
 
-    pub fn flatten(&self) -> impl Iterator<Item = (PackageId, Dependency)> + '_ {
+    pub fn flatten(&self) -> impl Iterator<Item = (PackageId, &Dependency)> + '_ {
         self.remaining_siblings
-            .clone()
+            .remaining()
             .map(move |(d, _, _)| (self.parent.package_id(), d))
     }
 }
@@ -251,15 +250,16 @@ impl RemainingDeps {
             // Figure out what our next dependency to activate is, and if nothing is
             // listed then we're entirely done with this frame (yay!) and we can
             // move on to the next frame.
-            if let Some(sibling) = deps_frame.remaining_siblings.next() {
+            let sibling = deps_frame.remaining_siblings.iter().next().cloned();
+            if let Some(sibling) = sibling {
                 let parent = Summary::clone(&deps_frame.parent);
                 self.data.insert((deps_frame, insertion_time));
                 return Some((just_here_for_the_error_messages, (parent, sibling)));
             }
         }
         None
     }
-    pub fn iter(&mut self) -> impl Iterator<Item = (PackageId, Dependency)> + '_ {
+    pub fn iter(&mut self) -> impl Iterator<Item = (PackageId, &Dependency)> + '_ {
         self.data.iter().flat_map(|(other, _)| other.flatten())
     }
 }
@@ -324,22 +324,27 @@ pub type ConflictMap = BTreeMap<PackageId, ConflictReason>;
 
 pub struct RcVecIter<T> {
     vec: Rc<Vec<T>>,
-    rest: Range<usize>,
+    offset: usize,
 }
 
 impl<T> RcVecIter<T> {
     pub fn new(vec: Rc<Vec<T>>) -> RcVecIter<T> {
-        RcVecIter {
-            rest: 0..vec.len(),
-            vec,
-        }
+        RcVecIter { vec, offset: 0 }
+    }
+
+    pub fn peek(&self) -> Option<&T> {
+        self.vec.get(self.offset)
     }
 
-    fn peek(&self) -> Option<(usize, &T)> {
-        self.rest
-            .clone()
-            .next()
-            .and_then(|i| self.vec.get(i).map(|val| (i, &*val)))
+    pub fn remaining(&self) -> impl Iterator<Item = &T> + '_ {
+        self.vec.get(self.offset..).into_iter().flatten()
+    }
+
+    pub fn iter(&mut self) -> impl Iterator<Item = &T> + '_ {
+        let iter = self.vec.get(self.offset..).into_iter().flatten();
+        // This call to `ìnspect()` is used to increment `self.offset` when iterating the inner `Vec`,
+        // while keeping the `ExactSizeIterator` property.
+        iter.inspect(|_| self.offset += 1)
     }
 }
 
@@ -348,25 +353,7 @@ impl<T> Clone for RcVecIter<T> {
     fn clone(&self) -> RcVecIter<T> {
         RcVecIter {
             vec: self.vec.clone(),
-            rest: self.rest.clone(),
+            offset: self.offset,
         }
     }
 }
-
-impl<T> Iterator for RcVecIter<T>
-where
-    T: Clone,
-{
-    type Item = T;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        self.rest.next().and_then(|i| self.vec.get(i).cloned())
-    }
-
-    fn size_hint(&self) -> (usize, Option<usize>) {
-        // rest is a std::ops::Range, which is an ExactSizeIterator.
-        self.rest.size_hint()
-    }
-}
-
-impl<T: Clone> ExactSizeIterator for RcVecIter<T> {}
