Skip to content

Faster resolver: clean code and the backtrack_stack #5187

New issue

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

#

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

Already on GitHub? # to your account

Merged
merged 8 commits into from
Mar 17, 2018
Merged

Faster resolver: clean code and the backtrack_stack #5187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
15c757a
Clean the `backtrack_stack` so we don't backtrack to a place with cas…
Eh2406 Mar 15, 2018
617856e
add a test
Eh2406 Mar 15, 2018
1291c50
cargo +stable fmt
Eh2406 Mar 15, 2018
68e9577
suggestions
Eh2406 Mar 16, 2018
59249d1
remove duplicated adding to the cache
Eh2406 Mar 16, 2018
f5a0f28
Revert "Clean the `backtrack_stack` so we don't backtrack to a place …
Eh2406 Mar 16, 2018
dd9ff1f
When test backtracking include conflicts in `remaining_candidates`
Eh2406 Mar 17, 2018
c771a4c
When activating for the better error messages don't waste time on the…
Eh2406 Mar 17, 2018
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
96 changes: 48 additions & 48 deletions src/cargo/core/resolver/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -951,12 +951,9 @@ fn activate_deps_loop(
&& past_conflicting_activations
.get(&dep)
.and_then(|past_bad| {
past_bad.iter().find(|conflicting| {
conflicting
.iter()
// note: a lot of redundant work in is_active for similar debs
.all(|(con, _)| cx.is_active(con))
})
past_bad
.iter()
.find(|conflicting| cx.is_conflicting(None, conflicting))
})
.is_some();

Expand Down Expand Up @@ -1081,61 +1078,53 @@ fn activate_deps_loop(

match res {
Ok(Some((mut frame, dur))) => {
// at this point we have technical already activated
// at this point we have technically already activated
// but we may want to scrap it if it is not going to end well
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(|(_, (deb, _, _))| {
past_conflicting_activations.get(&deb).and_then(|past_bad| {
// for each dependency check all of its cashed conflicts
past_bad.iter().find(|conflicting| {
conflicting
.iter()
// note: a lot of redundant work in is_active for similar debs
.all(|(con, _)| cx.is_active(con))
})
})
.filter_map(|(_, (new_dep, _, _))| {
past_conflicting_activations.get(&new_dep)
})
.next()
.flat_map(|x| x)
.find(|con| cx.is_conflicting(None, con))
{
// if any of them match than it will just backtrack to us
// so let's save the effort.
conflicting_activations.extend(conflicting.clone());
has_past_conflicting_dep = true;
}
}
if !has_another && has_past_conflicting_dep && !backtracked {
// we have not activated ANY candidates and
// we are out of choices so add it to the cache
// so our parent will know that we don't work
let past = past_conflicting_activations
.entry(dep.clone())
.or_insert_with(Vec::new);
if !past.contains(&conflicting_activations) {
trace!(
"{}[{}]>{} adding a meta-skip {:?}",
parent.name(),
cur,
dep.name(),
conflicting_activations
);
past.push(conflicting_activations.clone());
let activate_for_error_message = has_past_conflicting_dep && !has_another && {
// has_past_conflicting_dep -> One of this candidate deps will fail.
// !has_another -> If the candidate is not accepted we will backtrack.

// So the question is will the user see that we skipped this candidate?
just_here_for_the_error_messages || {
// make sure we know about all the possible conflicts
conflicting_activations
.extend(remaining_candidates.conflicting_prev_active.clone());
// test if backtracking will get to the user
find_candidate(
&mut backtrack_stack.clone(),
&parent,
&conflicting_activations,
).is_none()
}
};
if activate_for_error_message {
// We know one of this candidate deps will fail,
// which means we will fail,
// and that none of the backtrack frames will
// find a candidate that will help.
// So lets clean up the useless backtrack frames.
backtrack_stack.clear();
}
// if not has_another we we activate for the better error messages
frame.just_for_error_messages = has_past_conflicting_dep;
if !has_past_conflicting_dep
|| (!has_another
&& (just_here_for_the_error_messages
|| find_candidate(
&mut backtrack_stack.clone(),
&parent,
&conflicting_activations,
).is_none()))
{
if !has_past_conflicting_dep || activate_for_error_message {
remaining_deps.push(frame);
} else {
trace!(
Expand Down Expand Up @@ -1195,11 +1184,9 @@ fn find_candidate<'a>(
frame.context_backup.prev_active(&frame.dep),
&frame.context_backup.links,
);
if frame.context_backup.is_active(parent.package_id())
&& conflicting_activations
.iter()
// note: a lot of redundant work in is_active for similar debs
.all(|(con, _)| frame.context_backup.is_active(con))
if frame
.context_backup
.is_conflicting(Some(parent.package_id()), conflicting_activations)
{
continue;
}
Expand Down Expand Up @@ -1643,6 +1630,19 @@ impl Context {
.unwrap_or(false)
}

/// checks whether all of `parent` and the keys of `conflicting activations`
/// are still active
fn is_conflicting(
&self,
parent: Option<&PackageId>,
conflicting_activations: &HashMap<PackageId, ConflictReason>,
) -> bool {
conflicting_activations
.keys()
.chain(parent)
.all(|id| self.is_active(id))
}

/// Return all dependencies and the features we want from them.
fn resolve_features<'b>(
&mut self,
Expand Down
43 changes: 43 additions & 0 deletions tests/testsuite/resolve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,49 @@ fn resolving_with_many_equivalent_backtracking() {
assert!(res.is_err());
}

#[test]
fn resolving_with_deep_traps() {
let mut reglist = Vec::new();

const DEPTH: usize = 200;
const BRANCHING_FACTOR: usize = 100;

// Each backtrack_trap depends on the next, and adds a backtrack frame.
// None of witch is going to help with `bad`.
for l in 0..DEPTH {
let name = format!("backtrack_trap{}", l);
let next = format!("backtrack_trap{}", l + 1);
for i in 1..BRANCHING_FACTOR {
let vsn = format!("1.0.{}", i);
reglist.push(pkg!((name.as_str(), vsn.as_str()) => [dep_req(next.as_str(), "*")]));
}
}
{
let name = format!("backtrack_trap{}", DEPTH);
for i in 1..BRANCHING_FACTOR {
let vsn = format!("1.0.{}", i);
reglist.push(pkg!((name.as_str(), vsn.as_str())));
}
}
{
// slightly less constrained to make sure `cloaking` gets picked last.
for i in 1..(BRANCHING_FACTOR + 10) {
let vsn = format!("1.0.{}", i);
reglist.push(pkg!(("cloaking", vsn.as_str()) => [dep_req("bad", "1.0.1")]));
}
}

let reg = registry(reglist.clone());

let res = resolve(
&pkg_id("root"),
vec![dep_req("backtrack_trap0", "*"), dep_req("cloaking", "*")],
&reg,
);

assert!(res.is_err());
}

#[test]
fn resolving_with_constrained_sibling_backtrack_activation() {
// It makes sense to resolve most-constrained deps first, but
Expand Down