Auto merge of #5619 - Eh2406:remove_redundant_hashmap, r=alexcrichton

Remove redundant hashmap

We have two structures keeping track of parshall lists of `conflicting_prev_active`. I believe that they are always merged with each other before being used. (CI will tell me if I am wrong.) So in this PR we us pass a `&mut` so that it is merged as it gos. This saves us a clone/allocation of a hashmap for every tick, although most of the time one or both are empty so I doubt it will be a big performance change.

Keeping the comments clear in this code is very important. @alexcrichton, are there new comments that should go in with this change or more that should come out?

Author: bors

src/cargo/core/interning.rs

@@ -1,7 +1,7 @@
 use serde::{Serialize, Serializer};
 
 use std::fmt;
-use std::sync::RwLock;
+use std::sync::Mutex;
 use std::collections::HashSet;
 use std::str;
 use std::ptr;
@@ -14,8 +14,8 @@ pub fn leak(s: String) -> &'static str {
 }
 
 lazy_static! {
-    static ref STRING_CACHE: RwLock<HashSet<&'static str>> =
-        RwLock::new(HashSet::new());
+    static ref STRING_CACHE: Mutex<HashSet<&'static str>> =
+        Mutex::new(HashSet::new());
 }
 
 #[derive(Clone, Copy)]
@@ -33,7 +33,7 @@ impl Eq for InternedString {}
 
 impl InternedString {
     pub fn new(str: &str) -> InternedString {
-        let mut cache = STRING_CACHE.write().unwrap();
+        let mut cache = STRING_CACHE.lock().unwrap();
         let s = cache.get(str).map(|&s| s).unwrap_or_else(|| {
             let s = leak(str.to_string());
             cache.insert(s);

src/cargo/core/resolver/mod.rs

@@ -54,9 +54,9 @@ use std::time::{Duration, Instant};
 
 use semver;
 
-use core::{Dependency, PackageId, Registry, Summary};
-use core::PackageIdSpec;
 use core::interning::InternedString;
+use core::PackageIdSpec;
+use core::{Dependency, PackageId, Registry, Summary};
 use util::config::Config;
 use util::errors::{CargoError, CargoResult};
 use util::profile;
@@ -70,9 +70,9 @@ pub use self::encode::{Metadata, WorkspaceResolve};
 pub use self::resolve::{Deps, DepsNotReplaced, Resolve};
 pub use self::types::Method;
 
+mod conflict_cache;
 mod context;
 mod encode;
-mod conflict_cache;
 mod resolve;
 mod types;
 
@@ -293,9 +293,9 @@ fn activate_deps_loop(
         let mut backtracked = false;
 
         loop {
-            let next = remaining_candidates.next(&cx, &dep);
+            let next = remaining_candidates.next(&mut conflicting_activations, &cx, &dep);
 
-            let (candidate, has_another) = next.or_else(|conflicting| {
+            let (candidate, has_another) = next.ok_or(()).or_else(|_| {
                 // If we get here then our `remaining_candidates` was just
                 // exhausted, so `dep` failed to activate.
                 //
@@ -304,10 +304,6 @@ fn activate_deps_loop(
                 // candidates whatsoever then it's time to bail entirely.
                 trace!("{}[{}]>{} -- no candidates", parent.name(), cur, dep.name());
 
-                // Add all the reasons to our frame's list of conflicting
-                // activations, as we may use this to start backtracking later.
-                conflicting_activations.extend(conflicting);
-
                 // Use our list of `conflicting_activations` to add to our
                 // global list of past conflicting activations, effectively
                 // globally poisoning `dep` if `conflicting_activations` ever
@@ -402,13 +398,7 @@ fn activate_deps_loop(
                 dep.name(),
                 candidate.summary.version()
             );
-            let res = activate(
-                &mut cx,
-                registry,
-                Some((&parent, &dep)),
-                candidate,
-                &method,
-            );
+            let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, &method);
 
             let successfully_activated = match res {
                 // Success! We've now activated our `candidate` in our context
@@ -522,8 +512,6 @@ fn activate_deps_loop(
                     // we'll want to present an error message for sure.
                     let activate_for_error_message = has_past_conflicting_dep && !has_another && {
                         just_here_for_the_error_messages || {
-                            conflicting_activations
-                                .extend(remaining_candidates.conflicting_prev_active.clone());
                             find_candidate(
                                 &mut backtrack_stack.clone(),
                                 &parent,
@@ -692,13 +680,10 @@ struct BacktrackFrame {
 /// is defined within a `Context`.
 ///
 /// Candidates passed to `new` may not be returned from `next` as they could be
-/// filtered out, and if iteration stops a map of all packages which caused
-/// filtered out candidates to be filtered out will be returned.
+/// filtered out, and as they are filtered the causes will be added to `conflicting_prev_active`.
 #[derive(Clone)]
 struct RemainingCandidates {
     remaining: RcVecIter<Candidate>,
-    // note: change to RcList or something if clone is to expensive
-    conflicting_prev_active: HashMap<PackageId, ConflictReason>,
     // This is a inlined peekable generator
     has_another: Option<Candidate>,
 }
@@ -707,7 +692,6 @@ impl RemainingCandidates {
     fn new(candidates: &Rc<Vec<Candidate>>) -> RemainingCandidates {
         RemainingCandidates {
             remaining: RcVecIter::new(Rc::clone(candidates)),
-            conflicting_prev_active: HashMap::new(),
             has_another: None,
         }
     }
@@ -730,9 +714,10 @@ impl RemainingCandidates {
     /// original list for the reason listed.
     fn next(
         &mut self,
+        conflicting_prev_active: &mut HashMap<PackageId, ConflictReason>,
         cx: &Context,
         dep: &Dependency,
-    ) -> Result<(Candidate, bool), HashMap<PackageId, ConflictReason>> {
+    ) -> Option<(Candidate, bool)> {
         let prev_active = cx.prev_active(dep);
 
         for (_, b) in self.remaining.by_ref() {
@@ -743,9 +728,9 @@ impl RemainingCandidates {
             if let Some(link) = b.summary.links() {
                 if let Some(a) = cx.links.get(&link) {
                     if a != b.summary.package_id() {
-                        self.conflicting_prev_active
+                        conflicting_prev_active
                             .entry(a.clone())
-                            .or_insert_with(|| ConflictReason::Links(link.to_string()));
+                            .or_insert(ConflictReason::Links(link));
                         continue;
                     }
                 }
@@ -764,7 +749,7 @@ impl RemainingCandidates {
                 .find(|a| compatible(a.version(), b.summary.version()))
             {
                 if *a != b.summary {
-                    self.conflicting_prev_active
+                    conflicting_prev_active
                         .entry(a.package_id().clone())
                         .or_insert(ConflictReason::Semver);
                     continue;
@@ -777,21 +762,14 @@ impl RemainingCandidates {
             // get returned later, and if we replaced something then that was
             // actually the candidate to try first so we return that.
             if let Some(r) = mem::replace(&mut self.has_another, Some(b)) {
-                return Ok((r, true));
+                return Some((r, true));
             }
         }
 
         // Alright we've entirely exhausted our list of candidates. If we've got
         // something stashed away return that here (also indicating that there's
-        // nothing else). If nothing is stashed away we return the list of all
-        // conflicting activations, if any.
-        //
-        // TODO: can the `conflicting_prev_active` clone be avoided here? should
-        //       panic if this is called twice and an error is already returned
-        self.has_another
-            .take()
-            .map(|r| (r, false))
-            .ok_or_else(|| self.conflicting_prev_active.clone())
+        // nothing else).
+        self.has_another.take().map(|r| (r, false))
     }
 }
 
@@ -831,12 +809,14 @@ fn find_candidate(
     conflicting_activations: &HashMap<PackageId, ConflictReason>,
 ) -> Option<(Candidate, bool, BacktrackFrame)> {
     while let Some(mut frame) = backtrack_stack.pop() {
-        let next = frame
-            .remaining_candidates
-            .next(&frame.context_backup, &frame.dep);
+        let next = frame.remaining_candidates.next(
+            &mut frame.conflicting_activations,
+            &frame.context_backup,
+            &frame.dep,
+        );
         let (candidate, has_another) = match next {
-            Ok(pair) => pair,
-            Err(_) => continue,
+            Some(pair) => pair,
+            None => continue,
         };
         // When we're calling this method we know that `parent` failed to
         // activate. That means that some dependency failed to get resolved for

src/cargo/core/resolver/types.rs

@@ -279,7 +279,7 @@ pub enum ConflictReason {
     /// The `links` key is being violated. For example one crate in the
     /// dependency graph has `links = "foo"` but this crate also had that, and
     /// we're only allowed one per dependency graph.
-    Links(String),
+    Links(InternedString),
 
     /// A dependency listed features that weren't actually available on the
     /// candidate. For example we tried to activate feature `foo` but the