Faster resolver: clean code and the backtrack_stack

src/cargo/core/resolver/mod.rs

@@ -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();
 
@@ -1081,25 +1078,16 @@ 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))
-                                    })
-                                })
-                            })
-                            .next()
+                            .filter_map(|(_, (new_dep, _, _))| past_conflicting_activations.get(&new_dep))
+                            .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.
@@ -1108,22 +1096,16 @@ fn activate_deps_loop(
                         }
                     }
                     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());
-                        }
+                        // we have not activated ANY candidates.
+                        // So clean the `backtrack_stack` so we don't
+                        // backtrack to a place where we will try this again.
+                        backtrack_stack.retain(|bframe| {
+                            // Maybe we could just not add them in the first place, but for now this works
+                            !bframe.context_backup.is_conflicting(
+                                Some(parent.package_id()),
+                                &conflicting_activations,
+                            )
+                        });
                     }
                     // if not has_another we we activate for the better error messages
                     frame.just_for_error_messages = has_past_conflicting_dep;
@@ -1195,11 +1177,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;
         }
@@ -1643,6 +1623,16 @@ 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,

tests/testsuite/resolve.rs

@@ -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