Use a ad hoc trie to avoid slow cases

src/cargo/core/resolver/conflict_cache.rs

@@ -1,40 +1,125 @@
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 
 use super::types::ConflictReason;
 use core::resolver::Context;
 use core::{Dependency, PackageId};
 
+/// This is a Trie for storing a large number of Sets designed to
+/// efficiently see if any of the stored Sets are a subset of a search Set.
+enum ConflictStoreTrie {
+    /// a Leaf is one of the stored Sets.
+    Leaf(BTreeMap<PackageId, ConflictReason>),
+    /// a Node is a map from an element to a subTrie where
+    /// all the Sets in the subTrie contains that element.
+    Node(HashMap<PackageId, ConflictStoreTrie>),
+}
+
+impl ConflictStoreTrie {
+    /// Finds any known set of conflicts, if any,
+    /// which are activated in `cx` and pass the `filter` specified?
+    fn find_conflicting<F>(
+        &self,
+        cx: &Context,
+        filter: &F,
+    ) -> Option<&BTreeMap<PackageId, ConflictReason>>
+    where
+        for<'r> F: Fn(&'r &BTreeMap<PackageId, ConflictReason>) -> bool,
+    {
+        match self {
+            ConflictStoreTrie::Leaf(c) => {
+                if filter(&c) {
+                    // is_conflicting checks that all the elements are active,
+                    // but we have checked each one by the recursion of this function.
+                    debug_assert!(cx.is_conflicting(None, c));
+                    Some(c)
+                } else {
+                    None
+                }
+            }
+            ConflictStoreTrie::Node(m) => {
+                for (pid, store) in m {
+                    // if the key is active then we need to check all of the corresponding subTrie.
+                    if cx.is_active(pid) {
+                        if let Some(o) = store.find_conflicting(cx, filter) {
+                            return Some(o);
+                        }
+                    } // else, if it is not active then there is no way any of the corresponding
+                    // subTrie will be conflicting.
+                }
+                None
+            }
+        }
+    }
+
+    fn insert<'a>(
+        &mut self,
+        mut iter: impl Iterator<Item = &'a PackageId>,
+        con: BTreeMap<PackageId, ConflictReason>,
+    ) {
+        if let Some(pid) = iter.next() {
+            if let ConflictStoreTrie::Node(p) = self {
+                p.entry(pid.clone())
+                    .or_insert_with(|| ConflictStoreTrie::Node(HashMap::new()))
+                    .insert(iter, con);
+            } // else, We already have a subset of this in the ConflictStore
+        } else {
+            // we are at the end of the set we are adding, there are 3 cases for what to do next:
+            // 1. self is a empty dummy Node inserted by `or_insert_with`
+            //      in witch case we should replace it with `Leaf(con)`.
+            // 2. self is a Node because we previously inserted a superset of
+            //      the thing we are working on (I don't know if this happens in practice)
+            //      but the subset that we are working on will
+            //      always match any time the larger set would have
+            //      in witch case we can replace it with `Leaf(con)`.
+            // 3. self is a Leaf that is in the same spot in the structure as
+            //      the thing we are working on. So it is equivalent.
+            //      We can replace it with `Leaf(con)`.
+            if cfg!(debug_assertions) {
+                if let ConflictStoreTrie::Leaf(c) = self {
+                    let a: Vec<_> = con.keys().collect();
+                    let b: Vec<_> = c.keys().collect();
+                    assert_eq!(a, b);
+                }
+            }
+            *self = ConflictStoreTrie::Leaf(con)
+        }
+    }
+}
+
 pub(super) struct ConflictCache {
     // `con_from_dep` is a cache of the reasons for each time we
     // backtrack. For example after several backtracks we may have:
     //
-    //  con_from_dep[`foo = "^1.0.2"`] = vec![
-    //      map!{`foo=1.0.1`: Semver},
-    //      map!{`foo=1.0.0`: Semver},
-    //  ];
+    //  con_from_dep[`foo = "^1.0.2"`] = map!{
+    //      `foo=1.0.1`: map!{`foo=1.0.1`: Semver},
+    //      `foo=1.0.0`: map!{`foo=1.0.0`: Semver},
+    //  };
     //
     // This can be read as "we cannot find a candidate for dep `foo = "^1.0.2"`
     // if either `foo=1.0.1` OR `foo=1.0.0` are activated".
     //
     // Another example after several backtracks we may have:
     //
-    //  con_from_dep[`foo = ">=0.8.2, <=0.9.3"`] = vec![
-    //      map!{`foo=0.8.1`: Semver, `foo=0.9.4`: Semver},
-    //  ];
+    //  con_from_dep[`foo = ">=0.8.2, <=0.9.3"`] = map!{
+    //      `foo=0.8.1`: map!{
+    //          `foo=0.9.4`: map!{`foo=0.8.1`: Semver, `foo=0.9.4`: Semver},
+    //      }
+    //  };
     //
     // This can be read as "we cannot find a candidate for dep `foo = ">=0.8.2,
     // <=0.9.3"` if both `foo=0.8.1` AND `foo=0.9.4` are activated".
     //
     // This is used to make sure we don't queue work we know will fail. See the
     // discussion in https://github.com/rust-lang/cargo/pull/5168 for why this
-    // is so important, and there can probably be a better data structure here
-    // but for now this works well enough!
+    // is so important. The nested HashMaps act as a kind of btree, that lets us
+    // look up which entries are still active without
+    // linearly scanning through the full list.
     //
     // Also, as a final note, this map is *not* ever removed from. This remains
     // as a global cache which we never delete from. Any entry in this map is
     // unconditionally true regardless of our resolution history of how we got
     // here.
-    con_from_dep: HashMap<Dependency, Vec<HashMap<PackageId, ConflictReason>>>,
+    con_from_dep: HashMap<Dependency, ConflictStoreTrie>,
     // `dep_from_pid` is an inverse-index of `con_from_dep`.
     // For every `PackageId` this lists the `Dependency`s that mention it in `dep_from_pid`.
     dep_from_pid: HashMap<PackageId, HashSet<Dependency>>,
@@ -54,47 +139,41 @@ impl ConflictCache {
         cx: &Context,
         dep: &Dependency,
         filter: F,
-    ) -> Option<&HashMap<PackageId, ConflictReason>>
+    ) -> Option<&BTreeMap<PackageId, ConflictReason>>
     where
-        for<'r> F: FnMut(&'r &HashMap<PackageId, ConflictReason>) -> bool,
+        for<'r> F: Fn(&'r &BTreeMap<PackageId, ConflictReason>) -> bool,
     {
-        self.con_from_dep
-            .get(dep)?
-            .iter()
-            .rev() // more general cases are normally found letter. So start the search there.
-            .filter(filter)
-            .find(|conflicting| cx.is_conflicting(None, conflicting))
+        self.con_from_dep.get(dep)?.find_conflicting(cx, &filter)
     }
     pub fn conflicting(
         &self,
         cx: &Context,
         dep: &Dependency,
-    ) -> Option<&HashMap<PackageId, ConflictReason>> {
+    ) -> Option<&BTreeMap<PackageId, ConflictReason>> {
         self.find_conflicting(cx, dep, |_| true)
     }
 
     /// Add to the cache a conflict of the form:
     /// `dep` is known to be unresolvable if
     /// all the `PackageId` entries are activated
-    pub fn insert(&mut self, dep: &Dependency, con: &HashMap<PackageId, ConflictReason>) {
-        let past = self
-            .con_from_dep
+    pub fn insert(&mut self, dep: &Dependency, con: &BTreeMap<PackageId, ConflictReason>) {
+        self.con_from_dep
             .entry(dep.clone())
-            .or_insert_with(Vec::new);
-        if !past.contains(con) {
-            trace!(
-                "{} = \"{}\" adding a skip {:?}",
-                dep.package_name(),
-                dep.version_req(),
-                con
-            );
-            past.push(con.clone());
-            for c in con.keys() {
-                self.dep_from_pid
-                    .entry(c.clone())
-                    .or_insert_with(HashSet::new)
-                    .insert(dep.clone());
-            }
+            .or_insert_with(|| ConflictStoreTrie::Node(HashMap::new()))
+            .insert(con.keys(), con.clone());
+
+        trace!(
+            "{} = \"{}\" adding a skip {:?}",
+            dep.package_name(),
+            dep.version_req(),
+            con
+        );
+
+        for c in con.keys() {
+            self.dep_from_pid
+                .entry(c.clone())
+                .or_insert_with(HashSet::new)
+                .insert(dep.clone());
         }
     }
     pub fn dependencies_conflicting_with(&self, pid: &PackageId) -> Option<&HashSet<Dependency>> {

src/cargo/core/resolver/context.rs

@@ -1,13 +1,13 @@
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::rc::Rc;
 
 use core::interning::InternedString;
 use core::{Dependency, FeatureValue, PackageId, SourceId, Summary};
 use util::CargoResult;
 use util::Graph;
 
-use super::types::{ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer};
 use super::errors::ActivateResult;
+use super::types::{ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer};
 
 pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use super::encode::{Metadata, WorkspaceResolve};
@@ -133,7 +133,7 @@ impl Context {
             .unwrap_or(&[])
     }
 
-    fn is_active(&self, id: &PackageId) -> bool {
+    pub fn is_active(&self, id: &PackageId) -> bool {
         self.activations
             .get(&(id.name(), id.source_id().clone()))
             .map(|v| v.iter().any(|s| s.package_id() == id))
@@ -145,7 +145,7 @@ impl Context {
     pub fn is_conflicting(
         &self,
         parent: Option<&PackageId>,
-        conflicting_activations: &HashMap<PackageId, ConflictReason>,
+        conflicting_activations: &BTreeMap<PackageId, ConflictReason>,
     ) -> bool {
         conflicting_activations
             .keys()
@@ -186,10 +186,11 @@ impl Context {
             // name.
             let base = reqs.deps.get(&dep.name_in_toml()).unwrap_or(&default_dep);
             used_features.insert(dep.name_in_toml());
-            let always_required = !dep.is_optional() && !s
-                .dependencies()
-                .iter()
-                .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
+            let always_required = !dep.is_optional()
+                && !s
+                    .dependencies()
+                    .iter()
+                    .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
             if always_required && base.0 {
                 self.warnings.push(format!(
                     "Package `{}` does not have feature `{}`. It has a required dependency \

src/cargo/core/resolver/errors.rs

@@ -1,4 +1,4 @@
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 use std::fmt;
 
 use core::{Dependency, PackageId, Registry, Summary};
@@ -73,7 +73,7 @@ pub(super) fn activation_error(
     registry: &mut Registry,
     parent: &Summary,
     dep: &Dependency,
-    conflicting_activations: &HashMap<PackageId, ConflictReason>,
+    conflicting_activations: &BTreeMap<PackageId, ConflictReason>,
     candidates: &[Candidate],
     config: Option<&Config>,
 ) -> ResolveError {

src/cargo/core/resolver/mod.rs

@@ -247,7 +247,7 @@ fn activate_deps_loop(
         //
         // This is a map of package id to a reason why that packaged caused a
         // conflict for us.
-        let mut conflicting_activations = HashMap::new();
+        let mut conflicting_activations = BTreeMap::new();
 
         // When backtracking we don't fully update `conflicting_activations`
         // especially for the cases that we didn't make a backtrack frame in the
@@ -641,7 +641,7 @@ struct BacktrackFrame {
     parent: Summary,
     dep: Dependency,
     features: Rc<Vec<InternedString>>,
-    conflicting_activations: HashMap<PackageId, ConflictReason>,
+    conflicting_activations: BTreeMap<PackageId, ConflictReason>,
 }
 
 /// A helper "iterator" used to extract candidates within a current `Context` of
@@ -688,7 +688,7 @@ impl RemainingCandidates {
     /// original list for the reason listed.
     fn next(
         &mut self,
-        conflicting_prev_active: &mut HashMap<PackageId, ConflictReason>,
+        conflicting_prev_active: &mut BTreeMap<PackageId, ConflictReason>,
         cx: &Context,
         dep: &Dependency,
     ) -> Option<(Candidate, bool)> {
@@ -781,7 +781,7 @@ fn find_candidate(
     backtrack_stack: &mut Vec<BacktrackFrame>,
     parent: &Summary,
     backtracked: bool,
-    conflicting_activations: &HashMap<PackageId, ConflictReason>,
+    conflicting_activations: &BTreeMap<PackageId, ConflictReason>,
 ) -> Option<(Candidate, bool, BacktrackFrame)> {
     while let Some(mut frame) = backtrack_stack.pop() {
         let next = frame.remaining_candidates.next(

tests/testsuite/resolve.rs

@@ -7,8 +7,8 @@ use cargo::util::Config;
 use support::project;
 use support::registry::Package;
 use support::resolver::{
-    assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names,
-    pkg, pkg_dep, pkg_id, pkg_loc, registry, registry_strategy, resolve, resolve_and_validated,
+    assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names, pkg, pkg_dep,
+    pkg_id, pkg_loc, registry, registry_strategy, resolve, resolve_and_validated,
     resolve_with_config, PrettyPrintRegistry, ToDep, ToPkgId,
 };
 
@@ -433,14 +433,12 @@ fn resolving_incompat_versions() {
         pkg!("bar" => [dep_req("foo", "=1.0.2")]),
     ]);
 
-    assert!(
-        resolve(
-            &pkg_id("root"),
-            vec![dep_req("foo", "=1.0.1"), dep("bar")],
-            &reg
-        )
-        .is_err()
-    );
+    assert!(resolve(
+        &pkg_id("root"),
+        vec![dep_req("foo", "=1.0.1"), dep("bar")],
+        &reg
+    )
+    .is_err());
 }
 
 #[test]
@@ -1174,3 +1172,32 @@ fn hard_equality() {
         &names(&[("root", "1.0.0"), ("foo", "1.0.0"), ("bar", "1.0.0")]),
     );
 }
+
+#[test]
+fn large_conflict_cache() {
+    let mut input = vec![
+        pkg!(("last", "0.0.0") => [dep("bad")]), // just to make sure last is less constrained
+    ];
+    let mut root_deps = vec![dep("last")];
+    const NUM_VERSIONS: u8 = 3;
+    for name in 0..=NUM_VERSIONS {
+        // 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")]));
+        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")]));
+        root_deps.push(dep_req(&plane_name, ">= 1.0.1"));
+
+        for i in 0..=NUM_VERSIONS {
+            input.push(pkg!((&sys_name, format!("{}.0.0", i))));
+            input.push(pkg!((&plane_name, format!("1.0.{}", i))));
+        }
+    }
+    let reg = registry(input);
+    let _ = resolve(&pkg_id("root"), root_deps, &reg);
+}