Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fast path for incompatible deps #6919

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,25 @@ impl Context {
.and_then(|(s, l)| if s.package_id() == id { Some(*l) } else { None })
}

/// This calculates `is_active` after backtracking to `age` and activating `alternative`.
/// If the package is active returns the `ContextAge` when it was added
/// but only if it is older then `age`.
/// If it is not active but it is the `alternative` then it returns `age`.
pub fn is_older_active_or(
&self,
id: PackageId,
age: ContextAge,
alternative: PackageId,
) -> Option<ContextAge> {
if id == alternative {
// we are imagining that we used other instead
Some(age)
} else {
// we only care about things that are older then critical_age
self.is_active(id).filter(|&other_age| other_age < age)
}
}

/// Checks whether all of `parent` and the keys of `conflicting activations`
/// are still active.
/// If so returns the `ContextAge` when the newest one was added.
Expand Down
218 changes: 167 additions & 51 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@ impl RemainingCandidates {
}
}

/// Attempts to find a new conflict that allows a `find_candidate` feather then the input one.
/// Attempts to find a new conflict that allows `find_candidate` to go further back then the input one.
/// It will add the new conflict to the cache if one is found.
///
/// Panics if the input conflict is not all active in `cx`.
Expand Down Expand Up @@ -880,80 +880,196 @@ fn generalize_conflicting(
// and let `backtrack_critical_id` figure this out.
return None;
}

// we will need to know the range of versions we can use
let our_candidates = registry
.query(dep)
.expect("an already used dep now error!?");

if our_candidates.is_empty() {
return None;
}

let our_activation_key = {
let first_activation_key = our_candidates[0].package_id().as_activations_key();

// If our dep only matches one semver range then we can fast path any other dep
// that also targets that semver range and has no overlap.
if our_candidates
.iter()
.all(|c| first_activation_key == c.package_id().as_activations_key())
{
Some(first_activation_key)
} else {
None
}
};

let our_link = {
let first_links = our_candidates[0].links();

// If our dep only matches things that have a links set then we can fast path any other dep
// that also all use that links and has no overlap.
if first_links.is_some() && our_candidates.iter().all(|c| first_links == c.links()) {
// All of `our_candidates` use the same non-`None` `links`, so we can use the fast path.
first_links
} else {
// Some of `our_candidates` use a different `links` so whatever `links` get used by
// the conflicting dep we can select the other. We cant use the fast path.
None
}
};

let our_candidates: HashSet<PackageId> =
our_candidates.iter().map(|our| our.package_id()).collect();

// What parents does that critical activation have
for (critical_parent, critical_parents_deps) in
cx.parents.edges(&backtrack_critical_id).filter(|(p, _)| {
// it will only help backjump further if it is older then the critical_age
cx.is_active(*p).expect("parent not currently active!?") < backtrack_critical_age
})
{
for critical_parents_dep in critical_parents_deps.iter() {
'dep: for critical_parents_dep in critical_parents_deps.iter() {
// A dep is equivalent to one of the things it can resolve to.
// Thus, if all the things it can resolve to have already ben determined
// to be conflicting, then we can just say that we conflict with the parent.
if let Some(others) = registry
let mut con = conflicting_activations.clone();
con.remove(&backtrack_critical_id);
con.insert(*critical_parent, backtrack_critical_reason.clone());

for other in registry
.query(critical_parents_dep)
.expect("an already used dep now error!?")
.iter()
.rev() // the last one to be tried is the least likely to be in the cache, so start with that.
.map(|other| {
past_conflicting_activations
.find(
dep,
&|id| {
if id == other.package_id() {
// we are imagining that we used other instead
Some(backtrack_critical_age)
} else {
cx.is_active(id).filter(|&age|
// we only care about things that are older then critical_age
age < backtrack_critical_age)
}
},
Some(other.package_id()),
)
.map(|con| (other.package_id(), con))
})
.collect::<Option<Vec<(PackageId, &ConflictMap)>>>()
.rev()
// the last one to be tried is the least likely to be in the cache, so start with that.
{
let mut con = conflicting_activations.clone();
// It is always valid to combine previously inserted conflicts.
// A, B are both known bad states each that can never be activated.
// A + B is redundant but cant be activated, as if
// A + B is active then A is active and we know that is not ok.
for (_, other) in &others {
con.extend(other.iter().map(|(&id, re)| (id, re.clone())));
if (
our_activation_key.map_or(false, |our| {
other.package_id().as_activations_key() == our
})
// `other` is semver compatible with all of `our_candidates`
|| our_link.map_or(false, |_| other.links() == our_link)
// or `other` have the same `links` as all of `our_candidates`
) && !our_candidates.contains(&other.package_id())
// and is not one of `our_candidates`.
{
// Thus `dep` can not be resolved when `critical_parents_dep` has bean resolved.
continue;
}
// Now that we have this combined conflict, we can do a substitution:
// A dep is equivalent to one of the things it can resolve to.
// So we can remove all the things that it resolves to and replace with the parent.
for (other_id, _) in &others {
con.remove(other_id);

// ok so we dont have a semver nor a links problem with `critical_parents_dep`
// getting set to `other`, maybe it still wont work do to one of its dependencies.
if let Some(conflicting) = generalize_children_conflicting(
cx,
backtrack_critical_age,
registry,
past_conflicting_activations,
dep,
&other,
*critical_parent,
) {
con.extend(conflicting.into_iter());
continue;
}
con.insert(*critical_parent, backtrack_critical_reason);

if cfg!(debug_assertions) {
// the entire point is to find an older conflict, so let's make sure we did
let new_age = con
.keys()
.map(|&c| cx.is_active(c).expect("not currently active!?"))
.max()
.unwrap();
assert!(
new_age < backtrack_critical_age,
"new_age {} < backtrack_critical_age {}",
new_age,
backtrack_critical_age

if let Some(conflicting) = past_conflicting_activations.find(
dep,
&|id| cx.is_older_active_or(id, backtrack_critical_age, other.package_id()),
Some(other.package_id()),
) {
con.extend(
conflicting
.iter()
.filter(|(&id, _)| id != other.package_id())
.map(|(&id, r)| (id, r.clone())),
);
continue;
}
past_conflicting_activations.insert(dep, &con);
return Some(con);

// We don't have a reason why `other` cant work.
// There for `critical_parents_dep` could have used it, and we can still be activated.
// We can not generalize over `critical_parents_dep`, but maybe the next `critical_parents_dep`
continue 'dep;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could there be a comment here indicating why we're breaking to the outer loop and cancelling this part of the search?

}

if cfg!(debug_assertions) {
// the entire point is to find an older conflict, so let's make sure we did
let new_age = con
.keys()
.map(|&c| cx.is_active(c).expect("not currently active!?"))
.max()
.unwrap();
assert!(
new_age < backtrack_critical_age,
"new_age {} < backtrack_critical_age {}",
new_age,
backtrack_critical_age
);
}
past_conflicting_activations.insert(dep, &con);
return Some(con);
}
}
None
}

/// Sees if the candidate is not usable because one of its children will have a conflict
///
/// Panics if the input conflict is not all active in `cx`.
fn generalize_children_conflicting(
cx: &Context,
critical_age: usize,
registry: &mut RegistryQueryer<'_>,
past_conflicting_activations: &mut conflict_cache::ConflictCache,
dep: &Dependency,
candidate: &Summary,
parent: PackageId,
) -> Option<ConflictMap> {
let method = Method::Required {
dev_deps: false,
features: Rc::new(dep.features().iter().cloned().collect()),
all_features: false,
uses_default_features: dep.uses_default_features(),
};
let mut out = ConflictMap::new();
match registry.build_deps(Some(parent), candidate, &method) {
Err(ActivateError::Fatal(_)) => return None,
Err(ActivateError::Conflict(pid, reason)) => {
out.insert(pid, reason);
}
Ok(deps) => {
let con = deps
.1
.iter()
.filter_map(|(ref new_dep, _, _)| {
past_conflicting_activations.find(
new_dep,
&|id| cx.is_older_active_or(id, critical_age, candidate.package_id()),
None,
)
})
.next()?;
out.extend(
con.iter()
.filter(|(pid, _)| **pid != candidate.package_id())
.map(|(&pid, reason)| (pid, reason.clone())),
);
}
};
// If one of candidate deps is known unresolvable
// then candidate will not succeed.
// How ever if candidate is part of the reason that
// one of its deps conflicts then
// we can make a stronger statement
// because candidate will definitely be activated when
// we try its dep.
out.remove(&candidate.package_id());

Some(out)
}

/// Looks through the states in `backtrack_stack` for dependencies with
/// remaining candidates. For each one, also checks if rolling back
/// could change the outcome of the failed resolution that caused backtracking
Expand Down
11 changes: 8 additions & 3 deletions tests/testsuite/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1352,16 +1352,21 @@ fn large_conflict_cache() {
// a large number of conflicts can easily be generated by a sys crate.
let sys_name = format!("{}-sys", (b'a' + name) as char);
let in_len = input.len();
input.push(pkg!(("last", format!("{}.0.0", in_len)) => [dep_req(&sys_name, "=0.0.0")]));
input.push(pkg!(("last", format!("{}.0.0", in_len)) => [dep_req(&sys_name, "<=0.0.1")]));
root_deps.push(dep_req(&sys_name, ">= 0.0.1"));

// a large number of conflicts can also easily be generated by a major release version.
let plane_name = format!("{}", (b'a' + name) as char);
let in_len = input.len();
input.push(pkg!(("last", format!("{}.0.0", in_len)) => [dep_req(&plane_name, "=1.0.0")]));
input.push(pkg!(("last", format!("{}.0.0", in_len)) => [dep_req(&plane_name, "<=1.0.1")]));
root_deps.push(dep_req(&plane_name, ">= 1.0.1"));

for i in 0..=NUM_VERSIONS {
input.push(pkg!((&sys_name, "0.0.0")));
input.push(pkg!((&plane_name, "1.0.0")));
// one version that can't be activated for some other reason
input.push(pkg!((&sys_name, "0.0.1") => [dep("bad")]));
input.push(pkg!((&plane_name, "1.0.1") => [dep("bad")]));
for i in 2..=NUM_VERSIONS {
input.push(pkg!((&sys_name, format!("{}.0.0", i))));
input.push(pkg!((&plane_name, format!("1.0.{}", i))));
}
Expand Down