remove the O(n) lookup

Author: Eh2406

src/cargo/core/resolver/conflict_cache.rs

@@ -1,40 +1,81 @@
-use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 
 use super::types::ConflictReason;
 use core::resolver::Context;
 use core::{Dependency, PackageId};
 
+enum ConflictStore {
+    Con(BTreeMap<PackageId, ConflictReason>),
+    Map(HashMap<PackageId, ConflictStore>),
+}
+
+impl ConflictStore {
+    /// Finds any known set of conflicts, if any,
+    /// which are activated in `cx` and pass the `filter` specified?
+    pub 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 {
+            ConflictStore::Con(c) => {
+                if filter(&c) {
+                    Some(c)
+                } else {
+                    None
+                }
+            }
+            ConflictStore::Map(m) => {
+                for (pid, store) in m {
+                    if cx.is_active(pid) {
+                        if let Some(o) = store.find_conflicting(cx, filter) {
+                            return Some(o);
+                        }
+                    }
+                }
+                None
+            }
+        }
+    }
+}
+
 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 a witch entry's 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, BTreeSet<BTreeMap<PackageId, ConflictReason>>>,
+    con_from_dep: HashMap<Dependency, ConflictStore>,
     // `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>>,
@@ -56,14 +97,9 @@ impl ConflictCache {
         filter: F,
     ) -> Option<&BTreeMap<PackageId, ConflictReason>>
     where
-        for<'r> F: FnMut(&'r &BTreeMap<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,
@@ -77,25 +113,43 @@ impl ConflictCache {
     /// `dep` is known to be unresolvable if
     /// all the `PackageId` entries are activated
     pub fn insert(&mut self, dep: &Dependency, con: &BTreeMap<PackageId, ConflictReason>) {
-        let past = self
+        let mut past = self
             .con_from_dep
             .entry(dep.clone())
-            .or_insert_with(BTreeSet::new);
-        if !past.contains(con) {
-            trace!(
-                "{} = \"{}\" adding a skip {:?}",
-                dep.package_name(),
-                dep.version_req(),
-                con
-            );
-            past.insert(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(|| ConflictStore::Map(HashMap::new()));
+
+        for pid in con.keys() {
+            match past {
+                ConflictStore::Con(_) => {
+                    // We already have a subset of this in the ConflictStore
+                    return;
+                }
+                ConflictStore::Map(p) => {
+                    past = p
+                        .entry(pid.clone())
+                        .or_insert_with(|| ConflictStore::Map(HashMap::new()));
+                }
             }
         }
+
+        if let ConflictStore::Con(_) = past {
+            // We already have this in the ConflictStore
+            return;
+        }
+        *past = ConflictStore::Con(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>> {
         self.dep_from_pid.get(pid)

src/cargo/core/resolver/context.rs

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

src/cargo/lib.rs

@@ -1,5 +1,5 @@
 #![cfg_attr(test, deny(warnings))]
-
+#![feature(nll)]
 // Clippy isn't enforced by CI, and know that @alexcrichton isn't a fan :)
 #![cfg_attr(feature = "cargo-clippy", allow(boxed_local))]             // bug rust-lang-nursery/rust-clippy#1123
 #![cfg_attr(feature = "cargo-clippy", allow(cyclomatic_complexity))]   // large project

tests/testsuite/resolve.rs

@@ -1175,8 +1175,8 @@ fn hard_equality() {
 
 #[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 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;