-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #5213 - Eh2406:faster_resolver, r=alexcrichton
Faster resolver: use a inverse-index to not activate the causes of conflict This adds a test for #4810 (comment) with two extensions that make it harder. It is the last reproducible and in the wild exponentially slow resolution (that I have found). The problem in the test is `backtrack_trap0 = "*"` is a lot of ways of saying `constrained = ">=1.1.0, <=2.0.0"` but `constrained= "2.0.1"` is already picked. Only then to try and solve `constrained= "~1.0.0"` which is incompatible. Our parent knows that we have been poisoned, and wont try to activate us again. Because of the order we evaluate deps we end up backtracking to where `constrained: 1.1.0` is set instead of our parent. And so the poisoning does not help. This is harder then #4810 (comment) because: 1. Having multiple semver compatible versions of constrained in play makes for a lot more bookkeeping. Specifically bookkeeping I forgot when I first submitted this PR. 2. The problematic dependencies are added deep in a combinatorial explosion of possibilities. So if we don't correctly handle caching that `backtrack_trap0 = "*"` is doomed then we will never finish looking thru the different possibilities for `level0 = "*"` This PR also includes a proof of concept solution for the test, which proves that it does solve #4810 (comment). The added code is tricky to read. It also adds a `O(remaining_deps)` job to every activation on the happy path, slower if the `past_conflicting_activations` is not empty. I'd like some brainstorming on better solutions.
- Loading branch information
Showing
3 changed files
with
269 additions
and
57 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| use std::collections::{HashMap, HashSet}; | ||
|
|
||
| use core::{Dependency, PackageId}; | ||
| use core::resolver::{ConflictReason, Context}; | ||
|
|
||
| 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}, | ||
| // ]; | ||
| // | ||
| // 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}, | ||
| // ]; | ||
| // | ||
| // 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! | ||
| // | ||
| // 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>>>, | ||
| // `past_conflict_triggers` is an | ||
| // of `past_conflicting_activations`. | ||
| // For every `PackageId` this lists the `Dependency`s that mention it in `past_conflicting_activations`. | ||
| dep_from_pid: HashMap<PackageId, HashSet<Dependency>>, | ||
| } | ||
|
|
||
| impl ConflictCache { | ||
| pub fn new() -> ConflictCache { | ||
| ConflictCache { | ||
| con_from_dep: HashMap::new(), | ||
| dep_from_pid: HashMap::new(), | ||
| } | ||
| } | ||
| /// 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, | ||
| dep: &Dependency, | ||
| filter: F, | ||
| ) -> Option<&HashMap<PackageId, ConflictReason>> | ||
| where | ||
| for<'r> F: FnMut(&'r &HashMap<PackageId, ConflictReason>) -> bool, | ||
| { | ||
| self.con_from_dep | ||
| .get(dep)? | ||
| .iter() | ||
| .filter(filter) | ||
| .find(|conflicting| cx.is_conflicting(None, conflicting)) | ||
| } | ||
| pub fn conflicting( | ||
| &self, | ||
| cx: &Context, | ||
| dep: &Dependency, | ||
| ) -> Option<&HashMap<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 | ||
| .entry(dep.clone()) | ||
| .or_insert_with(Vec::new); | ||
| if !past.contains(con) { | ||
| trace!("{} adding a skip {:?}", dep.name(), 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()); | ||
| } | ||
| } | ||
| } | ||
| pub fn dependencies_conflicting_with(&self, pid: &PackageId) -> Option<&HashSet<Dependency>> { | ||
| self.dep_from_pid.get(pid) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters