From 5c244e1c4b88480df9c3f15026c33e8435430ce2 Mon Sep 17 00:00:00 2001 From: Aidan Hobson Sayers Date: Mon, 18 Dec 2017 17:06:45 +0000 Subject: [PATCH 1/4] Make resolution backtracking smarter There's no need to try every candidate for every dependency - instead, only try alternative candidates if they *could* change the eventual failure that caused backtracking in the first place. --- src/cargo/core/resolver/mod.rs | 23 +++++++++- tests/resolve.rs | 80 ++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 02a26a9f79c..3522df18b3e 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -745,8 +745,13 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, Ok(cx) } -// Searches up `backtrack_stack` until it finds a dependency with remaining -// candidates. Resets `cx` and `remaining_deps` to that level and returns the +// Looks through the states in `backtrack_stack` for dependencies with +// remaining candidates. For each one, also checks if rolling back +// could change the outcome of the failed resolution that caused backtracking +// in the first place - namely, if we've backtracked past the parent of the +// failed dep, or the previous number of activations of the failed dep has +// changed (possibly relaxing version constraints). If the outcome could differ, +// resets `cx` and `remaining_deps` to that level and returns the // next candidate. If all candidates have been exhausted, returns None. fn find_candidate<'a>(backtrack_stack: &mut Vec>, cx: &mut Context<'a>, @@ -755,12 +760,18 @@ fn find_candidate<'a>(backtrack_stack: &mut Vec>, cur: &mut usize, dep: &mut Dependency, features: &mut Rc>) -> Option { + let num_dep_prev_active = cx.prev_active(dep).len(); while let Some(mut frame) = backtrack_stack.pop() { let (next, has_another) = { let prev_active = frame.context_backup.prev_active(&frame.dep); (frame.remaining_candidates.next(prev_active), frame.remaining_candidates.clone().next(prev_active).is_some()) }; + let maychange = !frame.context_backup.is_active(parent) || + frame.context_backup.prev_active(dep).len() != num_dep_prev_active; + if !maychange { + continue + } if let Some(candidate) = next { *cur = frame.cur; if has_another { @@ -1178,6 +1189,14 @@ impl<'a> Context<'a> { .unwrap_or(&[]) } + fn is_active(&mut self, summary: &Summary) -> bool { + let id = summary.package_id(); + self.activations.get(id.name()) + .and_then(|v| v.get(id.source_id())) + .map(|v| v.iter().any(|s| s == summary)) + .unwrap_or(false) + } + /// Return all dependencies and the features we want from them. fn resolve_features<'b>(&mut self, s: &'b Summary, diff --git a/tests/resolve.rs b/tests/resolve.rs index 42a67dd37d0..411c3930068 100644 --- a/tests/resolve.rs +++ b/tests/resolve.rs @@ -74,6 +74,13 @@ impl ToPkgId for (&'static str, &'static str) { } } +impl ToPkgId for (&'static str, String) { + fn to_pkgid(&self) -> PackageId { + let (name, ref vers) = *self; + PackageId::new(name, vers, ®istry_loc()).unwrap() + } +} + macro_rules! pkg { ($pkgid:expr => [$($deps:expr),+]) => ({ let d: Vec = vec![$($deps.to_dep()),+]; @@ -364,6 +371,79 @@ fn resolving_with_deep_backtracking() { ("baz", "1.0.1")]))); } +#[test] +fn resolving_with_constrained_sibling_backtrack_parent() { + // There is no point in considering all of the backtrack_trap{1,2} + // candidates since they can't change the result of failing to + // resolve 'constrained'. Cargo should skip past them and resume + // resolution once the activation of the parent, 'bar', is rolled back. + let mut reglist = vec![ + pkg!(("foo", "1.0.0") => [dep_req("bar", "1.0"), + dep_req("constrained", "=1.0.0")]), + + pkg!(("bar", "1.0.0") => [dep_req("backtrack_trap1", "1.0.2"), + dep_req("backtrack_trap2", "1.0.2"), + dep_req("constrained", "1.0.0")]), + pkg!(("constrained", "1.0.0")), + pkg!(("backtrack_trap1", "1.0.0")), + pkg!(("backtrack_trap2", "1.0.0")), + ]; + for i in 1..50 { + let vsn = format!("1.0.{}", i); + reglist.push(pkg!(("bar", vsn.clone()) => [dep_req("backtrack_trap1", "1.0.2"), + dep_req("backtrack_trap2", "1.0.2"), + dep_req("constrained", "1.0.1")])); + reglist.push(pkg!(("backtrack_trap1", vsn.clone()))); + reglist.push(pkg!(("backtrack_trap2", vsn.clone()))); + reglist.push(pkg!(("constrained", vsn.clone()))); + } + let reg = registry(reglist); + + let res = resolve(&pkg_id("root"), vec![ + dep_req("foo", "1"), + ], ®).unwrap(); + + assert_that(&res, contains(names(&[("root", "1.0.0"), + ("foo", "1.0.0"), + ("bar", "1.0.0"), + ("constrained", "1.0.0")]))); +} + +#[test] +fn resolving_with_constrained_sibling_backtrack_activation() { + // It makes sense to resolve most-constrained deps first, but + // with that logic the backtrack traps here come between the two + // attempted resolutions of 'constrained'. When backtracking, + // cargo should skip past them and resume resolution once the + // number of activations for 'constrained' changes. + let mut reglist = vec![ + pkg!(("foo", "1.0.0") => [dep_req("bar", "=1.0.0"), + dep_req("backtrack_trap1", "1.0"), + dep_req("backtrack_trap2", "1.0"), + dep_req("constrained", "<=1.0.60")]), + pkg!(("bar", "1.0.0") => [dep_req("constrained", ">=1.0.60")]), + ]; + for i in 0..45 { + let vsn = format!("1.0.{}", i); + reglist.push(pkg!(("backtrack_trap1", vsn.clone()))); + reglist.push(pkg!(("backtrack_trap2", vsn.clone()))); + } + for i in 0..100 { + let vsn = format!("1.0.{}", i); + reglist.push(pkg!(("constrained", vsn.clone()))); + } + let reg = registry(reglist); + + let res = resolve(&pkg_id("root"), vec![ + dep_req("foo", "1"), + ], ®).unwrap(); + + assert_that(&res, contains(names(&[("root", "1.0.0"), + ("foo", "1.0.0"), + ("bar", "1.0.0"), + ("constrained", "1.0.60")]))); +} + #[test] fn resolving_but_no_exists() { let reg = registry(vec![ From b9c7f98ce9d3e866f0fc5d05e7208e548497c636 Mon Sep 17 00:00:00 2001 From: Aidan Hobson Sayers Date: Sat, 3 Feb 2018 21:24:22 +0000 Subject: [PATCH 2/4] Clearer desired behaviour with an assertion --- src/cargo/core/resolver/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 3522df18b3e..24dab364622 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -767,8 +767,11 @@ fn find_candidate<'a>(backtrack_stack: &mut Vec>, (frame.remaining_candidates.next(prev_active), frame.remaining_candidates.clone().next(prev_active).is_some()) }; + let cur_num_dep_prev_active = frame.context_backup.prev_active(dep).len(); + // Activations should monotonically decrease during backtracking + assert!(cur_num_dep_prev_active <= num_dep_prev_active); let maychange = !frame.context_backup.is_active(parent) || - frame.context_backup.prev_active(dep).len() != num_dep_prev_active; + cur_num_dep_prev_active != num_dep_prev_active; if !maychange { continue } From 034b93c4d1562f5efdf047ad06258c25f135937d Mon Sep 17 00:00:00 2001 From: Aidan Hobson Sayers Date: Sat, 3 Feb 2018 21:49:42 +0000 Subject: [PATCH 3/4] Elaborate on test configuration, add a new test --- tests/resolve.rs | 66 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 4 deletions(-) diff --git a/tests/resolve.rs b/tests/resolve.rs index 411c3930068..6e43528fa3f 100644 --- a/tests/resolve.rs +++ b/tests/resolve.rs @@ -375,8 +375,10 @@ fn resolving_with_deep_backtracking() { fn resolving_with_constrained_sibling_backtrack_parent() { // There is no point in considering all of the backtrack_trap{1,2} // candidates since they can't change the result of failing to - // resolve 'constrained'. Cargo should skip past them and resume + // resolve 'constrained'. Cargo should (ideally) skip past them and resume // resolution once the activation of the parent, 'bar', is rolled back. + // Note that the traps are slightly more constrained to make sure they + // get picked first. let mut reglist = vec![ pkg!(("foo", "1.0.0") => [dep_req("bar", "1.0"), dep_req("constrained", "=1.0.0")]), @@ -388,7 +390,10 @@ fn resolving_with_constrained_sibling_backtrack_parent() { pkg!(("backtrack_trap1", "1.0.0")), pkg!(("backtrack_trap2", "1.0.0")), ]; - for i in 1..50 { + // Bump this to make the test harder - it adds more versions of bar that will + // fail to resolve, and more versions of the traps to consider. + const NUM_BARS_AND_TRAPS: usize = 50; // minimum 2 + for i in 1..NUM_BARS_AND_TRAPS { let vsn = format!("1.0.{}", i); reglist.push(pkg!(("bar", vsn.clone()) => [dep_req("backtrack_trap1", "1.0.2"), dep_req("backtrack_trap2", "1.0.2"), @@ -423,12 +428,19 @@ fn resolving_with_constrained_sibling_backtrack_activation() { dep_req("constrained", "<=1.0.60")]), pkg!(("bar", "1.0.0") => [dep_req("constrained", ">=1.0.60")]), ]; - for i in 0..45 { + // Bump these to make the test harder, but you'll also need to + // change the version constraints on `constrained` above. To correctly + // exercise Cargo, the relationship between the values is: + // NUM_CONSTRAINED - vsn < NUM_TRAPS < vsn + // to make sure the traps are resolved between `constrained`. + const NUM_TRAPS: usize = 45; // min 1 + const NUM_CONSTRAINED: usize = 100; // min 1 + for i in 0..NUM_TRAPS { let vsn = format!("1.0.{}", i); reglist.push(pkg!(("backtrack_trap1", vsn.clone()))); reglist.push(pkg!(("backtrack_trap2", vsn.clone()))); } - for i in 0..100 { + for i in 0..NUM_CONSTRAINED { let vsn = format!("1.0.{}", i); reglist.push(pkg!(("constrained", vsn.clone()))); } @@ -444,6 +456,52 @@ fn resolving_with_constrained_sibling_backtrack_activation() { ("constrained", "1.0.60")]))); } +#[test] +fn resolving_with_constrained_sibling_transitive_dep_effects() { + // When backtracking due to a failed dependency, if Cargo is + // trying to be clever and skip irrelevant dependencies, care must + // be taken to not miss the transitive effects of alternatives. E.g. + // in the right-to-left resolution of the graph below, B may + // affect whether D is successfully resolved. + // + // A + // / | \ + // B C D + // | | + // C D + let reg = registry(vec![ + pkg!(("A", "1.0.0") => [dep_req("B", "1.0"), + dep_req("C", "1.0"), + dep_req("D", "1.0.100")]), + + pkg!(("B", "1.0.0") => [dep_req("C", ">=1.0.0")]), + pkg!(("B", "1.0.1") => [dep_req("C", ">=1.0.1")]), + + pkg!(("C", "1.0.0") => [dep_req("D", "1.0.0")]), + pkg!(("C", "1.0.1") => [dep_req("D", ">=1.0.1,<1.0.100")]), + pkg!(("C", "1.0.2") => [dep_req("D", ">=1.0.2,<1.0.100")]), + + pkg!(("D", "1.0.0")), + pkg!(("D", "1.0.1")), + pkg!(("D", "1.0.2")), + pkg!(("D", "1.0.100")), + pkg!(("D", "1.0.101")), + pkg!(("D", "1.0.102")), + pkg!(("D", "1.0.103")), + pkg!(("D", "1.0.104")), + pkg!(("D", "1.0.105")), + ]); + + let res = resolve(&pkg_id("root"), vec![ + dep_req("A", "1"), + ], ®).unwrap(); + + assert_that(&res, contains(names(&[("A", "1.0.0"), + ("B", "1.0.0"), + ("C", "1.0.0"), + ("D", "1.0.105")]))); +} + #[test] fn resolving_but_no_exists() { let reg = registry(vec![ From a85f9b6cf2142a5061ed4b4020be160444b5c49a Mon Sep 17 00:00:00 2001 From: Aidan Hobson Sayers Date: Tue, 6 Feb 2018 02:15:17 +0000 Subject: [PATCH 4/4] Add reference to clarifying comment --- src/cargo/core/resolver/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 24dab364622..7977e958aa2 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -753,6 +753,8 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, // changed (possibly relaxing version constraints). If the outcome could differ, // resets `cx` and `remaining_deps` to that level and returns the // next candidate. If all candidates have been exhausted, returns None. +// Read https://github.com/rust-lang/cargo/pull/4834#issuecomment-362871537 for +// a more detailed explanation of the logic here. fn find_candidate<'a>(backtrack_stack: &mut Vec>, cx: &mut Context<'a>, remaining_deps: &mut BinaryHeap,