Auto merge of #6097 - Eh2406:refactoring, r=alexcrichton

bors · bors · commit 0253f4e91341 · 2018-09-26T05:26:07.000Z
small refactor

This moves the visual indicator code out of the most complicated loop in the resolver. The `activate_deps_loop` is the heart of the resolver, with all its moving pieces in play at the same time. It is completely overwhelming. This is a true refactoring, nothing about the executable has changed, but a screen full of code/comments got moved to a helper type in a different file.

I'd love to see more moved out of `activate_deps_loop` but this is the thing that came most cleanly.

edit: I was also able to move the `pop_most_constrained` code out as well. So that is now also in this PR.
diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
@@ -47,7 +47,7 @@
 //! that we're implementing something that probably shouldn't be allocating all
 //! over the place.
 
-use std::collections::{BTreeMap, BinaryHeap, HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::mem;
 use std::rc::Rc;
 use std::time::{Duration, Instant};
@@ -64,7 +64,7 @@ use util::profile;
 
 use self::context::{Activations, Context};
 use self::types::{ActivateError, ActivateResult, Candidate, ConflictReason, DepsFrame, GraphNode};
-use self::types::{RcVecIter, RegistryQueryer};
+use self::types::{RcVecIter, RegistryQueryer, RemainingDeps, ResolverProgress};
 
 pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use self::encode::{Metadata, WorkspaceResolve};
@@ -170,15 +170,8 @@ fn activate_deps_loop(
     summaries: &[(Summary, Method)],
     config: Option<&Config>,
 ) -> CargoResult<Context> {
-    // Note that a `BinaryHeap` is used for the remaining dependencies that need
-    // activation. This heap is sorted such that the "largest value" is the most
-    // constrained dependency, or the one with the least candidates.
-    //
-    // This helps us get through super constrained portions of the dependency
-    // graph quickly and hopefully lock down what later larger dependencies can
-    // use (those with more candidates).
     let mut backtrack_stack = Vec::new();
-    let mut remaining_deps = BinaryHeap::new();
+    let mut remaining_deps = RemainingDeps::new();
 
     // `past_conflicting_activations` is a cache of the reasons for each time we
     // backtrack.
@@ -200,11 +193,7 @@ fn activate_deps_loop(
         }
     }
 
-    let mut ticks = 0u16;
-    let start = Instant::now();
-    let time_to_print = Duration::from_millis(500);
-    let mut printed = false;
-    let mut deps_time = Duration::new(0, 0);
+    let mut printed = ResolverProgress::new();
 
     // Main resolution loop, this is the workhorse of the resolution algorithm.
     //
@@ -219,55 +208,14 @@ fn activate_deps_loop(
     // its own dependencies in turn. The `backtrack_stack` is a side table of
     // backtracking states where if we hit an error we can return to in order to
     // attempt to continue resolving.
-    while let Some(mut deps_frame) = remaining_deps.pop() {
-        // If we spend a lot of time here (we shouldn't in most cases) then give
-        // a bit of a visual indicator as to what we're doing. Only enable this
-        // when stderr is a tty (a human is likely to be watching) to ensure we
-        // get deterministic output otherwise when observed by tools.
-        //
-        // Also note that we hit this loop a lot, so it's fairly performance
-        // sensitive. As a result try to defer a possibly expensive operation
-        // like `Instant::now` by only checking every N iterations of this loop
-        // to amortize the cost of the current time lookup.
-        ticks += 1;
-        if let Some(config) = config {
-            if config.shell().is_err_tty()
-                && !printed
-                && ticks % 1000 == 0
-                && start.elapsed() - deps_time > time_to_print
-            {
-                printed = true;
-                config.shell().status("Resolving", "dependency graph...")?;
-            }
-        }
-        // The largest test in our sweet takes less then 5000 ticks
-        // with all the algorithm improvements.
-        // If any of them are removed then it takes more than I am willing to measure.
-        // So lets fail the test fast if we have ben running for two long.
-        debug_assert!(ticks < 50_000);
-        // The largest test in our sweet takes less then 30 sec
-        // with all the improvements to how fast a tick can go.
-        // If any of them are removed then it takes more than I am willing to measure.
-        // So lets fail the test fast if we have ben running for two long.
-        if cfg!(debug_assertions) && (ticks % 1000 == 0) {
-            assert!(start.elapsed() - deps_time < Duration::from_secs(90));
-        }
-
-        let just_here_for_the_error_messages = deps_frame.just_for_error_messages;
-
-        // Figure out what our next dependency to activate is, and if nothing is
-        // listed then we're entirely done with this frame (yay!) and we can
-        // move on to the next frame.
-        let frame = match deps_frame.remaining_siblings.next() {
-            Some(sibling) => {
-                let parent = Summary::clone(&deps_frame.parent);
-                remaining_deps.push(deps_frame);
-                (parent, sibling)
-            }
-            None => continue,
-        };
+    while let Some((just_here_for_the_error_messages, frame)) =
+        remaining_deps.pop_most_constrained()
+    {
         let (mut parent, (mut cur, (mut dep, candidates, mut features))) = frame;
-        assert!(!remaining_deps.is_empty());
+
+        // If we spend a lot of time here (we shouldn't in most cases) then give
+        // a bit of a visual indicator as to what we're doing.
+        printed.shell_status(config)?;
 
         trace!(
             "{}[{}]>{} {} candidates",
@@ -398,7 +346,7 @@ fn activate_deps_loop(
                 Some(BacktrackFrame {
                     cur,
                     context_backup: Context::clone(&cx),
-                    deps_backup: <BinaryHeap<DepsFrame>>::clone(&remaining_deps),
+                    deps_backup: remaining_deps.clone(),
                     remaining_candidates: remaining_candidates.clone(),
                     parent: Summary::clone(&parent),
                     dep: Dependency::clone(&dep),
@@ -431,7 +379,7 @@ fn activate_deps_loop(
                 // frame in the end if it looks like it's not going to end well,
                 // so figure that out here.
                 Ok(Some((mut frame, dur))) => {
-                    deps_time += dur;
+                    printed.elapsed(dur);
 
                     // Our `frame` here is a new package with its own list of
                     // dependencies. Do a sanity check here of all those
@@ -486,7 +434,6 @@ fn activate_deps_loop(
                         {
                             if let Some((other_parent, conflict)) = remaining_deps
                                 .iter()
-                                .flat_map(|other| other.flatten())
                                 // for deps related to us
                                 .filter(|&(_, ref other_dep)| {
                                     known_related_bad_deps.contains(other_dep)
@@ -683,7 +630,7 @@ fn activate(
 struct BacktrackFrame {
     cur: usize,
     context_backup: Context,
-    deps_backup: BinaryHeap<DepsFrame>,
+    deps_backup: RemainingDeps,
     remaining_candidates: RemainingCandidates,
     parent: Summary,
     dep: Dependency,
diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs
@@ -1,11 +1,70 @@
 use std::cmp::Ordering;
-use std::collections::{HashMap, HashSet};
+use std::collections::{BinaryHeap, HashMap, HashSet};
 use std::ops::Range;
 use std::rc::Rc;
+use std::time::{Duration, Instant};
 
 use core::interning::InternedString;
 use core::{Dependency, PackageId, PackageIdSpec, Registry, Summary};
-use util::{CargoError, CargoResult};
+use util::{CargoError, CargoResult, Config};
+
+pub struct ResolverProgress {
+    ticks: u16,
+    start: Instant,
+    time_to_print: Duration,
+    printed: bool,
+    deps_time: Duration,
+}
+
+impl ResolverProgress {
+    pub fn new() -> ResolverProgress {
+        ResolverProgress {
+            ticks: 0,
+            start: Instant::now(),
+            time_to_print: Duration::from_millis(500),
+            printed: false,
+            deps_time: Duration::new(0, 0),
+        }
+    }
+    pub fn shell_status(&mut self, config: Option<&Config>) -> CargoResult<()> {
+        // If we spend a lot of time here (we shouldn't in most cases) then give
+        // a bit of a visual indicator as to what we're doing. Only enable this
+        // when stderr is a tty (a human is likely to be watching) to ensure we
+        // get deterministic output otherwise when observed by tools.
+        //
+        // Also note that we hit this loop a lot, so it's fairly performance
+        // sensitive. As a result try to defer a possibly expensive operation
+        // like `Instant::now` by only checking every N iterations of this loop
+        // to amortize the cost of the current time lookup.
+        self.ticks += 1;
+        if let Some(config) = config {
+            if config.shell().is_err_tty()
+                && !self.printed
+                && self.ticks % 1000 == 0
+                && self.start.elapsed() - self.deps_time > self.time_to_print
+            {
+                self.printed = true;
+                config.shell().status("Resolving", "dependency graph...")?;
+            }
+        }
+        // The largest test in our sweet takes less then 5000 ticks
+        // with all the algorithm improvements.
+        // If any of them are removed then it takes more than I am willing to measure.
+        // So lets fail the test fast if we have ben running for two long.
+        debug_assert!(self.ticks < 50_000);
+        // The largest test in our sweet takes less then 30 sec
+        // with all the improvements to how fast a tick can go.
+        // If any of them are removed then it takes more than I am willing to measure.
+        // So lets fail the test fast if we have ben running for two long.
+        if cfg!(debug_assertions) && (self.ticks % 1000 == 0) {
+            assert!(self.start.elapsed() - self.deps_time < Duration::from_secs(90));
+        }
+        Ok(())
+    }
+    pub fn elapsed(&mut self, dur: Duration) {
+        self.deps_time += dur;
+    }
+}
 
 pub struct RegistryQueryer<'a> {
     pub registry: &'a mut (Registry + 'a),
@@ -46,24 +105,33 @@ impl<'a> RegistryQueryer<'a> {
         }
 
         let mut ret = Vec::new();
-        self.registry.query(dep, &mut |s| {
-            ret.push(Candidate {
-                summary: s,
-                replace: None,
-            });
-        }, false)?;
+        self.registry.query(
+            dep,
+            &mut |s| {
+                ret.push(Candidate {
+                    summary: s,
+                    replace: None,
+                });
+            },
+            false,
+        )?;
         for candidate in ret.iter_mut() {
             let summary = &candidate.summary;
 
-            let mut potential_matches = self.replacements
+            let mut potential_matches = self
+                .replacements
                 .iter()
                 .filter(|&&(ref spec, _)| spec.matches(summary.package_id()));
 
             let &(ref spec, ref dep) = match potential_matches.next() {
                 None => continue,
                 Some(replacement) => replacement,
             };
-            debug!("found an override for {} {}", dep.package_name(), dep.version_req());
+            debug!(
+                "found an override for {} {}",
+                dep.package_name(),
+                dep.version_req()
+            );
 
             let mut summaries = self.registry.query_vec(dep, false)?.into_iter();
             let s = summaries.next().ok_or_else(|| {
@@ -204,7 +272,7 @@ impl DepsFrame {
             .unwrap_or(0)
     }
 
-    pub fn flatten<'s>(&'s self) -> impl Iterator<Item = (&PackageId, Dependency)> + 's {
+    pub fn flatten(&self) -> impl Iterator<Item = (&PackageId, Dependency)> {
         self.remaining_siblings
             .clone()
             .map(move |(_, (d, _, _))| (self.parent.package_id(), d))
@@ -238,6 +306,43 @@ impl Ord for DepsFrame {
     }
 }
 
+/// Note that a `BinaryHeap` is used for the remaining dependencies that need
+/// activation. This heap is sorted such that the "largest value" is the most
+/// constrained dependency, or the one with the least candidates.
+///
+/// This helps us get through super constrained portions of the dependency
+/// graph quickly and hopefully lock down what later larger dependencies can
+/// use (those with more candidates).
+#[derive(Clone)]
+pub struct RemainingDeps(BinaryHeap<DepsFrame>);
+
+impl RemainingDeps {
+    pub fn new() -> RemainingDeps {
+        RemainingDeps(BinaryHeap::new())
+    }
+    pub fn push(&mut self, x: DepsFrame) {
+        self.0.push(x)
+    }
+    pub fn pop_most_constrained(&mut self) -> Option<(bool, (Summary, (usize, DepInfo)))> {
+        while let Some(mut deps_frame) = self.0.pop() {
+            let just_here_for_the_error_messages = deps_frame.just_for_error_messages;
+
+            // Figure out what our next dependency to activate is, and if nothing is
+            // listed then we're entirely done with this frame (yay!) and we can
+            // move on to the next frame.
+            if let Some(sibling) = deps_frame.remaining_siblings.next() {
+                let parent = Summary::clone(&deps_frame.parent);
+                self.0.push(deps_frame);
+                return Some((just_here_for_the_error_messages, (parent, sibling)));
+            }
+        }
+        None
+    }
+    pub fn iter(&mut self) -> impl Iterator<Item = (&PackageId, Dependency)> {
+        self.0.iter().flat_map(|other| other.flatten())
+    }
+}
+
 // Information about the dependencies for a crate, a tuple of:
 //
 // (dependency info, candidates, features activated)
