Skip to content

Commit

Permalink
Auto merge of #6776 - Eh2406:resolver-simplification, r=alexcrichton
Browse files Browse the repository at this point in the history
Resolver: A dep is equivalent to one of the things it can resolve to.

This is a series of small changes to the resolver, each one on its own is not worth the cherne, but somehow all together we can add a new optimization rule. The result is that the test in #6283 is no longer exponencial (still a large polynomial, cubick?) and so N can be bumped from 3 to 20. This also means that we pass with all the cases reported in #6258. Resolution is NP-Hard, so we are moving the slow cases around. To reduce the chance that we will be flooded by new bad cases I run the 4 proptests overnight, and they did not find a new exponencial case.

I would recommend reviewing this commit by commit. As each change is pretty simple on its own, but the mixed diff is harder to follow. This is submitted as one big PR as that is @alexcrichton's preference.

A special thanks to @nex3, our conversation was important in convincing me that several of these changes would be needed even in an eventual PubGrub based system. And, the question "why would PubGrub not have a problem with #6283" was wat crystallized this optimization opportunity in my mind.
  • Loading branch information
bors committed Apr 2, 2019
2 parents 6bdb9d3 + 91b5a9d commit c866f48
Show file tree
Hide file tree
Showing 6 changed files with 339 additions and 112 deletions.
71 changes: 61 additions & 10 deletions src/cargo/core/resolver/conflict_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,41 +18,55 @@ enum ConflictStoreTrie {

impl ConflictStoreTrie {
/// Finds any known set of conflicts, if any,
/// which are activated in `cx` and pass the `filter` specified?
/// which are activated in `cx` and contain `PackageId` specified.
/// If more then one are activated, then it will return
/// one that will allow for the most jump-back.
fn find_conflicting(
&self,
cx: &Context,
must_contain: Option<PackageId>,
) -> Option<&ConflictMap> {
) -> Option<(&ConflictMap, usize)> {
match self {
ConflictStoreTrie::Leaf(c) => {
if must_contain.is_none() {
// `is_conflicting` checks that all the elements are active,
// but we have checked each one by the recursion of this function.
debug_assert!(cx.is_conflicting(None, c));
Some(c)
debug_assert!(cx.is_conflicting(None, c).is_some());
Some((c, 0))
} else {
// We did not find `must_contain`, so we need to keep looking.
None
}
}
ConflictStoreTrie::Node(m) => {
let mut out = None;
for (&pid, store) in must_contain
.map(|f| m.range(..=f))
.unwrap_or_else(|| m.range(..))
{
// If the key is active, then we need to check all of the corresponding subtrie.
if cx.is_active(pid) {
if let Some(o) =
if let Some(age_this) = cx.is_active(pid) {
if let Some((o, age_o)) =
store.find_conflicting(cx, must_contain.filter(|&f| f != pid))
{
return Some(o);
let age = if must_contain == Some(pid) {
// all the results will include `must_contain`
// so the age of must_contain is not relevant to find the best result.
age_o
} else {
std::cmp::max(age_this, age_o)
};
let out_age = out.get_or_insert((o, age)).1;
if out_age > age {
// we found one that can jump-back further so replace the out.
out = Some((o, age));
}
}
}
// Else, if it is not active then there is no way any of the corresponding
// subtrie will be conflicting.
}
None
out
}
}
}
Expand Down Expand Up @@ -88,6 +102,28 @@ impl ConflictStoreTrie {
*self = ConflictStoreTrie::Leaf(con)
}
}

fn contains(&self, mut iter: impl Iterator<Item = PackageId>, con: &ConflictMap) -> bool {
match (self, iter.next()) {
(ConflictStoreTrie::Leaf(c), None) => {
if cfg!(debug_assertions) {
let a: Vec<_> = con.keys().collect();
let b: Vec<_> = c.keys().collect();
assert_eq!(a, b);
}
true
}
(ConflictStoreTrie::Leaf(_), Some(_)) => false,
(ConflictStoreTrie::Node(_), None) => false,
(ConflictStoreTrie::Node(m), Some(n)) => {
if let Some(next) = m.get(&n) {
next.contains(iter, con)
} else {
false
}
}
}
}
}

pub(super) struct ConflictCache {
Expand Down Expand Up @@ -137,7 +173,9 @@ impl ConflictCache {
}
}
/// Finds any known set of conflicts, if any,
/// which are activated in `cx` and pass the `filter` specified?
/// which are activated in `cx` and contain `PackageId` specified.
/// If more then one are activated, then it will return
/// one that will allow for the most jump-back.
pub fn find_conflicting(
&self,
cx: &Context,
Expand All @@ -147,7 +185,8 @@ impl ConflictCache {
let out = self
.con_from_dep
.get(dep)?
.find_conflicting(cx, must_contain);
.find_conflicting(cx, must_contain)
.map(|(c, _)| c);
if cfg!(debug_assertions) {
if let Some(f) = must_contain {
if let Some(c) = &out {
Expand Down Expand Up @@ -189,6 +228,18 @@ impl ConflictCache {
.insert(dep.clone());
}
}

/// Check if a conflict was previously added of the form:
/// `dep` is known to be unresolvable if
/// all the `PackageId` entries are activated.
pub fn contains(&self, dep: &Dependency, con: &ConflictMap) -> bool {
if let Some(cst) = self.con_from_dep.get(dep) {
cst.contains(con.keys().cloned(), &con)
} else {
false
}
}

pub fn dependencies_conflicting_with(&self, pid: PackageId) -> Option<&HashSet<Dependency>> {
self.dep_from_pid.get(&pid)
}
Expand Down
112 changes: 82 additions & 30 deletions src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::collections::{HashMap, HashSet};
use std::num::NonZeroU64;
use std::rc::Rc;

// "ensure" seems to require "bail" be in scope (macro hygiene issue?).
Expand Down Expand Up @@ -52,8 +53,47 @@ pub struct Context {
pub warnings: RcList<String>,
}

/// list all the activated versions of a particular crate name from a source
pub type Activations = im_rc::HashMap<(InternedString, SourceId), Rc<Vec<Summary>>>;
/// When backtracking it can be useful to know how far back to go.
/// The `ContextAge` of a `Context` is a monotonically increasing counter of the number
/// of decisions made to get to this state.
/// Several structures store the `ContextAge` when it was added,
/// to be used in `find_candidate` for backtracking.
pub type ContextAge = usize;

/// Find the activated version of a crate based on the name, source, and semver compatibility.
/// By storing this in a hash map we ensure that there is only one
/// semver compatible version of each crate.
/// This all so stores the `ContextAge`.
pub type Activations =
im_rc::HashMap<(InternedString, SourceId, SemverCompatibility), (Summary, ContextAge)>;

/// A type that represents when cargo treats two Versions as compatible.
/// Versions `a` and `b` are compatible if their left-most nonzero digit is the
/// same.
#[derive(Clone, Copy, Eq, PartialEq, Hash)]
pub enum SemverCompatibility {
Major(NonZeroU64),
Minor(NonZeroU64),
Patch(u64),
}

impl From<&semver::Version> for SemverCompatibility {
fn from(ver: &semver::Version) -> Self {
if let Some(m) = NonZeroU64::new(ver.major) {
return SemverCompatibility::Major(m);
}
if let Some(m) = NonZeroU64::new(ver.minor) {
return SemverCompatibility::Minor(m);
}
SemverCompatibility::Patch(ver.patch)
}
}

impl PackageId {
pub fn as_activations_key(&self) -> (InternedString, SourceId, SemverCompatibility) {
(self.name(), self.source_id(), self.version().into())
}
}

impl Context {
pub fn new(check_public_visible_dependencies: bool) -> Context {
Expand All @@ -78,22 +118,28 @@ impl Context {
/// Returns `true` if this summary with the given method is already activated.
pub fn flag_activated(&mut self, summary: &Summary, method: &Method<'_>) -> CargoResult<bool> {
let id = summary.package_id();
let prev = self
.activations
.entry((id.name(), id.source_id()))
.or_insert_with(|| Rc::new(Vec::new()));
if !prev.iter().any(|c| c == summary) {
self.resolve_graph.push(GraphNode::Add(id));
if let Some(link) = summary.links() {
ensure!(
self.links.insert(link, id).is_none(),
"Attempting to resolve a dependency with more then one crate with the \
links={}.\nThis will not build as is. Consider rebuilding the .lock file.",
&*link
let age: ContextAge = self.age();
match self.activations.entry(id.as_activations_key()) {
im_rc::hashmap::Entry::Occupied(o) => {
debug_assert_eq!(
&o.get().0,
summary,
"cargo does not allow two semver compatible versions"
);
}
Rc::make_mut(prev).push(summary.clone());
return Ok(false);
im_rc::hashmap::Entry::Vacant(v) => {
self.resolve_graph.push(GraphNode::Add(id));
if let Some(link) = summary.links() {
ensure!(
self.links.insert(link, id).is_none(),
"Attempting to resolve a dependency with more then one crate with the \
links={}.\nThis will not build as is. Consider rebuilding the .lock file.",
&*link
);
}
v.insert((summary.clone(), age));
return Ok(false);
}
}
debug!("checking if {} is already activated", summary.package_id());
let (features, use_default) = match *method {
Expand Down Expand Up @@ -149,31 +195,37 @@ impl Context {
Ok(deps)
}

pub fn prev_active(&self, dep: &Dependency) -> &[Summary] {
self.activations
.get(&(dep.package_name(), dep.source_id()))
.map(|v| &v[..])
.unwrap_or(&[])
/// Returns the `ContextAge` of this `Context`.
/// For now we use (len of activations) as the age.
/// See the `ContextAge` docs for more details.
pub fn age(&self) -> ContextAge {
self.activations.len()
}

pub fn is_active(&self, id: PackageId) -> bool {
/// If the package is active returns the `ContextAge` when it was added
pub fn is_active(&self, id: PackageId) -> Option<ContextAge> {
self.activations
.get(&(id.name(), id.source_id()))
.map(|v| v.iter().any(|s| s.package_id() == id))
.unwrap_or(false)
.get(&id.as_activations_key())
.and_then(|(s, l)| if s.package_id() == id { Some(*l) } else { None })
}

/// Checks whether all of `parent` and the keys of `conflicting activations`
/// are still active.
/// If so returns the `ContextAge` when the newest one was added.
pub fn is_conflicting(
&self,
parent: Option<PackageId>,
conflicting_activations: &ConflictMap,
) -> bool {
conflicting_activations
.keys()
.chain(parent.as_ref())
.all(|&id| self.is_active(id))
) -> Option<usize> {
let mut max = 0;
for &id in conflicting_activations.keys().chain(parent.as_ref()) {
if let Some(age) = self.is_active(id) {
max = std::cmp::max(max, age);
} else {
return None;
}
}
Some(max)
}

/// Returns all dependencies and the features we want from them.
Expand Down
Loading

0 comments on commit c866f48

Please sign in to comment.