From f7f209fe7afa4753708332178bf3e21fbd2b437e Mon Sep 17 00:00:00 2001 From: x-hgg-x <39058530+x-hgg-x@users.noreply.github.com> Date: Tue, 15 Oct 2024 08:58:00 +0200 Subject: [PATCH] fix(resolver): avoid cloning when iterating using RcVecIter --- src/cargo/core/resolver/mod.rs | 20 ++++++------ src/cargo/core/resolver/types.rs | 54 +++++++++++--------------------- 2 files changed, 29 insertions(+), 45 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 08d5face1d8b..f4813f9bb5df 100644 --- 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_by_ref() { 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 index 36def5a964d9..79a16f24f046 100644 --- 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 + '_ { + pub fn flatten(&self) -> impl Iterator + '_ { self.remaining_siblings - .clone() + .remaining() .map(move |(d, _, _)| (self.parent.package_id(), d)) } } @@ -251,7 +250,8 @@ 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_by_ref().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))); @@ -259,7 +259,7 @@ impl RemainingDeps { } None } - pub fn iter(&mut self) -> impl Iterator + '_ { + pub fn iter(&mut self) -> impl Iterator + '_ { self.data.iter().flat_map(|(other, _)| other.flatten()) } } @@ -324,22 +324,24 @@ pub type ConflictMap = BTreeMap; pub struct RcVecIter { vec: Rc>, - rest: Range, + offset: usize, } impl RcVecIter { pub fn new(vec: Rc>) -> RcVecIter { - 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 + '_ { + self.vec.get(self.offset..).into_iter().flatten() + } + + pub fn iter_by_ref(&mut self) -> impl Iterator + '_ { + (self.vec.get(self.offset..).into_iter().flatten()).inspect(|_| self.offset += 1) } } @@ -348,25 +350,7 @@ impl Clone for RcVecIter { fn clone(&self) -> RcVecIter { RcVecIter { vec: self.vec.clone(), - rest: self.rest.clone(), + offset: self.offset, } } } - -impl Iterator for RcVecIter -where - T: Clone, -{ - type Item = T; - - fn next(&mut self) -> Option { - self.rest.next().and_then(|i| self.vec.get(i).cloned()) - } - - fn size_hint(&self) -> (usize, Option) { - // rest is a std::ops::Range, which is an ExactSizeIterator. - self.rest.size_hint() - } -} - -impl ExactSizeIterator for RcVecIter {}