Skip to content

Commit

Permalink
fix(resolver): avoid cloning when iterating using RcVecIter
Browse files Browse the repository at this point in the history
  • Loading branch information
x-hgg-x committed Oct 15, 2024
1 parent 430fabe commit f7f209f
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 45 deletions.
20 changes: 10 additions & 10 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -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));
}
}
Expand Down
54 changes: 19 additions & 35 deletions src/cargo/core/resolver/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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))
}
}
Expand Down Expand Up @@ -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_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)));
}
}
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())
}
}
Expand Down Expand Up @@ -324,22 +324,24 @@ 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_by_ref(&mut self) -> impl Iterator<Item = &T> + '_ {
(self.vec.get(self.offset..).into_iter().flatten()).inspect(|_| self.offset += 1)
}
}

Expand All @@ -348,25 +350,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> {}

0 comments on commit f7f209f

Please sign in to comment.