Auto merge of #6366 - Eh2406:faster-filtered-search, r=alexcrichton

ConflictStoreTrie: Faster filtered search

This is an optimization that I was thinking of doing in #6283. I did not then as this is optimizing a not particularly hot path. I wish to do it now as it is stuck in my head, and I need that head space for more important things.

This also "accidentally" fixes the [indeterminacy](#6283 (comment)) introduced in #6283, by replacing the `HashMap.iter().find()` like code with `BTreeMap.iter().find()` like code. This is not strictly needed, as @alexcrichton pointed out (In most use cases the index will change between any two invocations anyway), but does make it much easier to deal with (fuzz) test cases.

Author: bors

src/cargo/core/resolver/conflict_cache.rs

@@ -11,36 +11,39 @@ enum ConflictStoreTrie {
     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>),
+    Node(BTreeMap<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>(
+    fn find_conflicting(
         &self,
         cx: &Context,
-        filter: &F,
-    ) -> Option<&BTreeMap<PackageId, ConflictReason>>
-    where
-        for<'r> F: Fn(&'r &BTreeMap<PackageId, ConflictReason>) -> bool,
-    {
+        must_contain: Option<PackageId>,
+    ) -> Option<&BTreeMap<PackageId, ConflictReason>> {
         match self {
             ConflictStoreTrie::Leaf(c) => {
-                if filter(&c) {
+                if must_contain.is_none() {
                     // 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 {
+                    // we did not find `must_contain` so we need to keep looking.
                     None
                 }
             }
             ConflictStoreTrie::Node(m) => {
-                for (&pid, store) in m {
+                for (&pid, store) in must_contain
+                    .map(|f| m.range(..=f))
+                    .unwrap_or_else(|| m.range(..))
+                {
                     // 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) {
+                        if let Some(o) =
+                            store.find_conflicting(cx, must_contain.filter(|&f| f != pid))
+                        {
                             return Some(o);
                         }
                     } // else, if it is not active then there is no way any of the corresponding
@@ -59,7 +62,7 @@ impl ConflictStoreTrie {
         if let Some(pid) = iter.next() {
             if let ConflictStoreTrie::Node(p) = self {
                 p.entry(pid)
-                    .or_insert_with(|| ConflictStoreTrie::Node(HashMap::new()))
+                    .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new()))
                     .insert(iter, con);
             } // else, We already have a subset of this in the ConflictStore
         } else {
@@ -134,23 +137,31 @@ impl ConflictCache {
     }
     /// Finds any known set of conflicts, if any,
     /// which are activated in `cx` and pass the `filter` specified?
-    pub fn find_conflicting<F>(
+    pub fn find_conflicting(
         &self,
         cx: &Context,
         dep: &Dependency,
-        filter: F,
-    ) -> Option<&BTreeMap<PackageId, ConflictReason>>
-    where
-        for<'r> F: Fn(&'r &BTreeMap<PackageId, ConflictReason>) -> bool,
-    {
-        self.con_from_dep.get(dep)?.find_conflicting(cx, &filter)
+        must_contain: Option<PackageId>,
+    ) -> Option<&BTreeMap<PackageId, ConflictReason>> {
+        let out = self
+            .con_from_dep
+            .get(dep)?
+            .find_conflicting(cx, must_contain);
+        if cfg!(debug_assertions) {
+            if let Some(f) = must_contain {
+                if let Some(c) = &out {
+                    assert!(c.contains_key(&f));
+                }
+            }
+        }
+        out
     }
     pub fn conflicting(
         &self,
         cx: &Context,
         dep: &Dependency,
     ) -> Option<&BTreeMap<PackageId, ConflictReason>> {
-        self.find_conflicting(cx, dep, |_| true)
+        self.find_conflicting(cx, dep, None)
     }
 
     /// Add to the cache a conflict of the form:
@@ -159,7 +170,7 @@ impl ConflictCache {
     pub fn insert(&mut self, dep: &Dependency, con: &BTreeMap<PackageId, ConflictReason>) {
         self.con_from_dep
             .entry(dep.clone())
-            .or_insert_with(|| ConflictStoreTrie::Node(HashMap::new()))
+            .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new()))
             .insert(con.keys().cloned(), con.clone());
 
         trace!(

src/cargo/core/resolver/mod.rs

@@ -442,9 +442,7 @@ fn activate_deps_loop(
                                 })
                                 .filter_map(|(other_parent, other_dep)| {
                                     past_conflicting_activations
-                                        .find_conflicting(&cx, &other_dep, |con| {
-                                            con.contains_key(&pid)
-                                        })
+                                        .find_conflicting(&cx, &other_dep, Some(pid))
                                         .map(|con| (other_parent, con))
                                 })
                                 .next()

tests/testsuite/resolve.rs

@@ -1201,3 +1201,43 @@ fn large_conflict_cache() {
     let reg = registry(input);
     let _ = resolve(&pkg_id("root"), root_deps, &reg);
 }
+
+#[test]
+fn conflict_store_bug() {
+    let input = vec![
+        pkg!(("A", "0.0.3")),
+        pkg!(("A", "0.0.5")),
+        pkg!(("A", "0.0.9") => [dep("bad"),]),
+        pkg!(("A", "0.0.10") => [dep("bad"),]),
+        pkg!(("L-sys", "0.0.1") => [dep("bad"),]),
+        pkg!(("L-sys", "0.0.5")),
+        pkg!(("R", "0.0.4") => [
+            dep_req("L-sys", "= 0.0.5"),
+        ]),
+        pkg!(("R", "0.0.6")),
+        pkg!(("a-sys", "0.0.5")),
+        pkg!(("a-sys", "0.0.11")),
+        pkg!(("c", "0.0.12") => [
+            dep_req("R", ">= 0.0.3, <= 0.0.4"),
+        ]),
+        pkg!(("c", "0.0.13") => [
+            dep_req("a-sys", ">= 0.0.8, <= 0.0.11"),
+        ]),
+        pkg!(("c0", "0.0.6") => [
+            dep_req("L-sys", "<= 0.0.2"),
+        ]),
+        pkg!(("c0", "0.0.10") => [
+            dep_req("A", ">= 0.0.9, <= 0.0.10"),
+            dep_req("a-sys", "= 0.0.5"),
+        ]),
+        pkg!("j" => [
+            dep_req("A", ">= 0.0.3, <= 0.0.5"),
+            dep_req("R", ">=0.0.4, <= 0.0.6"),
+            dep_req("c", ">= 0.0.9"),
+            dep_req("c0", ">= 0.0.6"),
+        ]),
+    ];
+
+    let reg = registry(input.clone());
+    let _ = resolve_and_validated(&pkg_id("root"), vec![dep("j")], &reg);
+}

tests/testsuite/support/resolver.rs

@@ -355,8 +355,13 @@ pub fn registry_strategy(
 ) -> impl Strategy<Value = PrettyPrintRegistry> {
     let name = string_regex("[A-Za-z][A-Za-z0-9_-]*(-sys)?").unwrap();
 
-    let raw_version = [..max_versions; 3];
-    let version_from_raw = |v: &[usize; 3]| format!("{}.{}.{}", v[0], v[1], v[2]);
+    let raw_version = ..max_versions.pow(3);
+    let version_from_raw = move |r: usize| {
+        let major = ((r / max_versions) / max_versions) % max_versions;
+        let minor = (r / max_versions) % max_versions;
+        let patch = r % max_versions;
+        format!("{}.{}.{}", major, minor, patch)
+    };
 
     // If this is false than the crate will depend on the nonexistent "bad"
     // instead of the complex set we generated for it.
@@ -365,7 +370,7 @@ pub fn registry_strategy(
     let list_of_versions =
         btree_map(raw_version, allow_deps, 1..=max_versions).prop_map(move |ver| {
             ver.into_iter()
-                .map(|a| (version_from_raw(&a.0), a.1))
+                .map(|a| (version_from_raw(a.0), a.1))
                 .collect::<Vec<_>>()
         });