From a703851abe68c3e9b64db42a86e46387876eff71 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 21 Jun 2018 16:06:09 -0400 Subject: [PATCH 1/4] try im-rs --- Cargo.toml | 1 + src/cargo/core/resolver/context.rs | 16 +++++++--------- src/cargo/lib.rs | 1 + 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 8c3065fb162..2eb0a7d5894 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -59,6 +59,7 @@ url = "1.1" clap = "2.31.2" unicode-width = "0.1.5" openssl = { version = '0.10.11', optional = true } +im-rc = "12.1.0" # A noop dependency that changes in the Rust repository, it's a bit of a hack. # See the `src/tools/rustc-workspace-hack/README.md` file in `rust-lang/rust` diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 96fdfd40daf..4a6629f93c5 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -5,6 +5,7 @@ use core::interning::InternedString; use core::{Dependency, FeatureValue, PackageId, SourceId, Summary}; use util::CargoResult; use util::Graph; +use im_rc; use super::errors::ActivateResult; use super::types::{ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer}; @@ -19,12 +20,9 @@ pub use super::resolve::Resolve; // possible. #[derive(Clone)] pub struct Context { - // TODO: Both this and the two maps below are super expensive to clone. We should - // switch to persistent hash maps if we can at some point or otherwise - // make these much cheaper to clone in general. pub activations: Activations, - pub resolve_features: HashMap>>, - pub links: HashMap, + pub resolve_features: im_rc::HashMap>>, + pub links: im_rc::HashMap, // These are two cheaply-cloneable lists (O(1) clone) which are effectively // hash maps but are built up as "construction lists". We'll iterate these @@ -36,16 +34,16 @@ pub struct Context { pub warnings: RcList, } -pub type Activations = HashMap<(InternedString, SourceId), Rc>>; +pub type Activations = im_rc::HashMap<(InternedString, SourceId), Rc>>; impl Context { pub fn new() -> Context { Context { resolve_graph: RcList::new(), - resolve_features: HashMap::new(), - links: HashMap::new(), + resolve_features: im_rc::HashMap::new(), + links: im_rc::HashMap::new(), resolve_replacements: RcList::new(), - activations: HashMap::new(), + activations: im_rc::HashMap::new(), warnings: RcList::new(), } } diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index e7fcac41d24..8359faceabe 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -62,6 +62,7 @@ extern crate termcolor; extern crate toml; extern crate unicode_width; extern crate url; +extern crate im_rc; use std::fmt; From e3b44cc3258eab458b79bd45cf8e7a50c4105d4a Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 27 Sep 2018 15:50:25 -0400 Subject: [PATCH 2/4] use im-rs for RemainingDeps --- src/cargo/core/resolver/types.rs | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index 0f39783a99e..d916d7ad62c 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -1,5 +1,5 @@ use std::cmp::Ordering; -use std::collections::{BinaryHeap, HashMap, HashSet}; +use std::collections::{HashMap, HashSet}; use std::ops::Range; use std::rc::Rc; use std::time::{Duration, Instant}; @@ -9,6 +9,8 @@ use core::{Dependency, PackageId, PackageIdSpec, Registry, Summary}; use util::errors::CargoResult; use util::Config; +use im_rc; + pub struct ResolverProgress { ticks: u16, start: Instant, @@ -299,33 +301,30 @@ impl Ord for DepsFrame { fn cmp(&self, other: &DepsFrame) -> Ordering { self.just_for_error_messages .cmp(&other.just_for_error_messages) - .then_with(|| - // the frame with the sibling that has the least number of candidates - // needs to get bubbled up to the top of the heap we use below, so - // reverse comparison here. - self.min_candidates().cmp(&other.min_candidates()).reverse()) + .reverse() + .then_with(|| self.min_candidates().cmp(&other.min_candidates())) } } -/// 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. +/// Note that a `OrdSet` is used for the remaining dependencies that need +/// activation. This set is sorted by how many candidates each dependency has. /// /// 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); +pub struct RemainingDeps(usize, im_rc::OrdSet<(DepsFrame, usize)>); impl RemainingDeps { pub fn new() -> RemainingDeps { - RemainingDeps(BinaryHeap::new()) + RemainingDeps(0, im_rc::OrdSet::new()) } pub fn push(&mut self, x: DepsFrame) { - self.0.push(x) + self.1.insert((x, self.0)); + self.0 += 1; } pub fn pop_most_constrained(&mut self) -> Option<(bool, (Summary, (usize, DepInfo)))> { - while let Some(mut deps_frame) = self.0.pop() { + while let Some((mut deps_frame, i)) = self.1.remove_min() { 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 @@ -333,14 +332,14 @@ impl RemainingDeps { // 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); + self.1.insert((deps_frame, i)); return Some((just_here_for_the_error_messages, (parent, sibling))); } } None } pub fn iter(&mut self) -> impl Iterator { - self.0.iter().flat_map(|other| other.flatten()) + self.1.iter().flat_map(|(other, _)| other.flatten()) } } From fd06af4e0bf38e174dd27babffe2a5dc19c1ba16 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 4 Oct 2018 13:18:18 -0400 Subject: [PATCH 3/4] better error for benchmarking --- src/cargo/core/resolver/types.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index d916d7ad62c..db1570f3705 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -54,7 +54,11 @@ impl ResolverProgress { // 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); + debug_assert!( + self.ticks < 50_000, + "got to 50_000 ticks in {:?}", + self.start.elapsed() + ); // The largest test in our suite 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. From 1699a5b1ce197596dae286fd39d48e7b91b3ed5f Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 20 Nov 2018 15:52:52 -0500 Subject: [PATCH 4/4] Clarify the need for a counter --- src/cargo/core/resolver/types.rs | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index db1570f3705..6f940afbc3d 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -317,18 +317,29 @@ impl Ord for DepsFrame { /// graph quickly and hopefully lock down what later larger dependencies can /// use (those with more candidates). #[derive(Clone)] -pub struct RemainingDeps(usize, im_rc::OrdSet<(DepsFrame, usize)>); +pub struct RemainingDeps { + /// a monotonic counter, increased for each new insertion. + time: u32, + /// the data is augmented by the insertion time. + /// This insures that no two items will cmp eq. + /// Forcing the OrdSet into a multi set. + data: im_rc::OrdSet<(DepsFrame, u32)>, +} impl RemainingDeps { pub fn new() -> RemainingDeps { - RemainingDeps(0, im_rc::OrdSet::new()) + RemainingDeps { + time: 0, + data: im_rc::OrdSet::new(), + } } pub fn push(&mut self, x: DepsFrame) { - self.1.insert((x, self.0)); - self.0 += 1; + let insertion_time = self.time; + self.data.insert((x, insertion_time)); + self.time += 1; } pub fn pop_most_constrained(&mut self) -> Option<(bool, (Summary, (usize, DepInfo)))> { - while let Some((mut deps_frame, i)) = self.1.remove_min() { + while let Some((mut deps_frame, insertion_time)) = self.data.remove_min() { 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 @@ -336,14 +347,14 @@ impl RemainingDeps { // move on to the next frame. if let Some(sibling) = deps_frame.remaining_siblings.next() { let parent = Summary::clone(&deps_frame.parent); - self.1.insert((deps_frame, i)); + self.data.insert((deps_frame, insertion_time)); return Some((just_here_for_the_error_messages, (parent, sibling))); } } None } pub fn iter(&mut self) -> impl Iterator { - self.1.iter().flat_map(|(other, _)| other.flatten()) + self.data.iter().flat_map(|(other, _)| other.flatten()) } }