diff --git a/src/cargo/core/interning.rs b/src/cargo/core/interning.rs index 10d44a93c6a..7c615056ae5 100644 --- a/src/cargo/core/interning.rs +++ b/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> = - RwLock::new(HashSet::new()); + static ref STRING_CACHE: Mutex> = + 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); diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 589b67e02f0..37ddf9691ef 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/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, - // note: change to RcList or something if clone is to expensive - conflicting_prev_active: HashMap, // This is a inlined peekable generator has_another: Option, } @@ -707,7 +692,6 @@ impl RemainingCandidates { fn new(candidates: &Rc>) -> 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, cx: &Context, dep: &Dependency, - ) -> Result<(Candidate, bool), HashMap> { + ) -> 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, ) -> 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 diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index f555d77fb99..2a70b97d34b 100644 --- a/src/cargo/core/resolver/types.rs +++ b/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