Skip to content

Commit bdc6fc2

Browse files
borsbors
committed
Auto merge of #5187 - Eh2406:faster_resolver, r=alexcrichton
Faster resolver: clean code and the `backtrack_stack` This is a small extension to #5168 and is inspired by #4834 (comment) After #5168 these work (and don't on cargo from nightly.): - `safe_core = "=0.22.4"` - `safe_vault = "=0.13.2"` But these don't work (and do on cargo from this PR.) - `crust = "=0.24.0"` - `elastic = "=0.3.0"` - `elastic = "=0.4.0"` - `elastic = "=0.5.0"` - `safe_vault = "=0.14.0"` It took some work to figure out why they are not working, and make a test case. This PR remove use of `conflicting_activations` before it is extended with the conflicting from next. #5187 (comment) However the `find_candidate(` is still needed so it now gets the conflicting from next before being called. It often happens that the candidate whose child will fail leading to it's failure, will have older siblings that have already set up `backtrack_frame`s. The candidate knows that it's failure can not be saved by its siblings, but sometimes we activate the child anyway for the error messages. Unfortunately the child does not know that is uncles can't save it, so it backtracks to one of them. Leading to a combinatorial loop. The solution is to clear the `backtrack_stack` if we are activating just for the error messages. Edit original end of this message, no longer accurate. #5168 means that when we find a permanent problem we will never **activate** its parent again. In practise there afften is a lot of work and `backtrack_frame`s between the problem and reactivating its parent. This PR removes `backtrack_frame`s where its parent and the problem are present. This means that when we find a permanent problem we will never **backtrack** into it again. An alternative is to scan all cashed problems while backtracking, but this seemed more efficient.
2 parents 545a4a2 + c771a4c commit bdc6fc2

File tree

2 files changed

+91
-48
lines changed

2 files changed

+91
-48
lines changed

‎src/cargo/core/resolver/mod.rs

Lines changed: 48 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -990,12 +990,9 @@ fn activate_deps_loop(
990990
&& past_conflicting_activations
991991
.get(&dep)
992992
.and_then(|past_bad| {
993-
past_bad.iter().find(|conflicting| {
994-
conflicting
995-
.iter()
996-
// note: a lot of redundant work in is_active for similar debs
997-
.all(|(con, _)| cx.is_active(con))
998-
})
993+
past_bad
994+
.iter()
995+
.find(|conflicting| cx.is_conflicting(None, conflicting))
999996
})
1000997
.is_some();
1001998

@@ -1120,61 +1117,53 @@ fn activate_deps_loop(
11201117

11211118
match res {
11221119
Ok(Some((mut frame, dur))) => {
1123-
// at this point we have technical already activated
1120+
// at this point we have technically already activated
11241121
// but we may want to scrap it if it is not going to end well
11251122
let mut has_past_conflicting_dep = just_here_for_the_error_messages;
11261123
if !has_past_conflicting_dep {
11271124
if let Some(conflicting) = frame
11281125
.remaining_siblings
11291126
.clone()
1130-
.filter_map(|(_, (deb, _, _))| {
1131-
past_conflicting_activations.get(&deb).and_then(|past_bad| {
1132-
// for each dependency check all of its cashed conflicts
1133-
past_bad.iter().find(|conflicting| {
1134-
conflicting
1135-
.iter()
1136-
// note: a lot of redundant work in is_active for similar debs
1137-
.all(|(con, _)| cx.is_active(con))
1138-
})
1139-
})
1127+
.filter_map(|(_, (new_dep, _, _))| {
1128+
past_conflicting_activations.get(&new_dep)
11401129
})
1141-
.next()
1130+
.flat_map(|x| x)
1131+
.find(|con| cx.is_conflicting(None, con))
11421132
{
11431133
// if any of them match than it will just backtrack to us
11441134
// so let's save the effort.
11451135
conflicting_activations.extend(conflicting.clone());
11461136
has_past_conflicting_dep = true;
11471137
}
11481138
}
1149-
if !has_another && has_past_conflicting_dep && !backtracked {
1150-
// we have not activated ANY candidates and
1151-
// we are out of choices so add it to the cache
1152-
// so our parent will know that we don't work
1153-
let past = past_conflicting_activations
1154-
.entry(dep.clone())
1155-
.or_insert_with(Vec::new);
1156-
if !past.contains(&conflicting_activations) {
1157-
trace!(
1158-
"{}[{}]>{} adding a meta-skip {:?}",
1159-
parent.name(),
1160-
cur,
1161-
dep.name(),
1162-
conflicting_activations
1163-
);
1164-
past.push(conflicting_activations.clone());
1139+
let activate_for_error_message = has_past_conflicting_dep && !has_another && {
1140+
// has_past_conflicting_dep -> One of this candidate deps will fail.
1141+
// !has_another -> If the candidate is not accepted we will backtrack.
1142+
1143+
// So the question is will the user see that we skipped this candidate?
1144+
just_here_for_the_error_messages || {
1145+
// make sure we know about all the possible conflicts
1146+
conflicting_activations
1147+
.extend(remaining_candidates.conflicting_prev_active.clone());
1148+
// test if backtracking will get to the user
1149+
find_candidate(
1150+
&mut backtrack_stack.clone(),
1151+
&parent,
1152+
&conflicting_activations,
1153+
).is_none()
11651154
}
1155+
};
1156+
if activate_for_error_message {
1157+
// We know one of this candidate deps will fail,
1158+
// which means we will fail,
1159+
// and that none of the backtrack frames will
1160+
// find a candidate that will help.
1161+
// So lets clean up the useless backtrack frames.
1162+
backtrack_stack.clear();
11661163
}
11671164
// if not has_another we we activate for the better error messages
11681165
frame.just_for_error_messages = has_past_conflicting_dep;
1169-
if !has_past_conflicting_dep
1170-
|| (!has_another
1171-
&& (just_here_for_the_error_messages
1172-
|| find_candidate(
1173-
&mut backtrack_stack.clone(),
1174-
&parent,
1175-
&conflicting_activations,
1176-
).is_none()))
1177-
{
1166+
if !has_past_conflicting_dep || activate_for_error_message {
11781167
remaining_deps.push(frame);
11791168
} else {
11801169
trace!(
@@ -1234,11 +1223,9 @@ fn find_candidate<'a>(
12341223
frame.context_backup.prev_active(&frame.dep),
12351224
&frame.context_backup.links,
12361225
);
1237-
if frame.context_backup.is_active(parent.package_id())
1238-
&& conflicting_activations
1239-
.iter()
1240-
// note: a lot of redundant work in is_active for similar debs
1241-
.all(|(con, _)| frame.context_backup.is_active(con))
1226+
if frame
1227+
.context_backup
1228+
.is_conflicting(Some(parent.package_id()), conflicting_activations)
12421229
{
12431230
continue;
12441231
}
@@ -1682,6 +1669,19 @@ impl Context {
16821669
.unwrap_or(false)
16831670
}
16841671

1672+
/// checks whether all of `parent` and the keys of `conflicting activations`
1673+
/// are still active
1674+
fn is_conflicting(
1675+
&self,
1676+
parent: Option<&PackageId>,
1677+
conflicting_activations: &HashMap<PackageId, ConflictReason>,
1678+
) -> bool {
1679+
conflicting_activations
1680+
.keys()
1681+
.chain(parent)
1682+
.all(|id| self.is_active(id))
1683+
}
1684+
16851685
/// Return all dependencies and the features we want from them.
16861686
fn resolve_features<'b>(
16871687
&mut self,

‎tests/testsuite/resolve.rs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -613,6 +613,49 @@ fn resolving_with_many_equivalent_backtracking() {
613613
assert!(res.is_err());
614614
}
615615

616+
#[test]
617+
fn resolving_with_deep_traps() {
618+
let mut reglist = Vec::new();
619+
620+
const DEPTH: usize = 200;
621+
const BRANCHING_FACTOR: usize = 100;
622+
623+
// Each backtrack_trap depends on the next, and adds a backtrack frame.
624+
// None of witch is going to help with `bad`.
625+
for l in 0..DEPTH {
626+
let name = format!("backtrack_trap{}", l);
627+
let next = format!("backtrack_trap{}", l + 1);
628+
for i in 1..BRANCHING_FACTOR {
629+
let vsn = format!("1.0.{}", i);
630+
reglist.push(pkg!((name.as_str(), vsn.as_str()) => [dep_req(next.as_str(), "*")]));
631+
}
632+
}
633+
{
634+
let name = format!("backtrack_trap{}", DEPTH);
635+
for i in 1..BRANCHING_FACTOR {
636+
let vsn = format!("1.0.{}", i);
637+
reglist.push(pkg!((name.as_str(), vsn.as_str())));
638+
}
639+
}
640+
{
641+
// slightly less constrained to make sure `cloaking` gets picked last.
642+
for i in 1..(BRANCHING_FACTOR + 10) {
643+
let vsn = format!("1.0.{}", i);
644+
reglist.push(pkg!(("cloaking", vsn.as_str()) => [dep_req("bad", "1.0.1")]));
645+
}
646+
}
647+
648+
let reg = registry(reglist.clone());
649+
650+
let res = resolve(
651+
&pkg_id("root"),
652+
vec![dep_req("backtrack_trap0", "*"), dep_req("cloaking", "*")],
653+
&reg,
654+
);
655+
656+
assert!(res.is_err());
657+
}
658+
616659
#[test]
617660
fn resolving_with_constrained_sibling_backtrack_activation() {
618661
// It makes sense to resolve most-constrained deps first, but

0 commit comments

Comments
 (0)