Skip to content

Commit

Permalink
Auto merge of #5112 - Eh2406:cache_queries, r=alexcrichton
Browse files Browse the repository at this point in the history 
Cache the query result.

Small performance gain.

In a test on #4810 (comment)
Before we got to 1700000 ticks in ~97 sec
After we got to 1700000 ticks in ~92 sec. I just reran and got ~82, so it seems to be unstable.
And query disappears from the flame graph
  • Loading branch information
@bors
bors committed Mar 3, 2018
2 parents 4429f4e + cb0df8e commit adadfab
Show file tree
Hide file tree
Showing 3 changed files with 125 additions and 103 deletions.
8 changes: 4 additions & 4 deletions src/cargo/core/dependency.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ use util::errors::{CargoResult, CargoResultExt, CargoError};

/// Information about a dependency requested by a Cargo manifest.
/// Cheap to copy.
#[derive(PartialEq, Clone, Debug)]
#[derive(PartialEq, Eq, Hash, Ord, PartialOrd, Clone, Debug)]
pub struct Dependency {
inner: Rc<Inner>,
}

/// The data underlying a Dependency.
#[derive(PartialEq, Clone, Debug)]
#[derive(PartialEq, Eq, Hash, Ord, PartialOrd, Clone, Debug)]
struct Inner {
name: String,
source_id: SourceId,
Expand All @@ -38,7 +38,7 @@ struct Inner {
platform: Option<Platform>,
}

#[derive(Clone, Debug, PartialEq)]
#[derive(Eq, PartialEq, Hash, Ord, PartialOrd, Clone, Debug)]
pub enum Platform {
Name(String),
Cfg(CfgExpr),
Expand Down Expand Up @@ -76,7 +76,7 @@ impl ser::Serialize for Dependency {
}
}

#[derive(PartialEq, Clone, Debug, Copy)]
#[derive(PartialEq, Eq, Hash, Ord, PartialOrd, Clone, Debug, Copy)]
pub enum Kind {
Normal,
Development,
Expand Down
216 changes: 119 additions & 97 deletions src/cargo/core/resolver/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ enum GraphNode {
// risk of being cloned *a lot* so we want to make this as cheap to clone as
// possible.
#[derive(Clone)]
struct Context<'a> {
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.
Expand All @@ -340,8 +340,6 @@ struct Context<'a> {
resolve_graph: RcList<GraphNode>,
resolve_replacements: RcList<(PackageId, PackageId)>,

replacements: &'a [(PackageIdSpec, Dependency)],

// These warnings are printed after resolution.
warnings: RcList<String>,
}
Expand All @@ -360,11 +358,10 @@ pub fn resolve(summaries: &[(Summary, Method)],
links: HashMap::new(),
resolve_replacements: RcList::new(),
activations: HashMap::new(),
replacements,
warnings: RcList::new(),
};
let _p = profile::start("resolving");
let cx = activate_deps_loop(cx, registry, summaries, config)?;
let cx = activate_deps_loop(cx, &mut RegistryQueryer::new(registry, replacements), summaries, config)?;

let mut resolve = Resolve {
graph: cx.graph(),
Expand Down Expand Up @@ -410,7 +407,7 @@ pub fn resolve(summaries: &[(Summary, Method)],
/// If `candidate` was activated, this function returns the dependency frame to
/// iterate through next.
fn activate(cx: &mut Context,
registry: &mut Registry,
registry: &mut RegistryQueryer,
parent: Option<&Summary>,
candidate: Candidate,
method: &Method)
Expand Down Expand Up @@ -573,10 +570,112 @@ impl ConflictReason {
}
}

struct RegistryQueryer<'a> {
registry: &'a mut (Registry + 'a),
replacements: &'a [(PackageIdSpec, Dependency)],
// TODO: with nll the Rc can be removed
cache: HashMap<Dependency, Rc<Vec<Candidate>>>,
}

impl<'a> RegistryQueryer<'a> {
fn new(registry: &'a mut Registry, replacements: &'a [(PackageIdSpec, Dependency)],) -> Self {
RegistryQueryer {
registry,
replacements,
cache: HashMap::new(),
}
}

/// Queries the `registry` to return a list of candidates for `dep`.
///
/// This method is the location where overrides are taken into account. If
/// any candidates are returned which match an override then the override is
/// applied by performing a second query for what the override should
/// return.
fn query(&mut self, dep: &Dependency) -> CargoResult<Rc<Vec<Candidate>>> {
if let Some(out) = self.cache.get(dep).cloned() {
return Ok(out);
}

let mut ret = Vec::new();
self.registry.query(dep, &mut |s| {
ret.push(Candidate { summary: s, replace: None });
})?;
for candidate in ret.iter_mut() {
let summary = &candidate.summary;

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.name(), dep.version_req());

let mut summaries = self.registry.query_vec(dep)?.into_iter();
let s = summaries.next().ok_or_else(|| {
format_err!("no matching package for override `{}` found\n\
location searched: {}\n\
version required: {}",
spec, dep.source_id(), dep.version_req())
})?;
let summaries = summaries.collect::<Vec<_>>();
if !summaries.is_empty() {
let bullets = summaries.iter().map(|s| {
format!(" * {}", s.package_id())
}).collect::<Vec<_>>();
bail!("the replacement specification `{}` matched \
multiple packages:\n * {}\n{}", spec, s.package_id(),
bullets.join("\n"));
}

// The dependency should be hard-coded to have the same name and an
// exact version requirement, so both of these assertions should
// never fail.
assert_eq!(s.version(), summary.version());
assert_eq!(s.name(), summary.name());

let replace = if s.source_id() == summary.source_id() {
debug!("Preventing\n{:?}\nfrom replacing\n{:?}", summary, s);
None
} else {
Some(s)
};
let matched_spec = spec.clone();

// Make sure no duplicates
if let Some(&(ref spec, _)) = potential_matches.next() {
bail!("overlapping replacement specifications found:\n\n \
* {}\n * {}\n\nboth specifications match: {}",
matched_spec, spec, summary.package_id());
}

for dep in summary.dependencies() {
debug!("\t{} => {}", dep.name(), dep.version_req());
}

candidate.replace = replace;
}

// When we attempt versions for a package, we'll want to start at
// the maximum version and work our way down.
ret.sort_unstable_by(|a, b| {
b.summary.version().cmp(a.summary.version())
});

let out = Rc::new(ret);

self.cache.insert(dep.clone(), out.clone());

Ok(out)
}
}

#[derive(Clone)]
struct BacktrackFrame<'a> {
struct BacktrackFrame {
cur: usize,
context_backup: Context<'a>,
context_backup: Context,
deps_backup: BinaryHeap<DepsFrame>,
remaining_candidates: RemainingCandidates,
parent: Summary,
Expand Down Expand Up @@ -658,12 +757,12 @@ impl RemainingCandidates {
///
/// If all dependencies can be activated and resolved to a version in the
/// dependency graph, cx.resolve is returned.
fn activate_deps_loop<'a>(
mut cx: Context<'a>,
registry: &mut Registry,
fn activate_deps_loop(
mut cx: Context,
registry: &mut RegistryQueryer,
summaries: &[(Summary, Method)],
config: Option<&Config>,
) -> CargoResult<Context<'a>> {
) -> 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.
Expand Down Expand Up @@ -780,7 +879,7 @@ fn activate_deps_loop<'a>(
).ok_or_else(|| {
activation_error(
&cx,
registry,
registry.registry,
&parent,
&dep,
&conflicting_activations,
Expand Down Expand Up @@ -850,9 +949,9 @@ fn activate_deps_loop<'a>(
/// If the outcome could differ, resets `cx` and `remaining_deps` to that
/// level and returns the next candidate.
/// If all candidates have been exhausted, returns None.
fn find_candidate<'a>(
backtrack_stack: &mut Vec<BacktrackFrame<'a>>,
cx: &mut Context<'a>,
fn find_candidate(
backtrack_stack: &mut Vec<BacktrackFrame>,
cx: &mut Context,
remaining_deps: &mut BinaryHeap<DepsFrame>,
parent: &mut Summary,
cur: &mut usize,
Expand Down Expand Up @@ -1176,7 +1275,7 @@ fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method)
Ok(reqs)
}

impl<'a> Context<'a> {
impl Context {
/// Activate this summary by inserting it into our list of known activations.
///
/// Returns true if this summary with the given method is already activated.
Expand Down Expand Up @@ -1219,7 +1318,7 @@ impl<'a> Context<'a> {
}

fn build_deps(&mut self,
registry: &mut Registry,
registry: &mut RegistryQueryer,
parent: Option<&Summary>,
candidate: &Summary,
method: &Method) -> ActivateResult<Vec<DepInfo>> {
Expand All @@ -1231,13 +1330,8 @@ impl<'a> Context<'a> {
// Next, transform all dependencies into a list of possible candidates
// which can satisfy that dependency.
let mut deps = deps.into_iter().map(|(dep, features)| {
let mut candidates = self.query(registry, &dep)?;
// When we attempt versions for a package, we'll want to start at
// the maximum version and work our way down.
candidates.sort_by(|a, b| {
b.summary.version().cmp(a.summary.version())
});
Ok((dep, Rc::new(candidates), Rc::new(features)))
let candidates = registry.query(&dep)?;
Ok((dep, candidates, Rc::new(features)))
}).collect::<CargoResult<Vec<DepInfo>>>()?;

// Attempt to resolve dependencies with fewer candidates before trying
Expand All @@ -1249,78 +1343,6 @@ impl<'a> Context<'a> {
Ok(deps)
}

/// Queries the `registry` to return a list of candidates for `dep`.
///
/// This method is the location where overrides are taken into account. If
/// any candidates are returned which match an override then the override is
/// applied by performing a second query for what the override should
/// return.
fn query(&self,
registry: &mut Registry,
dep: &Dependency) -> CargoResult<Vec<Candidate>> {
let mut ret = Vec::new();
registry.query(dep, &mut |s| {
ret.push(Candidate { summary: s, replace: None });
})?;
for candidate in ret.iter_mut() {
let summary = &candidate.summary;

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.name(), dep.version_req());

let mut summaries = registry.query_vec(dep)?.into_iter();
let s = summaries.next().ok_or_else(|| {
format_err!("no matching package for override `{}` found\n\
location searched: {}\n\
version required: {}",
spec, dep.source_id(), dep.version_req())
})?;
let summaries = summaries.collect::<Vec<_>>();
if !summaries.is_empty() {
let bullets = summaries.iter().map(|s| {
format!(" * {}", s.package_id())
}).collect::<Vec<_>>();
bail!("the replacement specification `{}` matched \
multiple packages:\n * {}\n{}", spec, s.package_id(),
bullets.join("\n"));
}

// The dependency should be hard-coded to have the same name and an
// exact version requirement, so both of these assertions should
// never fail.
assert_eq!(s.version(), summary.version());
assert_eq!(s.name(), summary.name());

let replace = if s.source_id() == summary.source_id() {
debug!("Preventing\n{:?}\nfrom replacing\n{:?}", summary, s);
None
} else {
Some(s)
};
let matched_spec = spec.clone();

// Make sure no duplicates
if let Some(&(ref spec, _)) = potential_matches.next() {
bail!("overlapping replacement specifications found:\n\n \
* {}\n * {}\n\nboth specifications match: {}",
matched_spec, spec, summary.package_id());
}

for dep in summary.dependencies() {
debug!("\t{} => {}", dep.name(), dep.version_req());
}

candidate.replace = replace;
}
Ok(ret)
}

fn prev_active(&self, dep: &Dependency) -> &[Summary] {
self.activations.get(dep.name())
.and_then(|v| v.get(dep.source_id()))
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/util/cfg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ use std::fmt;

use util::{CargoError, CargoResult};

#[derive(Clone, PartialEq, Debug)]
#[derive(Eq, PartialEq, Hash, Ord, PartialOrd, Clone, Debug)]
pub enum Cfg {
Name(String),
KeyPair(String, String),
}

#[derive(Clone, PartialEq, Debug)]
#[derive(Eq, PartialEq, Hash, Ord, PartialOrd, Clone, Debug)]
pub enum CfgExpr {
Not(Box<CfgExpr>),
All(Vec<CfgExpr>),
Expand Down

0 comments on commit adadfab

Please sign in to comment.