Skip to content

Commit

Permalink
Auto merge of #5302 - Eh2406:MoreRc, r=alexcrichton
Browse files Browse the repository at this point in the history
use more Rc in the part of resolver that gets cloned a lot 2

This is the same idea as #5118, I was looking at a profile and noted that ~5% of our time was sent dropping `HashMap<PackageId, HashSet<InternedString>>`. A quick rg and I found the culprit, we are cloning the set of features for every new `Context`. With some Rc we are now just cloning for each time we insert.

To benchmark I commented out line https://github.com/rust-lang/cargo/blob/b9aa315158fe4d8d63672a49200401922ef7385d/src/cargo/core/resolver/mod.rs#L453
the smallest change to get #4810 (comment) not to solve instantly.
Before 17000000 ticks, 90s, 188.889 ticks/ms
After 17000000 ticks, 73s, 232.877 ticks/ms
  • Loading branch information
bors committed Apr 6, 2018
2 parents b9aa315 + 04fbc5f commit d83f113
Showing 1 changed file with 7 additions and 8 deletions.
15 changes: 7 additions & 8 deletions src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub struct Context {
// 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<PackageId, HashSet<InternedString>>,
pub resolve_features: HashMap<PackageId, Rc<HashSet<InternedString>>>,
pub links: HashMap<InternedString, PackageId>,

// These are two cheaply-cloneable lists (O(1) clone) which are effectively
Expand All @@ -36,6 +36,8 @@ pub struct Context {
pub warnings: RcList<String>,
}

pub type Activations = HashMap<(InternedString, SourceId), Rc<Vec<Summary>>>;

impl Context {
pub fn new() -> Context {
Context {
Expand Down Expand Up @@ -66,9 +68,7 @@ impl Context {
&*link
);
}
let mut inner: Vec<_> = (**prev).clone();
inner.push(summary.clone());
*prev = Rc::new(inner);
Rc::make_mut(prev).push(summary.clone());
return Ok(false);
}
debug!("checking if {} is already activated", summary.package_id());
Expand Down Expand Up @@ -244,9 +244,10 @@ impl Context {
if !reqs.used.is_empty() {
let pkgid = s.package_id();

let set = self.resolve_features
let set = Rc::make_mut(self.resolve_features
.entry(pkgid.clone())
.or_insert_with(HashSet::new);
.or_insert_with(|| Rc::new(HashSet::new())));

for feature in reqs.used {
set.insert(InternedString::new(feature));
}
Expand Down Expand Up @@ -405,5 +406,3 @@ impl<'r> Requirements<'r> {
}
}
}

pub type Activations = HashMap<(InternedString, SourceId), Rc<Vec<Summary>>>;

0 comments on commit d83f113

Please sign in to comment.