From 52012e016d4f28561a08364bce5205cc9c4426d7 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 18 Jul 2019 12:49:03 -0400 Subject: [PATCH 01/20] allow graph to store multiple edges between a pair of nodes --- Cargo.toml | 1 + crates/resolver-tests/src/lib.rs | 2 +- src/cargo/core/compiler/unit_dependencies.rs | 8 +- src/cargo/core/resolver/context.rs | 15 +- src/cargo/core/resolver/mod.rs | 26 ++- src/cargo/core/resolver/resolve.rs | 45 ++--- src/cargo/ops/cargo_fetch.rs | 4 +- src/cargo/util/graph.rs | 187 +++++++++++++++---- 8 files changed, 195 insertions(+), 93 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9b20fb766fc..aae7be46cea 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,6 +40,7 @@ hex = "0.4" home = "0.5" humantime = "1.2.0" ignore = "0.4.7" +indexmap = "1.0.2" lazy_static = "1.2.0" jobserver = "0.1.13" lazycell = "1.2.0" diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index c77a050758e..58a47f0e26c 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -70,7 +70,7 @@ pub fn resolve_and_validated( let mut self_pub_dep = HashSet::new(); self_pub_dep.insert(p); for (dp, deps) in resolve.deps(p) { - if deps.iter().any(|d| d.is_public()) { + if deps.clone().any(|d| d.is_public()) { self_pub_dep.extend(pub_deps[&dp].iter().cloned()) } } diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 085c41e97cc..25430426eaa 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -225,9 +225,9 @@ fn compute_deps<'a, 'cfg>( let bcx = state.bcx; let id = unit.pkg.package_id(); - let deps = state.resolve().deps(id).filter(|&(_id, deps)| { + let deps = state.resolve().deps(id).filter(|(_id, deps)| { assert!(!deps.is_empty()); - deps.iter().any(|dep| { + (*deps).clone().any(|dep| { // If this target is a build command, then we only want build // dependencies, otherwise we want everything *other than* build // dependencies. @@ -391,8 +391,8 @@ fn compute_deps_doc<'a, 'cfg>( let deps = state .resolve() .deps(unit.pkg.package_id()) - .filter(|&(_id, deps)| { - deps.iter().any(|dep| match dep.kind() { + .filter(|(_id, deps)| { + (*deps).clone().any(|dep| match dep.kind() { DepKind::Normal => bcx.dep_platform_activated(dep, unit.kind), _ => false, }) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 991b746e3c2..f2ef634868d 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -1,6 +1,5 @@ use std::collections::HashMap; use std::num::NonZeroU64; -use std::rc::Rc; use failure::format_err; use log::debug; @@ -35,7 +34,7 @@ pub struct Context { /// a way to look up for a package in activations what packages required it /// and all of the exact deps that it fulfilled. - pub parents: Graph>>, + pub parents: Graph, } /// When backtracking it can be useful to know how far back to go. @@ -224,17 +223,17 @@ impl Context { .collect() } - pub fn graph(&self) -> Graph> { - let mut graph: Graph> = Graph::new(); + pub fn graph(&self) -> Graph { + let mut graph: Graph = Graph::new(); self.activations .values() .for_each(|(r, _)| graph.add(r.package_id())); for i in self.parents.iter() { graph.add(*i); - for (o, e) in self.parents.edges(i) { - let old_link = graph.link(*o, *i); - assert!(old_link.is_empty()); - *old_link = e.to_vec(); + for (o, edges) in self.parents.edges(i) { + for e in edges { + graph.add_edge(*o, *i, e.clone()) + } } } graph diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 4aaa7eeafed..96fe10b1389 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -596,12 +596,8 @@ fn activate( let candidate_pid = candidate.package_id(); if let Some((parent, dep)) = parent { let parent_pid = parent.package_id(); - Rc::make_mut( - // add a edge from candidate to parent in the parents graph - cx.parents.link(candidate_pid, parent_pid), - ) - // and associate dep with that edge - .push(dep.clone()); + // add a edge from candidate to parent in the parents graph and associate dep with that edge + cx.parents.add_edge(candidate_pid, parent_pid, dep.clone()); if let Some(public_dependency) = cx.public_dependency.as_mut() { // one tricky part is that `candidate_pid` may already be active and // have public dependencies of its own. So we not only need to mark @@ -643,8 +639,8 @@ fn activate( // if `candidate_pid` was a private dependency of `p` then `p` parents can't see `c` thru `p` if public { // if it was public, then we add all of `p`s parents to be checked - for &(grand, ref d) in cx.parents.edges(&p) { - stack.push((grand, d.iter().any(|x| x.is_public()))); + for (&grand, mut d) in cx.parents.edges(&p) { + stack.push((grand, d.any(|x| x.is_public()))); } } } @@ -825,8 +821,8 @@ impl RemainingCandidates { // if `b` was a private dependency of `p` then `p` parents can't see `t` thru `p` if public { // if it was public, then we add all of `p`s parents to be checked - for &(grand, ref d) in cx.parents.edges(&p) { - stack.push((grand, d.iter().any(|x| x.is_public()))); + for (&grand, mut d) in cx.parents.edges(&p) { + stack.push((grand, d.any(|x| x.is_public()))); } } } @@ -885,12 +881,12 @@ fn generalize_conflicting( } // What parents does that critical activation have for (critical_parent, critical_parents_deps) in - cx.parents.edges(&backtrack_critical_id).filter(|(p, _)| { + cx.parents.edges(&backtrack_critical_id).filter(|(&p, _)| { // it will only help backjump further if it is older then the critical_age - cx.is_active(*p).expect("parent not currently active!?") < backtrack_critical_age + cx.is_active(p).expect("parent not currently active!?") < backtrack_critical_age }) { - for critical_parents_dep in critical_parents_deps.iter() { + for critical_parents_dep in critical_parents_deps { // A dep is equivalent to one of the things it can resolve to. // Thus, if all the things it can resolve to have already ben determined // to be conflicting, then we can just say that we conflict with the parent. @@ -1083,8 +1079,8 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> { // visitation list as we can't induce a cycle through transitive // dependencies. if checked.insert(id) { - for (dep, listings) in resolve.deps_not_replaced(id) { - let is_transitive = listings.iter().any(|d| d.is_transitive()); + for (dep, mut listings) in resolve.deps_not_replaced(id) { + let is_transitive = listings.any(|d| d.is_transitive()); let mut empty = HashSet::new(); let visited = if is_transitive { &mut *visited diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index dcb7cd44361..9b8b4e20bb5 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -1,12 +1,13 @@ use std::borrow::Borrow; use std::collections::{HashMap, HashSet}; use std::fmt; +use std::hash::Hash; use std::iter::FromIterator; use crate::core::dependency::Kind; use crate::core::{Dependency, PackageId, PackageIdSpec, Summary, Target}; use crate::util::errors::CargoResult; -use crate::util::Graph; +use crate::util::graph::{self, Graph}; use super::encode::Metadata; @@ -18,9 +19,9 @@ use super::encode::Metadata; #[derive(PartialEq)] pub struct Resolve { /// A graph, whose vertices are packages and edges are dependency specifications - /// from `Cargo.toml`. We need a `Vec` here because the same package + /// from `Cargo.toml`. We may have more then one here because the same package /// might be present in both `[dependencies]` and `[build-dependencies]`. - graph: Graph>, + graph: Graph, /// Replacements from the `[replace]` table. replacements: HashMap, /// Inverted version of `replacements`. @@ -67,7 +68,7 @@ pub enum ResolveVersion { impl Resolve { pub fn new( - graph: Graph>, + graph: Graph, replacements: HashMap, features: HashMap>, checksums: HashMap>, @@ -82,7 +83,8 @@ impl Resolve { let public_deps = graph .edges(p) .filter(|(_, deps)| { - deps.iter() + (*deps) + .clone() .any(|d| d.kind() == Kind::Normal && d.is_public()) }) .map(|(dep_package, _)| *dep_package) @@ -232,7 +234,7 @@ unable to verify that `{0}` is the same as when the lockfile was generated pub fn contains(&self, k: &Q) -> bool where PackageId: Borrow, - Q: Ord + Eq, + Q: Hash + Eq, { self.graph.contains(k) } @@ -245,7 +247,10 @@ unable to verify that `{0}` is the same as when the lockfile was generated self.graph.iter().cloned() } - pub fn deps(&self, pkg: PackageId) -> impl Iterator { + pub fn deps( + &self, + pkg: PackageId, + ) -> impl Iterator)> { self.deps_not_replaced(pkg) .map(move |(id, deps)| (self.replacement(id).unwrap_or(id), deps)) } @@ -253,10 +258,8 @@ unable to verify that `{0}` is the same as when the lockfile was generated pub fn deps_not_replaced( &self, pkg: PackageId, - ) -> impl Iterator { - self.graph - .edges(&pkg) - .map(|(id, deps)| (*id, deps.as_slice())) + ) -> impl Iterator)> { + self.graph.edges(&pkg).map(|(id, deps)| (*id, deps)) } pub fn replacement(&self, pkg: PackageId) -> Option { @@ -306,14 +309,10 @@ unable to verify that `{0}` is the same as when the lockfile was generated to: PackageId, to_target: &Target, ) -> CargoResult { - let deps = if from == to { - &[] - } else { - self.dependencies_listed(from, to) - }; + let deps = self.dependencies_listed(from, to); let crate_name = to_target.crate_name(); - let mut names = deps.iter().map(|d| { + let mut names = deps.map(|d| { d.explicit_name_in_toml() .map(|s| s.as_str().replace("-", "_")) .unwrap_or_else(|| crate_name.clone()) @@ -330,7 +329,7 @@ unable to verify that `{0}` is the same as when the lockfile was generated Ok(name) } - fn dependencies_listed(&self, from: PackageId, to: PackageId) -> &[Dependency] { + fn dependencies_listed(&self, from: PackageId, to: PackageId) -> graph::Edges<'_, Dependency> { // We've got a dependency on `from` to `to`, but this dependency edge // may be affected by [replace]. If the `to` package is listed as the // target of a replacement (aka the key of a reverse replacement map) @@ -341,14 +340,16 @@ unable to verify that `{0}` is the same as when the lockfile was generated // that's where the dependency originates from, and we only replace // targets of dependencies not the originator. if let Some(replace) = self.reverse_replacements.get(&to) { - if let Some(deps) = self.graph.edge(&from, replace) { + let deps = self.graph.edge(&from, replace); + if !deps.is_empty() { return deps; } } - match self.graph.edge(&from, &to) { - Some(ret) => ret, - None => panic!("no Dependency listed for `{}` => `{}`", from, to), + let deps = self.graph.edge(&from, &to); + if deps.is_empty() { + panic!("no Dependency listed for `{}` => `{}`", from, to); } + deps } /// Returns the version of the encoding that's being used for this lock diff --git a/src/cargo/ops/cargo_fetch.rs b/src/cargo/ops/cargo_fetch.rs index 43835374ab8..1b2023550ec 100644 --- a/src/cargo/ops/cargo_fetch.rs +++ b/src/cargo/ops/cargo_fetch.rs @@ -41,8 +41,8 @@ pub fn fetch<'a>( to_download.push(id); let deps = resolve .deps(id) - .filter(|&(_id, deps)| { - deps.iter().any(|d| { + .filter(|(_id, deps)| { + (*deps).clone().any(|d| { // If no target was specified then all dependencies can // be fetched. let target = match options.target { diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 00f58a292fb..351973cf5ce 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -1,67 +1,175 @@ +use indexmap::IndexMap; use std::borrow::Borrow; -use std::collections::BTreeSet; +use std::collections::HashSet; use std::fmt; +use std::hash::Hash; +use std::num::NonZeroUsize; -use im_rc; +#[derive(Clone)] +struct EdgeLink { + value: E, + next: Option, +} +#[derive(Clone)] pub struct Graph { - nodes: im_rc::OrdMap>, + edges: Vec>, + nodes: indexmap::IndexMap>>, +} + +#[derive(Clone)] +pub struct Edges<'a, E: Clone> { + graph: &'a [EdgeLink], + index: Option, } -impl Graph { +impl<'a, E: Clone> Edges<'a, E> { + pub fn is_empty(&self) -> bool { + self.index.is_none() + } +} + +impl<'a, E: Clone> Iterator for Edges<'a, E> { + type Item = &'a E; + + fn next(&mut self) -> Option<&'a E> { + let old_index = self.index; + old_index.map(|old_index| { + self.index = self.graph[old_index].next.map(|i| i.get()); + &self.graph[old_index].value + }) + } +} + +impl Graph { pub fn new() -> Graph { Graph { - nodes: im_rc::OrdMap::new(), + edges: vec![], + nodes: IndexMap::new(), } } pub fn add(&mut self, node: N) { - self.nodes.entry(node).or_insert_with(im_rc::OrdMap::new); + self.nodes.entry(node).or_insert_with(IndexMap::new); + } + + pub fn link(&mut self, node: N, child: N) { + use indexmap::map::Entry; + match self + .nodes + .entry(node.clone()) + .or_insert_with(IndexMap::new) + .entry(child.clone()) + { + Entry::Vacant(entry) => { + entry.insert(None); + } + Entry::Occupied(_) => {} + }; } - pub fn link(&mut self, node: N, child: N) -> &mut E { - self.nodes - .entry(node) - .or_insert_with(im_rc::OrdMap::new) - .entry(child) - .or_insert_with(Default::default) + pub fn add_edge(&mut self, node: N, child: N, edge: E) { + use indexmap::map::Entry; + match self + .nodes + .entry(node.clone()) + .or_insert_with(IndexMap::new) + .entry(child.clone()) + { + Entry::Vacant(entry) => { + self.edges.push(EdgeLink { + value: edge, + next: None, + }); + let edge_index = self.edges.len() - 1; + entry.insert(Some(edge_index)); + } + Entry::Occupied(mut entry) => { + let edge_index = *entry.get(); + match edge_index { + None => { + self.edges.push(EdgeLink { + value: edge, + next: None, + }); + let edge_index = self.edges.len() - 1; + entry.insert(Some(edge_index)); + } + Some(mut edge_index) => loop { + if self.edges[edge_index].value == edge { + return; + } + if self.edges[edge_index].next.is_none() { + self.edges.push(EdgeLink { + value: edge, + next: None, + }); + let new_index = NonZeroUsize::new(self.edges.len() - 1).unwrap(); + self.edges[edge_index].next = Some(new_index); + return; + } + edge_index = self.edges[edge_index].next.unwrap().get(); + }, + } + } + } } pub fn contains(&self, k: &Q) -> bool where N: Borrow, - Q: Ord + Eq, + Q: Hash + Eq, { self.nodes.contains_key(k) } - pub fn edge(&self, from: &N, to: &N) -> Option<&E> { - self.nodes.get(from)?.get(to) + pub fn edge(&self, from: &N, to: &N) -> Edges<'_, E> { + Edges { + graph: self.edges.as_slice(), + index: self + .nodes + .get(from) + .and_then(|x| x.get(to).copied()) + .and_then(|x| x), + } } - pub fn edges(&self, from: &N) -> impl Iterator { - self.nodes.get(from).into_iter().flat_map(|x| x.iter()) + pub fn edges(&self, from: &N) -> impl Iterator)> { + let edges = self.edges.as_slice(); + self.nodes.get(from).into_iter().flat_map(move |x| { + x.iter().map(move |(to, idx)| { + ( + to, + Edges { + graph: edges, + index: *idx, + }, + ) + }) + }) } /// A topological sort of the `Graph` pub fn sort(&self) -> Vec { let mut ret = Vec::new(); - let mut marks = BTreeSet::new(); + let mut marks = HashSet::new(); - for node in self.nodes.keys() { + for node in self.nodes.keys().rev() { self.sort_inner_visit(node, &mut ret, &mut marks); } ret } - fn sort_inner_visit(&self, node: &N, dst: &mut Vec, marks: &mut BTreeSet) { + fn sort_inner_visit(&self, node: &N, dst: &mut Vec, marks: &mut HashSet) { if !marks.insert(node.clone()) { return; } - for child in self.nodes[node].keys() { - self.sort_inner_visit(child, dst, marks); + if let Some(nodes) = self.nodes.get(node) { + for child in nodes.keys().rev() { + self.sort_inner_visit(child, dst, marks); + } } dst.push(node.clone()); @@ -74,7 +182,7 @@ impl Graph { /// Checks if there is a path from `from` to `to`. pub fn is_path_from_to<'a>(&'a self, from: &'a N, to: &'a N) -> bool { let mut stack = vec![from]; - let mut seen = BTreeSet::new(); + let mut seen = HashSet::new(); seen.insert(from); while let Some(iter) = stack.pop().and_then(|p| self.nodes.get(p)) { for p in iter.keys() { @@ -98,7 +206,7 @@ impl Graph { // Note that we can have "cycles" introduced through dev-dependency // edges, so make sure we don't loop infinitely. .find(|&(node, _)| !result.contains(&node)) - .map(|(ref p, _)| p) + .map(|(p, _)| p) }) { result.push(p); pkg = p; @@ -120,7 +228,7 @@ impl Graph { // Note that we can have "cycles" introduced through dev-dependency // edges, so make sure we don't loop infinitely. .find(|&(node, _)| !res.contains(&node)) - .map(|(ref p, _)| p) + .map(|(p, _)| p) }; while let Some(p) = first_pkg_depending_on(pkg, &result) { result.push(p); @@ -130,21 +238,26 @@ impl Graph { } } -impl Default for Graph { +impl Default for Graph { fn default() -> Graph { Graph::new() } } -impl fmt::Debug for Graph { +impl fmt::Debug + for Graph +{ fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { writeln!(fmt, "Graph {{")?; - for (n, e) in &self.nodes { - writeln!(fmt, " - {}", n)?; + for (from, e) in &self.nodes { + writeln!(fmt, " - {}", from)?; - for n in e.keys() { - writeln!(fmt, " - {}", n)?; + for to in e.keys() { + writeln!(fmt, " - {}", to)?; + for edge in self.edge(from, to) { + writeln!(fmt, " - {:?}", edge)?; + } } } @@ -154,17 +267,9 @@ impl fmt::Debug for Graph { } } -impl PartialEq for Graph { +impl PartialEq for Graph { fn eq(&self, other: &Graph) -> bool { self.nodes.eq(&other.nodes) } } -impl Eq for Graph {} - -impl Clone for Graph { - fn clone(&self) -> Graph { - Graph { - nodes: self.nodes.clone(), - } - } -} +impl Eq for Graph {} From 78a984d7b826f130dfbfcad1be65350585f9ecdb Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Mon, 8 Jul 2019 20:14:27 -0400 Subject: [PATCH 02/20] query the prefix of a graph --- src/cargo/util/graph.rs | 216 +++++++++++++++++++++++++++------------- 1 file changed, 149 insertions(+), 67 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 351973cf5ce..f505a15092b 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -4,6 +4,7 @@ use std::collections::HashSet; use std::fmt; use std::hash::Hash; use std::num::NonZeroUsize; +use std::rc::Rc; #[derive(Clone)] struct EdgeLink { @@ -12,9 +13,32 @@ struct EdgeLink { } #[derive(Clone)] -pub struct Graph { +struct GraphCore { edges: Vec>, - nodes: indexmap::IndexMap>>, + link_count: usize, + nodes: indexmap::IndexMap)>>, +} +#[derive(Copy, Clone, PartialEq, Debug)] +struct GraphAge { + link_count: usize, + len_edges: usize, + len_nodes: usize, +} + +impl GraphCore { + fn len(&self) -> GraphAge { + GraphAge { + link_count: self.link_count, + len_edges: self.edges.len(), + len_nodes: self.nodes.len(), + } + } +} + +#[derive(Clone)] +pub struct Graph { + inner: Rc>, + age: GraphAge, } #[derive(Clone)] @@ -34,35 +58,51 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { fn next(&mut self) -> Option<&'a E> { let old_index = self.index; - old_index.map(|old_index| { - self.index = self.graph[old_index].next.map(|i| i.get()); - &self.graph[old_index].value - }) + old_index + .and_then(|old_index| self.graph.get(old_index)) + .map(|edge_link| { + self.index = edge_link.next.map(|i| i.get()); + &edge_link.value + }) } } impl Graph { pub fn new() -> Graph { Graph { - edges: vec![], - nodes: IndexMap::new(), + inner: Rc::new(GraphCore { + edges: vec![], + link_count: 0, + nodes: Default::default(), + }), + age: GraphAge { + link_count: 0, + len_edges: 0, + len_nodes: 0, + }, } } pub fn add(&mut self, node: N) { - self.nodes.entry(node).or_insert_with(IndexMap::new); + assert_eq!(self.age, self.inner.len()); + let inner = Rc::make_mut(&mut self.inner); + inner.nodes.entry(node).or_insert_with(IndexMap::new); + self.age = inner.len(); } pub fn link(&mut self, node: N, child: N) { use indexmap::map::Entry; - match self + assert_eq!(self.age, self.inner.len()); + let inner = Rc::make_mut(&mut self.inner); + match inner .nodes .entry(node.clone()) .or_insert_with(IndexMap::new) .entry(child.clone()) { Entry::Vacant(entry) => { - entry.insert(None); + inner.link_count += 1; + entry.insert((inner.link_count, None)); } Entry::Occupied(_) => {} }; @@ -70,49 +110,53 @@ impl Graph { pub fn add_edge(&mut self, node: N, child: N, edge: E) { use indexmap::map::Entry; - match self + assert_eq!(self.age, self.inner.len()); + let inner = Rc::make_mut(&mut self.inner); + match inner .nodes .entry(node.clone()) .or_insert_with(IndexMap::new) .entry(child.clone()) { Entry::Vacant(entry) => { - self.edges.push(EdgeLink { + inner.edges.push(EdgeLink { value: edge, next: None, }); - let edge_index = self.edges.len() - 1; - entry.insert(Some(edge_index)); + let edge_index = inner.edges.len() - 1; + inner.link_count += 1; + entry.insert((inner.link_count, Some(edge_index))); } Entry::Occupied(mut entry) => { let edge_index = *entry.get(); match edge_index { - None => { - self.edges.push(EdgeLink { + (link_count, None) => { + inner.edges.push(EdgeLink { value: edge, next: None, }); - let edge_index = self.edges.len() - 1; - entry.insert(Some(edge_index)); + let edge_index = inner.edges.len() - 1; + entry.insert((link_count, Some(edge_index))); } - Some(mut edge_index) => loop { - if self.edges[edge_index].value == edge { + (_, Some(mut edge_index)) => loop { + if inner.edges[edge_index].value == edge { return; } - if self.edges[edge_index].next.is_none() { - self.edges.push(EdgeLink { + if inner.edges[edge_index].next.is_none() { + inner.edges.push(EdgeLink { value: edge, next: None, }); - let new_index = NonZeroUsize::new(self.edges.len() - 1).unwrap(); - self.edges[edge_index].next = Some(new_index); + let new_index = NonZeroUsize::new(inner.edges.len() - 1).unwrap(); + inner.edges[edge_index].next = Some(new_index); return; } - edge_index = self.edges[edge_index].next.unwrap().get(); + edge_index = inner.edges[edge_index].next.unwrap().get(); }, } } } + self.age = inner.len(); } pub fn contains(&self, k: &Q) -> bool @@ -120,33 +164,46 @@ impl Graph { N: Borrow, Q: Hash + Eq, { - self.nodes.contains_key(k) + self.inner + .nodes + .get_full(k) + .filter(|(i, _, _)| *i < self.age.len_nodes) + .is_some() } pub fn edge(&self, from: &N, to: &N) -> Edges<'_, E> { Edges { - graph: self.edges.as_slice(), + graph: &self.inner.edges[..self.age.len_edges], index: self + .inner .nodes .get(from) .and_then(|x| x.get(to).copied()) - .and_then(|x| x), + .filter(|(c, _)| c <= &self.age.link_count) + .and_then(|x| x.1), } } pub fn edges(&self, from: &N) -> impl Iterator)> { - let edges = self.edges.as_slice(); - self.nodes.get(from).into_iter().flat_map(move |x| { - x.iter().map(move |(to, idx)| { - ( - to, - Edges { - graph: edges, - index: *idx, - }, - ) + let edges = &self.inner.edges[..self.age.len_edges]; + self.inner + .nodes + .get_full(from) + .filter(|(i, _, _)| *i < self.age.len_nodes) + .into_iter() + .flat_map(move |(_, _, x)| { + x.iter() + .filter(move |(_, (c, _))| c <= &self.age.link_count) + .map(move |(to, (_, idx))| { + ( + to, + Edges { + graph: edges, + index: *idx, + }, + ) + }) }) - }) } /// A topological sort of the `Graph` @@ -154,7 +211,7 @@ impl Graph { let mut ret = Vec::new(); let mut marks = HashSet::new(); - for node in self.nodes.keys().rev() { + for node in self.inner.nodes.keys().take(self.age.len_nodes) { self.sort_inner_visit(node, &mut ret, &mut marks); } @@ -166,9 +223,11 @@ impl Graph { return; } - if let Some(nodes) = self.nodes.get(node) { - for child in nodes.keys().rev() { - self.sort_inner_visit(child, dst, marks); + if let Some(nodes) = self.inner.nodes.get(node) { + for (child, (c, _)) in nodes.iter().rev() { + if *c <= self.age.link_count { + self.sort_inner_visit(child, dst, marks); + } } } @@ -176,7 +235,7 @@ impl Graph { } pub fn iter(&self) -> impl Iterator { - self.nodes.keys() + self.inner.nodes.keys().take(self.age.len_nodes) } /// Checks if there is a path from `from` to `to`. @@ -184,13 +243,20 @@ impl Graph { let mut stack = vec![from]; let mut seen = HashSet::new(); seen.insert(from); - while let Some(iter) = stack.pop().and_then(|p| self.nodes.get(p)) { - for p in iter.keys() { - if p == to { - return true; - } - if seen.insert(p) { - stack.push(p); + while let Some((_, _, iter)) = stack.pop().and_then(|p| { + self.inner + .nodes + .get_full(p) + .filter(|(i, _, _)| *i < self.age.len_nodes) + }) { + for (p, (c, _)) in iter.iter() { + if *c <= self.age.link_count { + if p == to { + return true; + } + if seen.insert(p) { + stack.push(p); + } } } } @@ -201,13 +267,20 @@ impl Graph { /// a leaf. pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { let mut result = vec![pkg]; - while let Some(p) = self.nodes.get(pkg).and_then(|p| { - p.iter() - // Note that we can have "cycles" introduced through dev-dependency - // edges, so make sure we don't loop infinitely. - .find(|&(node, _)| !result.contains(&node)) - .map(|(p, _)| p) - }) { + while let Some(p) = self + .inner + .nodes + .get_full(pkg) + .filter(|(i, _, _)| *i < self.age.len_nodes) + .and_then(|(_, _, p)| { + p.iter() + // Note that we can have "cycles" introduced through dev-dependency + // edges, so make sure we don't loop infinitely. + .filter(|(_, (c, _))| c <= &self.age.link_count) + .find(|&(node, _)| !result.contains(&node)) + .map(|(p, _)| p) + }) + { result.push(p); pkg = p; } @@ -222,9 +295,16 @@ impl Graph { // it's used for! let mut result = vec![pkg]; let first_pkg_depending_on = |pkg: &N, res: &[&N]| { - self.nodes + self.inner + .nodes .iter() - .filter(|&(_, adjacent)| adjacent.contains_key(pkg)) + .take(self.age.len_nodes) + .filter(|&(_, adjacent)| { + adjacent + .get_full(pkg) + .filter(|(i, _, _)| *i < self.age.len_nodes) + .is_some() + }) // Note that we can have "cycles" introduced through dev-dependency // edges, so make sure we don't loop infinitely. .find(|&(node, _)| !res.contains(&node)) @@ -250,13 +330,15 @@ impl fmt fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { writeln!(fmt, "Graph {{")?; - for (from, e) in &self.nodes { + for (from, e) in self.inner.nodes.iter().take(self.age.len_nodes) { writeln!(fmt, " - {}", from)?; - for to in e.keys() { - writeln!(fmt, " - {}", to)?; - for edge in self.edge(from, to) { - writeln!(fmt, " - {:?}", edge)?; + for (to, (c, _)) in e.iter() { + if *c <= self.age.link_count { + writeln!(fmt, " - {}", to)?; + for edge in self.edge(from, to) { + writeln!(fmt, " - {:?}", edge)?; + } } } } @@ -269,7 +351,7 @@ impl fmt impl PartialEq for Graph { fn eq(&self, other: &Graph) -> bool { - self.nodes.eq(&other.nodes) + self.inner.nodes.eq(&other.inner.nodes) } } impl Eq for Graph {} From b79dbbc8e34cb61df881b6378b4c8b9d92ac5c3a Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 12 Jul 2019 15:05:11 -0400 Subject: [PATCH 03/20] Only have one copy of the graph. (borrow checker not happy) --- src/cargo/util/graph.rs | 217 +++++++++++++++++++++++++--------------- 1 file changed, 137 insertions(+), 80 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index f505a15092b..a7dd8d8b520 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -15,6 +15,7 @@ struct EdgeLink { #[derive(Clone)] struct GraphCore { edges: Vec>, + back_refs: Vec<(NonZeroUsize, usize)>, link_count: usize, nodes: indexmap::IndexMap)>>, } @@ -35,6 +36,104 @@ impl GraphCore { } } +impl GraphCore { + fn reset_to(&mut self, age: GraphAge) { + while self.nodes.len() > age.len_nodes { + self.nodes.pop(); + } + for (_, lookup) in self.nodes.iter_mut() { + lookup.retain(|_, idx| { + if idx.1 >= age.len_edges { + idx.1 = None; + } + *idx.0 <= age.link_count + }); + } + while self + .back_refs + .last() + .filter(|(idx, _)| idx.get() >= age.len_edges) + .is_some() + { + let (_, idx) = self.back_refs.pop().unwrap(); + self.edges[idx].next = None; + } + self.edges.truncate(age.len_edges); + } + + pub fn add(&mut self, node: N) -> GraphAge { + self.nodes.entry(node).or_insert_with(IndexMap::new); + self.len() + } + + pub fn link(&mut self, node: N, child: N) -> GraphAge { + use indexmap::map::Entry; + match self + .nodes + .entry(node.clone()) + .or_insert_with(IndexMap::new) + .entry(child.clone()) + { + Entry::Vacant(entry) => { + self.link_count += 1; + entry.insert((self.link_count, None)); + } + Entry::Occupied(_) => {} + }; + self.len() + } + + pub fn add_edge(&mut self, node: N, child: N, edge: E) -> GraphAge { + use indexmap::map::Entry; + match self + .nodes + .entry(node.clone()) + .or_insert_with(IndexMap::new) + .entry(child.clone()) + { + Entry::Vacant(entry) => { + self.edges.push(EdgeLink { + value: edge, + next: None, + }); + let edge_index = self.edges.len() - 1; + self.link_count += 1; + entry.insert((self.link_count, Some(edge_index))); + } + Entry::Occupied(mut entry) => { + let edge_index = *entry.get(); + match edge_index { + (link_count, None) => { + self.edges.push(EdgeLink { + value: edge, + next: None, + }); + let edge_index = self.edges.len() - 1; + entry.insert((link_count, Some(edge_index))); + } + (_, Some(mut edge_index)) => loop { + if inner.edges[edge_index].value == edge { + return self.len(); + } + if self.edges[edge_index].next.is_none() { + self.edges.push(EdgeLink { + value: edge, + next: None, + }); + let new_index = NonZeroUsize::new(self.edges.len() - 1).unwrap(); + self.edges[edge_index].next = Some(new_index); + self.back_refs.push((new_index, edge_index)); + return self.len(); + } + edge_index = self.edges[edge_index].next.unwrap().get(); + }, + } + } + } + self.len() + } +} + #[derive(Clone)] pub struct Graph { inner: Rc>, @@ -72,6 +171,7 @@ impl Graph { Graph { inner: Rc::new(GraphCore { edges: vec![], + back_refs: vec![], link_count: 0, nodes: Default::default(), }), @@ -83,80 +183,37 @@ impl Graph { } } - pub fn add(&mut self, node: N) { - assert_eq!(self.age, self.inner.len()); + fn borrow(&self) -> &GraphCore { + let inner = &self.inner; + assert!(self.age.len_edges <= inner.len().len_edges); + assert!(self.age.len_nodes <= inner.len().len_nodes); + inner + } + + fn activate(&mut self) -> &mut GraphCore { + // if { + // let inner = &self.inner; + // self.age.len_edges >= inner.len().len_edges + // || self.age.len_nodes >= inner.len().len_nodes + // } { let inner = Rc::make_mut(&mut self.inner); - inner.nodes.entry(node).or_insert_with(IndexMap::new); - self.age = inner.len(); + inner.reset_to(self.age); + inner + } + + pub fn add(&mut self, node: N) { + let age = self.activate().add(node); + self.age = age; } pub fn link(&mut self, node: N, child: N) { - use indexmap::map::Entry; - assert_eq!(self.age, self.inner.len()); - let inner = Rc::make_mut(&mut self.inner); - match inner - .nodes - .entry(node.clone()) - .or_insert_with(IndexMap::new) - .entry(child.clone()) - { - Entry::Vacant(entry) => { - inner.link_count += 1; - entry.insert((inner.link_count, None)); - } - Entry::Occupied(_) => {} - }; + let age = self.activate().link(node, child); + self.age = age; } pub fn add_edge(&mut self, node: N, child: N, edge: E) { - use indexmap::map::Entry; - assert_eq!(self.age, self.inner.len()); - let inner = Rc::make_mut(&mut self.inner); - match inner - .nodes - .entry(node.clone()) - .or_insert_with(IndexMap::new) - .entry(child.clone()) - { - Entry::Vacant(entry) => { - inner.edges.push(EdgeLink { - value: edge, - next: None, - }); - let edge_index = inner.edges.len() - 1; - inner.link_count += 1; - entry.insert((inner.link_count, Some(edge_index))); - } - Entry::Occupied(mut entry) => { - let edge_index = *entry.get(); - match edge_index { - (link_count, None) => { - inner.edges.push(EdgeLink { - value: edge, - next: None, - }); - let edge_index = inner.edges.len() - 1; - entry.insert((link_count, Some(edge_index))); - } - (_, Some(mut edge_index)) => loop { - if inner.edges[edge_index].value == edge { - return; - } - if inner.edges[edge_index].next.is_none() { - inner.edges.push(EdgeLink { - value: edge, - next: None, - }); - let new_index = NonZeroUsize::new(inner.edges.len() - 1).unwrap(); - inner.edges[edge_index].next = Some(new_index); - return; - } - edge_index = inner.edges[edge_index].next.unwrap().get(); - }, - } - } - } - self.age = inner.len(); + let age = self.activate().add_edge(node, child, edge); + self.age = age; } pub fn contains(&self, k: &Q) -> bool @@ -164,7 +221,7 @@ impl Graph { N: Borrow, Q: Hash + Eq, { - self.inner + self.borrow() .nodes .get_full(k) .filter(|(i, _, _)| *i < self.age.len_nodes) @@ -173,9 +230,9 @@ impl Graph { pub fn edge(&self, from: &N, to: &N) -> Edges<'_, E> { Edges { - graph: &self.inner.edges[..self.age.len_edges], + graph: &self.borrow().edges[..self.age.len_edges], index: self - .inner + .borrow() .nodes .get(from) .and_then(|x| x.get(to).copied()) @@ -185,8 +242,8 @@ impl Graph { } pub fn edges(&self, from: &N) -> impl Iterator)> { - let edges = &self.inner.edges[..self.age.len_edges]; - self.inner + let edges = &self.borrow().edges[..self.age.len_edges]; + self.borrow() .nodes .get_full(from) .filter(|(i, _, _)| *i < self.age.len_nodes) @@ -211,7 +268,7 @@ impl Graph { let mut ret = Vec::new(); let mut marks = HashSet::new(); - for node in self.inner.nodes.keys().take(self.age.len_nodes) { + for node in self.borrow().nodes.keys().take(self.age.len_nodes) { self.sort_inner_visit(node, &mut ret, &mut marks); } @@ -223,7 +280,7 @@ impl Graph { return; } - if let Some(nodes) = self.inner.nodes.get(node) { + if let Some(nodes) = self.borrow().nodes.get(node) { for (child, (c, _)) in nodes.iter().rev() { if *c <= self.age.link_count { self.sort_inner_visit(child, dst, marks); @@ -235,7 +292,7 @@ impl Graph { } pub fn iter(&self) -> impl Iterator { - self.inner.nodes.keys().take(self.age.len_nodes) + self.borrow().nodes.keys().take(self.age.len_nodes) } /// Checks if there is a path from `from` to `to`. @@ -244,7 +301,7 @@ impl Graph { let mut seen = HashSet::new(); seen.insert(from); while let Some((_, _, iter)) = stack.pop().and_then(|p| { - self.inner + self.borrow() .nodes .get_full(p) .filter(|(i, _, _)| *i < self.age.len_nodes) @@ -268,7 +325,7 @@ impl Graph { pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { let mut result = vec![pkg]; while let Some(p) = self - .inner + .borrow() .nodes .get_full(pkg) .filter(|(i, _, _)| *i < self.age.len_nodes) @@ -295,7 +352,7 @@ impl Graph { // it's used for! let mut result = vec![pkg]; let first_pkg_depending_on = |pkg: &N, res: &[&N]| { - self.inner + self.borrow() .nodes .iter() .take(self.age.len_nodes) @@ -330,7 +387,7 @@ impl fmt fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { writeln!(fmt, "Graph {{")?; - for (from, e) in self.inner.nodes.iter().take(self.age.len_nodes) { + for (from, e) in self.borrow().nodes.iter().take(self.age.len_nodes) { writeln!(fmt, " - {}", from)?; for (to, (c, _)) in e.iter() { @@ -351,7 +408,7 @@ impl fmt impl PartialEq for Graph { fn eq(&self, other: &Graph) -> bool { - self.inner.nodes.eq(&other.inner.nodes) + self.borrow().nodes.eq(&other.borrow().nodes) } } impl Eq for Graph {} From f9d7990e967f96a038725cd1ab72219fa5d81064 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 18 Jul 2019 17:10:38 -0400 Subject: [PATCH 04/20] StackGraph where interior mutability may help --- src/cargo/core/resolver/context.rs | 6 +- src/cargo/util/graph.rs | 328 +++++++++++++++++++---------- src/cargo/util/mod.rs | 2 +- 3 files changed, 219 insertions(+), 117 deletions(-) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index f2ef634868d..b0dbc536686 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -6,7 +6,7 @@ use log::debug; use crate::core::interning::InternedString; use crate::core::{Dependency, PackageId, SourceId, Summary}; -use crate::util::Graph; +use crate::util::{Graph, StackGraph}; use super::dep_cache::RegistryQueryer; use super::errors::ActivateResult; @@ -34,7 +34,7 @@ pub struct Context { /// a way to look up for a package in activations what packages required it /// and all of the exact deps that it fulfilled. - pub parents: Graph, + pub parents: StackGraph, } /// When backtracking it can be useful to know how far back to go. @@ -89,7 +89,7 @@ impl Context { } else { None }, - parents: Graph::new(), + parents: StackGraph::new(), activations: im_rc::HashMap::new(), } } diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index a7dd8d8b520..8407c09307c 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -13,12 +13,13 @@ struct EdgeLink { } #[derive(Clone)] -struct GraphCore { +pub struct Graph { edges: Vec>, back_refs: Vec<(NonZeroUsize, usize)>, link_count: usize, nodes: indexmap::IndexMap)>>, } + #[derive(Copy, Clone, PartialEq, Debug)] struct GraphAge { link_count: usize, @@ -26,7 +27,7 @@ struct GraphAge { len_nodes: usize, } -impl GraphCore { +impl Graph { fn len(&self) -> GraphAge { GraphAge { link_count: self.link_count, @@ -36,17 +37,26 @@ impl GraphCore { } } -impl GraphCore { +impl Graph { + pub fn new() -> Graph { + Graph { + edges: vec![], + back_refs: vec![], + link_count: 0, + nodes: Default::default(), + } + } + fn reset_to(&mut self, age: GraphAge) { while self.nodes.len() > age.len_nodes { self.nodes.pop(); } for (_, lookup) in self.nodes.iter_mut() { lookup.retain(|_, idx| { - if idx.1 >= age.len_edges { + if idx.1 >= Some(age.len_edges) { idx.1 = None; } - *idx.0 <= age.link_count + idx.0 <= age.link_count }); } while self @@ -61,12 +71,11 @@ impl GraphCore { self.edges.truncate(age.len_edges); } - pub fn add(&mut self, node: N) -> GraphAge { + pub fn add(&mut self, node: N) { self.nodes.entry(node).or_insert_with(IndexMap::new); - self.len() } - pub fn link(&mut self, node: N, child: N) -> GraphAge { + pub fn link(&mut self, node: N, child: N) { use indexmap::map::Entry; match self .nodes @@ -80,10 +89,9 @@ impl GraphCore { } Entry::Occupied(_) => {} }; - self.len() } - pub fn add_edge(&mut self, node: N, child: N, edge: E) -> GraphAge { + pub fn add_edge(&mut self, node: N, child: N, edge: E) { use indexmap::map::Entry; match self .nodes @@ -112,8 +120,8 @@ impl GraphCore { entry.insert((link_count, Some(edge_index))); } (_, Some(mut edge_index)) => loop { - if inner.edges[edge_index].value == edge { - return self.len(); + if self.edges[edge_index].value == edge { + return; } if self.edges[edge_index].next.is_none() { self.edges.push(EdgeLink { @@ -123,22 +131,173 @@ impl GraphCore { let new_index = NonZeroUsize::new(self.edges.len() - 1).unwrap(); self.edges[edge_index].next = Some(new_index); self.back_refs.push((new_index, edge_index)); - return self.len(); + return; } edge_index = self.edges[edge_index].next.unwrap().get(); }, } } } - self.len() + } + + pub fn contains(&self, k: &Q) -> bool + where + N: Borrow, + Q: Hash + Eq, + { + self.nodes.contains_key(k) + } + + pub fn iter(&self) -> impl Iterator { + self.borrow().nodes.keys() + } + + pub fn edge(&self, from: &N, to: &N) -> Edges<'_, E> { + Edges { + graph: &self.edges, + index: self + .nodes + .get(from) + .and_then(|x| x.get(to).copied()) + .and_then(|x| x.1), + } + } + + pub fn edges(&self, from: &N) -> impl Iterator)> { + let edges = &self.edges; + self.nodes.get(from).into_iter().flat_map(move |x| { + x.iter().map(move |(to, (_, idx))| { + ( + to, + Edges { + graph: edges, + index: *idx, + }, + ) + }) + }) + } + + /// A topological sort of the `Graph` + pub fn sort(&self) -> Vec { + let mut ret = Vec::new(); + let mut marks = HashSet::new(); + + for node in self.nodes.keys() { + self.sort_inner_visit(node, &mut ret, &mut marks); + } + + ret + } + + fn sort_inner_visit(&self, node: &N, dst: &mut Vec, marks: &mut HashSet) { + if !marks.insert(node.clone()) { + return; + } + + if let Some(nodes) = self.nodes.get(node) { + for child in nodes.keys().rev() { + self.sort_inner_visit(child, dst, marks); + } + } + + dst.push(node.clone()); + } + + /// Checks if there is a path from `from` to `to`. + pub fn is_path_from_to<'a>(&'a self, from: &'a N, to: &'a N) -> bool { + let mut stack = vec![from]; + let mut seen = HashSet::new(); + seen.insert(from); + while let Some(iter) = stack.pop().and_then(|p| self.nodes.get(p)) { + for p in iter.keys() { + if p == to { + return true; + } + if seen.insert(p) { + stack.push(p); + } + } + } + false + } + + /// Resolves one of the paths from the given dependent package down to + /// a leaf. + pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { + let mut result = vec![pkg]; + while let Some(p) = self.nodes.get(pkg).and_then(|p| { + p.keys() + // Note that we can have "cycles" introduced through dev-dependency + // edges, so make sure we don't loop infinitely. + .find(|node| !result.contains(node)) + }) { + result.push(p); + pkg = p; + } + result + } + + /// Resolves one of the paths from the given dependent package up to + /// the root. + pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { + // Note that this implementation isn't the most robust per se, we'll + // likely have to tweak this over time. For now though it works for what + // it's used for! + let mut result = vec![pkg]; + let first_pkg_depending_on = |pkg: &N, res: &[&N]| { + self.borrow() + .nodes + .iter() + .filter(|&(_, adjacent)| adjacent.contains_key(pkg)) + // Note that we can have "cycles" introduced through dev-dependency + // edges, so make sure we don't loop infinitely. + .find(|&(node, _)| !res.contains(&node)) + .map(|(p, _)| p) + }; + while let Some(p) = first_pkg_depending_on(pkg, &result) { + result.push(p); + pkg = p; + } + result + } +} + +impl Default for Graph { + fn default() -> Graph { + Graph::new() + } +} + +impl fmt::Debug + for Graph +{ + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + writeln!(fmt, "Graph {{")?; + + for (from, e) in self.nodes.iter() { + writeln!(fmt, " - {}", from)?; + + for to in e.keys() { + writeln!(fmt, " - {}", to)?; + for edge in self.edge(from, to) { + writeln!(fmt, " - {:?}", edge)?; + } + } + } + + write!(fmt, "}}")?; + + Ok(()) } } -#[derive(Clone)] -pub struct Graph { - inner: Rc>, - age: GraphAge, +impl PartialEq for Graph { + fn eq(&self, other: &Graph) -> bool { + self.nodes.eq(&other.nodes) + } } +impl Eq for Graph {} #[derive(Clone)] pub struct Edges<'a, E: Clone> { @@ -166,31 +325,30 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { } } -impl Graph { - pub fn new() -> Graph { - Graph { - inner: Rc::new(GraphCore { - edges: vec![], - back_refs: vec![], - link_count: 0, - nodes: Default::default(), - }), - age: GraphAge { - link_count: 0, - len_edges: 0, - len_nodes: 0, - }, +#[derive(Clone)] +pub struct StackGraph { + inner: Rc>, + age: GraphAge, +} + +impl StackGraph { + pub fn new() -> StackGraph { + let inner = Graph::new(); + let age = inner.len(); + StackGraph { + inner: Rc::new(inner), + age, } } - fn borrow(&self) -> &GraphCore { + fn borrow(&self) -> &Graph { let inner = &self.inner; assert!(self.age.len_edges <= inner.len().len_edges); assert!(self.age.len_nodes <= inner.len().len_nodes); inner } - fn activate(&mut self) -> &mut GraphCore { + fn activate(&mut self) -> &mut Graph { // if { // let inner = &self.inner; // self.age.len_edges >= inner.len().len_edges @@ -202,18 +360,27 @@ impl Graph { } pub fn add(&mut self, node: N) { - let age = self.activate().add(node); - self.age = age; + self.age = { + let g = self.activate(); + g.add(node); + g.len() + }; } pub fn link(&mut self, node: N, child: N) { - let age = self.activate().link(node, child); - self.age = age; + self.age = { + let g = self.activate(); + g.link(node, child); + g.len() + }; } pub fn add_edge(&mut self, node: N, child: N, edge: E) { - let age = self.activate().add_edge(node, child, edge); - self.age = age; + self.age = { + let g = self.activate(); + g.add_edge(node, child, edge);; + g.len() + }; } pub fn contains(&self, k: &Q) -> bool @@ -228,6 +395,10 @@ impl Graph { .is_some() } + pub fn iter(&self) -> impl Iterator { + self.borrow().nodes.keys().take(self.age.len_nodes) + } + pub fn edge(&self, from: &N, to: &N) -> Edges<'_, E> { Edges { graph: &self.borrow().edges[..self.age.len_edges], @@ -263,38 +434,6 @@ impl Graph { }) } - /// A topological sort of the `Graph` - pub fn sort(&self) -> Vec { - let mut ret = Vec::new(); - let mut marks = HashSet::new(); - - for node in self.borrow().nodes.keys().take(self.age.len_nodes) { - self.sort_inner_visit(node, &mut ret, &mut marks); - } - - ret - } - - fn sort_inner_visit(&self, node: &N, dst: &mut Vec, marks: &mut HashSet) { - if !marks.insert(node.clone()) { - return; - } - - if let Some(nodes) = self.borrow().nodes.get(node) { - for (child, (c, _)) in nodes.iter().rev() { - if *c <= self.age.link_count { - self.sort_inner_visit(child, dst, marks); - } - } - } - - dst.push(node.clone()); - } - - pub fn iter(&self) -> impl Iterator { - self.borrow().nodes.keys().take(self.age.len_nodes) - } - /// Checks if there is a path from `from` to `to`. pub fn is_path_from_to<'a>(&'a self, from: &'a N, to: &'a N) -> bool { let mut stack = vec![from]; @@ -343,46 +482,16 @@ impl Graph { } result } - - /// Resolves one of the paths from the given dependent package up to - /// the root. - pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { - // Note that this implementation isn't the most robust per se, we'll - // likely have to tweak this over time. For now though it works for what - // it's used for! - let mut result = vec![pkg]; - let first_pkg_depending_on = |pkg: &N, res: &[&N]| { - self.borrow() - .nodes - .iter() - .take(self.age.len_nodes) - .filter(|&(_, adjacent)| { - adjacent - .get_full(pkg) - .filter(|(i, _, _)| *i < self.age.len_nodes) - .is_some() - }) - // Note that we can have "cycles" introduced through dev-dependency - // edges, so make sure we don't loop infinitely. - .find(|&(node, _)| !res.contains(&node)) - .map(|(p, _)| p) - }; - while let Some(p) = first_pkg_depending_on(pkg, &result) { - result.push(p); - pkg = p; - } - result - } } -impl Default for Graph { - fn default() -> Graph { - Graph::new() +impl Default for StackGraph { + fn default() -> StackGraph { + StackGraph::new() } } impl fmt::Debug - for Graph + for StackGraph { fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { writeln!(fmt, "Graph {{")?; @@ -405,10 +514,3 @@ impl fmt Ok(()) } } - -impl PartialEq for Graph { - fn eq(&self, other: &Graph) -> bool { - self.borrow().nodes.eq(&other.borrow().nodes) - } -} -impl Eq for Graph {} diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 94946ae36de..20d2c581cf9 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -8,7 +8,7 @@ pub use self::errors::{internal, process_error}; pub use self::errors::{CargoResult, CargoResultExt, CliResult, Test}; pub use self::errors::{CargoTestError, CliError, ProcessError}; pub use self::flock::{FileLock, Filesystem}; -pub use self::graph::Graph; +pub use self::graph::{Graph, StackGraph}; pub use self::hex::{hash_u64, short_hash, to_hex}; pub use self::into_url::IntoUrl; pub use self::into_url_with_base::IntoUrlWithBase; From 27a4d80be1c44d452ab8ee62328c78ef58768627 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 18 Jul 2019 18:18:22 -0400 Subject: [PATCH 05/20] interior mutability for the win! --- src/cargo/core/resolver/context.rs | 4 +- src/cargo/core/resolver/errors.rs | 11 +- src/cargo/core/resolver/mod.rs | 11 +- src/cargo/core/resolver/resolve.rs | 2 +- src/cargo/util/graph.rs | 178 +++++++++++++++-------------- 5 files changed, 111 insertions(+), 95 deletions(-) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index b0dbc536686..60ded0809e0 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -228,9 +228,9 @@ impl Context { self.activations .values() .for_each(|(r, _)| graph.add(r.package_id())); - for i in self.parents.iter() { + for i in self.parents.borrow().iter() { graph.add(*i); - for (o, edges) in self.parents.edges(i) { + for (o, edges) in self.parents.borrow().edges(i) { for e in edges { graph.add_edge(*o, *i, e.clone()) } diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs index 823120e167b..e835c183b3c 100644 --- a/src/cargo/core/resolver/errors.rs +++ b/src/cargo/core/resolver/errors.rs @@ -81,6 +81,7 @@ pub(super) fn activation_error( ResolveError::new( err, cx.parents + .borrow() .path_to_bottom(&parent.package_id()) .into_iter() .cloned() @@ -92,7 +93,7 @@ pub(super) fn activation_error( let mut msg = format!("failed to select a version for `{}`.", dep.package_name()); msg.push_str("\n ... required by "); msg.push_str(&describe_path( - &cx.parents.path_to_bottom(&parent.package_id()), + &cx.parents.borrow().path_to_bottom(&parent.package_id()), )); msg.push_str("\nversions that meet the requirements `"); @@ -124,7 +125,7 @@ pub(super) fn activation_error( msg.push_str(link); msg.push_str("` as well:\n"); } - msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); + msg.push_str(&describe_path(&cx.parents.borrow().path_to_bottom(p))); } let (features_errors, mut other_errors): (Vec<_>, Vec<_>) = other_errors @@ -178,7 +179,7 @@ pub(super) fn activation_error( for &(p, _) in other_errors.iter() { msg.push_str("\n\n previously selected "); - msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); + msg.push_str(&describe_path(&cx.parents.borrow().path_to_bottom(p))); } msg.push_str("\n\nfailed to select a version for `"); @@ -230,7 +231,7 @@ pub(super) fn activation_error( ); msg.push_str("required by "); msg.push_str(&describe_path( - &cx.parents.path_to_bottom(&parent.package_id()), + &cx.parents.borrow().path_to_bottom(&parent.package_id()), )); // If we have a path dependency with a locked version, then this may @@ -307,7 +308,7 @@ pub(super) fn activation_error( } msg.push_str("required by "); msg.push_str(&describe_path( - &cx.parents.path_to_bottom(&parent.package_id()), + &cx.parents.borrow().path_to_bottom(&parent.package_id()), )); msg diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 96fe10b1389..a53fdaf38e0 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -639,7 +639,7 @@ fn activate( // if `candidate_pid` was a private dependency of `p` then `p` parents can't see `c` thru `p` if public { // if it was public, then we add all of `p`s parents to be checked - for (&grand, mut d) in cx.parents.edges(&p) { + for (&grand, mut d) in cx.parents.borrow().edges(&p) { stack.push((grand, d.any(|x| x.is_public()))); } } @@ -821,7 +821,7 @@ impl RemainingCandidates { // if `b` was a private dependency of `p` then `p` parents can't see `t` thru `p` if public { // if it was public, then we add all of `p`s parents to be checked - for (&grand, mut d) in cx.parents.edges(&p) { + for (&grand, mut d) in cx.parents.borrow().edges(&p) { stack.push((grand, d.any(|x| x.is_public()))); } } @@ -880,8 +880,11 @@ fn generalize_conflicting( return None; } // What parents does that critical activation have - for (critical_parent, critical_parents_deps) in - cx.parents.edges(&backtrack_critical_id).filter(|(&p, _)| { + for (critical_parent, critical_parents_deps) in cx + .parents + .borrow() + .edges(&backtrack_critical_id) + .filter(|(&p, _)| { // it will only help backjump further if it is older then the critical_age cx.is_active(p).expect("parent not currently active!?") < backtrack_critical_age }) diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index 9b8b4e20bb5..176df30e3a8 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -346,7 +346,7 @@ unable to verify that `{0}` is the same as when the lockfile was generated } } let deps = self.graph.edge(&from, &to); - if deps.is_empty() { + if deps.is_empty() && from != to { panic!("no Dependency listed for `{}` => `{}`", from, to); } deps diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 8407c09307c..2b4acdb6512 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -1,12 +1,13 @@ use indexmap::IndexMap; use std::borrow::Borrow; +use std::cell::{Ref, RefCell, RefMut}; use std::collections::HashSet; use std::fmt; use std::hash::Hash; use std::num::NonZeroUsize; use std::rc::Rc; -#[derive(Clone)] +#[derive(Clone, Debug)] struct EdgeLink { value: E, next: Option, @@ -59,6 +60,7 @@ impl Graph { idx.0 <= age.link_count }); } + self.link_count = age.link_count; while self .back_refs .last() @@ -69,6 +71,7 @@ impl Graph { self.edges[idx].next = None; } self.edges.truncate(age.len_edges); + assert_eq!(self.len(), age); } pub fn add(&mut self, node: N) { @@ -299,7 +302,7 @@ impl PartialEq for Graph } impl Eq for Graph {} -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct Edges<'a, E: Clone> { graph: &'a [EdgeLink], index: Option, @@ -307,7 +310,7 @@ pub struct Edges<'a, E: Clone> { impl<'a, E: Clone> Edges<'a, E> { pub fn is_empty(&self) -> bool { - self.index.is_none() + self.index.filter(|&idx| idx < self.graph.len()).is_none() } } @@ -327,7 +330,7 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { #[derive(Clone)] pub struct StackGraph { - inner: Rc>, + inner: Rc>>, age: GraphAge, } @@ -336,32 +339,32 @@ impl StackGraph { let inner = Graph::new(); let age = inner.len(); StackGraph { - inner: Rc::new(inner), + inner: Rc::new(RefCell::new(inner)), age, } } - fn borrow(&self) -> &Graph { - let inner = &self.inner; + pub fn borrow(&self) -> StackGraphView<'_, N, E> { + let inner = RefCell::borrow(&self.inner); assert!(self.age.len_edges <= inner.len().len_edges); assert!(self.age.len_nodes <= inner.len().len_nodes); - inner + StackGraphView { + inner, + age: self.age, + } } - fn activate(&mut self) -> &mut Graph { - // if { - // let inner = &self.inner; - // self.age.len_edges >= inner.len().len_edges - // || self.age.len_nodes >= inner.len().len_nodes - // } { - let inner = Rc::make_mut(&mut self.inner); - inner.reset_to(self.age); + fn activate(&mut self) -> RefMut<'_, Graph> { + let mut inner = RefCell::borrow_mut(&mut self.inner); + if self.age != inner.len() { + inner.reset_to(self.age); + } inner } pub fn add(&mut self, node: N) { self.age = { - let g = self.activate(); + let mut g = self.activate(); g.add(node); g.len() }; @@ -369,7 +372,7 @@ impl StackGraph { pub fn link(&mut self, node: N, child: N) { self.age = { - let g = self.activate(); + let mut g = self.activate(); g.link(node, child); g.len() }; @@ -377,8 +380,8 @@ impl StackGraph { pub fn add_edge(&mut self, node: N, child: N, edge: E) { self.age = { - let g = self.activate(); - g.add_edge(node, child, edge);; + let mut g = self.activate(); + g.add_edge(node, child, edge); g.len() }; } @@ -389,21 +392,86 @@ impl StackGraph { Q: Hash + Eq, { self.borrow() + .inner .nodes .get_full(k) .filter(|(i, _, _)| *i < self.age.len_nodes) .is_some() } + /// Checks if there is a path from `from` to `to`. + pub fn is_path_from_to<'a>(&'a self, from: &'a N, to: &'a N) -> bool { + let mut stack = vec![from]; + let mut seen = HashSet::new(); + let inner = &self.borrow().inner; + seen.insert(from); + while let Some((_, _, iter)) = stack.pop().and_then(|p| { + inner + .nodes + .get_full(p) + .filter(|(i, _, _)| *i < self.age.len_nodes) + }) { + for (p, (c, _)) in iter.iter() { + if *c <= self.age.link_count { + if p == to { + return true; + } + if seen.insert(p) { + stack.push(p); + } + } + } + } + false + } +} + +impl Default for StackGraph { + fn default() -> StackGraph { + StackGraph::new() + } +} + +impl fmt::Debug + for StackGraph +{ + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + writeln!(fmt, "Graph {{")?; + + for (from, e) in self.borrow().inner.nodes.iter().take(self.age.len_nodes) { + writeln!(fmt, " - {}", from)?; + + for (to, (c, _)) in e.iter() { + if *c <= self.age.link_count { + writeln!(fmt, " - {}", to)?; + for edge in self.borrow().edge(from, to) { + writeln!(fmt, " - {:?}", edge)?; + } + } + } + } + + write!(fmt, "}}")?; + + Ok(()) + } +} + +pub struct StackGraphView<'a, N: Clone, E: Clone> { + inner: Ref<'a, Graph>, + age: GraphAge, +} + +impl<'a, N: Eq + Hash + Clone, E: Clone + PartialEq> StackGraphView<'a, N, E> { pub fn iter(&self) -> impl Iterator { - self.borrow().nodes.keys().take(self.age.len_nodes) + self.inner.nodes.keys().take(self.age.len_nodes) } pub fn edge(&self, from: &N, to: &N) -> Edges<'_, E> { Edges { - graph: &self.borrow().edges[..self.age.len_edges], + graph: &self.inner.edges[..self.age.len_edges], index: self - .borrow() + .inner .nodes .get(from) .and_then(|x| x.get(to).copied()) @@ -413,8 +481,8 @@ impl StackGraph { } pub fn edges(&self, from: &N) -> impl Iterator)> { - let edges = &self.borrow().edges[..self.age.len_edges]; - self.borrow() + let edges = &self.inner.edges[..self.age.len_edges]; + self.inner .nodes .get_full(from) .filter(|(i, _, _)| *i < self.age.len_nodes) @@ -434,37 +502,12 @@ impl StackGraph { }) } - /// Checks if there is a path from `from` to `to`. - pub fn is_path_from_to<'a>(&'a self, from: &'a N, to: &'a N) -> bool { - let mut stack = vec![from]; - let mut seen = HashSet::new(); - seen.insert(from); - while let Some((_, _, iter)) = stack.pop().and_then(|p| { - self.borrow() - .nodes - .get_full(p) - .filter(|(i, _, _)| *i < self.age.len_nodes) - }) { - for (p, (c, _)) in iter.iter() { - if *c <= self.age.link_count { - if p == to { - return true; - } - if seen.insert(p) { - stack.push(p); - } - } - } - } - false - } - /// Resolves one of the paths from the given dependent package down to /// a leaf. - pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { + pub fn path_to_bottom<'s>(&'s self, mut pkg: &'s N) -> Vec<&'s N> { let mut result = vec![pkg]; while let Some(p) = self - .borrow() + .inner .nodes .get_full(pkg) .filter(|(i, _, _)| *i < self.age.len_nodes) @@ -483,34 +526,3 @@ impl StackGraph { result } } - -impl Default for StackGraph { - fn default() -> StackGraph { - StackGraph::new() - } -} - -impl fmt::Debug - for StackGraph -{ - fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { - writeln!(fmt, "Graph {{")?; - - for (from, e) in self.borrow().nodes.iter().take(self.age.len_nodes) { - writeln!(fmt, " - {}", from)?; - - for (to, (c, _)) in e.iter() { - if *c <= self.age.link_count { - writeln!(fmt, " - {}", to)?; - for edge in self.edge(from, to) { - writeln!(fmt, " - {:?}", edge)?; - } - } - } - } - - write!(fmt, "}}")?; - - Ok(()) - } -} From 2c5242ae1685fd3beff442d85a23e205b68f155e Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 23 Jul 2019 14:16:36 -0400 Subject: [PATCH 06/20] add comments --- src/cargo/util/graph.rs | 52 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 2b4acdb6512..d03b1f0076b 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -13,14 +13,26 @@ struct EdgeLink { next: Option, } +/// This is a directed Graph structure. Each edge can have an `E` associated with it, +/// but may have more then one or none. Furthermore, it is designed to be "append only" so that +/// it can be queried as it would have bean when it was smaller. This allows a `reset_to` method +/// that efficiently undoes the most reason modifications. #[derive(Clone)] pub struct Graph { + /// an index based linked list of the edge data for links. This maintains insertion order. edges: Vec>, + /// every forward link in `edges` is recorded in `back_refs` so it can efficiently be undone. back_refs: Vec<(NonZeroUsize, usize)>, + /// The total number of links in the `Graph`. This needs to be kept separately as a link may + /// not have any edge data associated with it. link_count: usize, + /// a hashmap that stores the set of nodes. This is an `IndexMap` so it maintains insertion order. + /// For etch node it stores all the other nodes that it links to. + /// For etch link it stores the link number when it was inserted and optionally a first index into `edges`. nodes: indexmap::IndexMap)>>, } +/// All the data needed to query the prefix of a `Graph` #[derive(Copy, Clone, PartialEq, Debug)] struct GraphAge { link_count: usize, @@ -49,10 +61,18 @@ impl Graph { } fn reset_to(&mut self, age: GraphAge) { + // the prefix we are resetting to had `age.len_nodes`, so remove all newer nodes + assert!(self.nodes.len() >= age.len_nodes); while self.nodes.len() > age.len_nodes { self.nodes.pop(); } + + // the prefix we are resetting to had `age.link_count`, so remove all newer links + assert!(self.link_count >= age.link_count); for (_, lookup) in self.nodes.iter_mut() { + // todo: this dose not need to look at every link to remove edges + // but this is only called when a lockfile is not being used + // so it dose not need to be the fastest lookup.retain(|_, idx| { if idx.1 >= Some(age.len_edges) { idx.1 = None; @@ -61,6 +81,10 @@ impl Graph { }); } self.link_count = age.link_count; + + // the prefix we are resetting to had `age.len_edges`, so + assert!(self.edges.len() >= age.len_edges); + // fix references into the newer edges we are about to remove while self .back_refs .last() @@ -70,16 +94,19 @@ impl Graph { let (_, idx) = self.back_refs.pop().unwrap(); self.edges[idx].next = None; } + // and remove all newer edges self.edges.truncate(age.len_edges); assert_eq!(self.len(), age); } pub fn add(&mut self, node: N) { + // IndexMap happens to do exactly what we need to keep the ordering correct. self.nodes.entry(node).or_insert_with(IndexMap::new); } pub fn link(&mut self, node: N, child: N) { use indexmap::map::Entry; + // IndexMap happens to do exactly what we need to keep the ordering correct. match self .nodes .entry(node.clone()) @@ -87,15 +114,19 @@ impl Graph { .entry(child.clone()) { Entry::Vacant(entry) => { + // add the new link at the end and record the new link count self.link_count += 1; entry.insert((self.link_count, None)); } - Entry::Occupied(_) => {} + Entry::Occupied(_) => { + // this pare is already linked + } }; } pub fn add_edge(&mut self, node: N, child: N, edge: E) { use indexmap::map::Entry; + // IndexMap happens to do exactly what we need to keep the ordering correct. match self .nodes .entry(node.clone()) @@ -103,6 +134,7 @@ impl Graph { .entry(child.clone()) { Entry::Vacant(entry) => { + // add the new edge, and link and fix the new link count self.edges.push(EdgeLink { value: edge, next: None, @@ -112,9 +144,11 @@ impl Graph { entry.insert((self.link_count, Some(edge_index))); } Entry::Occupied(mut entry) => { + // this pare is already linked let edge_index = *entry.get(); match edge_index { (link_count, None) => { + // but when it was linked before it did not have edge data, so add it. self.edges.push(EdgeLink { value: edge, next: None, @@ -123,15 +157,18 @@ impl Graph { entry.insert((link_count, Some(edge_index))); } (_, Some(mut edge_index)) => loop { + // follow the linked list if self.edges[edge_index].value == edge { return; } if self.edges[edge_index].next.is_none() { + // we found the end, add the new edge self.edges.push(EdgeLink { value: edge, next: None, }); let new_index = NonZeroUsize::new(self.edges.len() - 1).unwrap(); + // make the list point to the new edge self.edges[edge_index].next = Some(new_index); self.back_refs.push((new_index, edge_index)); return; @@ -328,6 +365,16 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { } } +/// This is a directed Graph structure, that builds on the `Graph`'s "append only" internals +/// to provide: +/// - `O(1)` clone +/// - the clone has no overhead to read the `Graph` as it was +/// - no overhead over using the `Graph` directly when modifying +/// Is this too good to be true? There are two caveats: +/// - It can only be modified using a strict "Stack Discipline", only modifying the biggest clone +/// of the graph. +/// - You can drop bigger modified clones, to allow a smaller clone to be activated for modifying. +/// this "backtracking" operation can be `O(n)` #[derive(Clone)] pub struct StackGraph { inner: Rc>>, @@ -348,6 +395,7 @@ impl StackGraph { let inner = RefCell::borrow(&self.inner); assert!(self.age.len_edges <= inner.len().len_edges); assert!(self.age.len_nodes <= inner.len().len_nodes); + assert!(self.age.link_count <= inner.link_count); StackGraphView { inner, age: self.age, @@ -513,9 +561,9 @@ impl<'a, N: Eq + Hash + Clone, E: Clone + PartialEq> StackGraphView<'a, N, E> { .filter(|(i, _, _)| *i < self.age.len_nodes) .and_then(|(_, _, p)| { p.iter() + .filter(|(_, (c, _))| c <= &self.age.link_count) // Note that we can have "cycles" introduced through dev-dependency // edges, so make sure we don't loop infinitely. - .filter(|(_, (c, _))| c <= &self.age.link_count) .find(|&(node, _)| !result.contains(&node)) .map(|(p, _)| p) }) From b70616d71517f1cb0ee32740f7a37d6053e8ac36 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 24 Jul 2019 19:32:29 -0400 Subject: [PATCH 07/20] store the `back_refs` directly in edges --- src/cargo/util/graph.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index d03b1f0076b..5de177a5327 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -11,6 +11,7 @@ use std::rc::Rc; struct EdgeLink { value: E, next: Option, + previous: Option, } /// This is a directed Graph structure. Each edge can have an `E` associated with it, @@ -21,8 +22,6 @@ struct EdgeLink { pub struct Graph { /// an index based linked list of the edge data for links. This maintains insertion order. edges: Vec>, - /// every forward link in `edges` is recorded in `back_refs` so it can efficiently be undone. - back_refs: Vec<(NonZeroUsize, usize)>, /// The total number of links in the `Graph`. This needs to be kept separately as a link may /// not have any edge data associated with it. link_count: usize, @@ -54,7 +53,6 @@ impl Graph { pub fn new() -> Graph { Graph { edges: vec![], - back_refs: vec![], link_count: 0, nodes: Default::default(), } @@ -84,18 +82,18 @@ impl Graph { // the prefix we are resetting to had `age.len_edges`, so assert!(self.edges.len() >= age.len_edges); + // remove all newer edges + let to_fix: Vec = self + .edges + .drain(age.len_edges..) + .filter_map(|e| e.previous) + .filter(|idx| idx <= &age.len_edges) + .collect(); + // fix references into the newer edges we are about to remove - while self - .back_refs - .last() - .filter(|(idx, _)| idx.get() >= age.len_edges) - .is_some() - { - let (_, idx) = self.back_refs.pop().unwrap(); + for idx in to_fix { self.edges[idx].next = None; } - // and remove all newer edges - self.edges.truncate(age.len_edges); assert_eq!(self.len(), age); } @@ -138,6 +136,7 @@ impl Graph { self.edges.push(EdgeLink { value: edge, next: None, + previous: None, }); let edge_index = self.edges.len() - 1; self.link_count += 1; @@ -152,6 +151,7 @@ impl Graph { self.edges.push(EdgeLink { value: edge, next: None, + previous: None, }); let edge_index = self.edges.len() - 1; entry.insert((link_count, Some(edge_index))); @@ -166,11 +166,11 @@ impl Graph { self.edges.push(EdgeLink { value: edge, next: None, + previous: Some(edge_index), }); let new_index = NonZeroUsize::new(self.edges.len() - 1).unwrap(); // make the list point to the new edge self.edges[edge_index].next = Some(new_index); - self.back_refs.push((new_index, edge_index)); return; } edge_index = self.edges[edge_index].next.unwrap().get(); From 0fe4dc5d8bc4a0ebc1480a3b7c6a0722fcc1fe4d Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 24 Jul 2019 20:26:49 -0400 Subject: [PATCH 08/20] add new types --- src/cargo/util/graph.rs | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 5de177a5327..8f38a258206 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -7,11 +7,17 @@ use std::hash::Hash; use std::num::NonZeroUsize; use std::rc::Rc; +type EdgeIndex = usize; +type NonZeroEdgeIndex = NonZeroUsize; +type NodesIndex = usize; + #[derive(Clone, Debug)] struct EdgeLink { value: E, - next: Option, - previous: Option, + /// the index into the edge list of the next edge related to the same (from, to) nodes + next: Option, + /// the index into the edge list of the previous edge related to the same (from, to) nodes + previous: Option, } /// This is a directed Graph structure. Each edge can have an `E` associated with it, @@ -24,11 +30,11 @@ pub struct Graph { edges: Vec>, /// The total number of links in the `Graph`. This needs to be kept separately as a link may /// not have any edge data associated with it. - link_count: usize, + link_count: NodesIndex, /// a hashmap that stores the set of nodes. This is an `IndexMap` so it maintains insertion order. /// For etch node it stores all the other nodes that it links to. /// For etch link it stores the link number when it was inserted and optionally a first index into `edges`. - nodes: indexmap::IndexMap)>>, + nodes: indexmap::IndexMap)>>, } /// All the data needed to query the prefix of a `Graph` @@ -71,11 +77,11 @@ impl Graph { // todo: this dose not need to look at every link to remove edges // but this is only called when a lockfile is not being used // so it dose not need to be the fastest - lookup.retain(|_, idx| { - if idx.1 >= Some(age.len_edges) { - idx.1 = None; + lookup.retain(|_, (nodes_idx, edge_idx)| { + if *edge_idx >= Some(age.len_edges) { + *edge_idx = None; } - idx.0 <= age.link_count + *nodes_idx <= age.link_count }); } self.link_count = age.link_count; @@ -83,7 +89,7 @@ impl Graph { // the prefix we are resetting to had `age.len_edges`, so assert!(self.edges.len() >= age.len_edges); // remove all newer edges - let to_fix: Vec = self + let to_fix: Vec = self .edges .drain(age.len_edges..) .filter_map(|e| e.previous) @@ -138,7 +144,7 @@ impl Graph { next: None, previous: None, }); - let edge_index = self.edges.len() - 1; + let edge_index: EdgeIndex = self.edges.len() - 1; self.link_count += 1; entry.insert((self.link_count, Some(edge_index))); } @@ -153,7 +159,7 @@ impl Graph { next: None, previous: None, }); - let edge_index = self.edges.len() - 1; + let edge_index: EdgeIndex = self.edges.len() - 1; entry.insert((link_count, Some(edge_index))); } (_, Some(mut edge_index)) => loop { @@ -168,7 +174,8 @@ impl Graph { next: None, previous: Some(edge_index), }); - let new_index = NonZeroUsize::new(self.edges.len() - 1).unwrap(); + let new_index: NonZeroEdgeIndex = + NonZeroUsize::new(self.edges.len() - 1).unwrap(); // make the list point to the new edge self.edges[edge_index].next = Some(new_index); return; @@ -342,7 +349,7 @@ impl Eq for Graph {} #[derive(Clone, Debug)] pub struct Edges<'a, E: Clone> { graph: &'a [EdgeLink], - index: Option, + index: Option, } impl<'a, E: Clone> Edges<'a, E> { From 46e70adad424dc8cf57d1e7d1459ae57196944d5 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 24 Jul 2019 23:44:16 -0400 Subject: [PATCH 09/20] better code by not handling mixes of `link` and `add_edge` --- src/cargo/util/graph.rs | 161 ++++++++++++++++++---------------------- 1 file changed, 74 insertions(+), 87 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 8f38a258206..466e1707dd6 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -9,11 +9,10 @@ use std::rc::Rc; type EdgeIndex = usize; type NonZeroEdgeIndex = NonZeroUsize; -type NodesIndex = usize; #[derive(Clone, Debug)] struct EdgeLink { - value: E, + value: Option, /// the index into the edge list of the next edge related to the same (from, to) nodes next: Option, /// the index into the edge list of the previous edge related to the same (from, to) nodes @@ -28,19 +27,15 @@ struct EdgeLink { pub struct Graph { /// an index based linked list of the edge data for links. This maintains insertion order. edges: Vec>, - /// The total number of links in the `Graph`. This needs to be kept separately as a link may - /// not have any edge data associated with it. - link_count: NodesIndex, /// a hashmap that stores the set of nodes. This is an `IndexMap` so it maintains insertion order. - /// For etch node it stores all the other nodes that it links to. - /// For etch link it stores the link number when it was inserted and optionally a first index into `edges`. - nodes: indexmap::IndexMap)>>, + /// For each node it stores all the other nodes that it links to. + /// For each link it stores the first index into `edges`. + nodes: indexmap::IndexMap>, } /// All the data needed to query the prefix of a `Graph` #[derive(Copy, Clone, PartialEq, Debug)] struct GraphAge { - link_count: usize, len_edges: usize, len_nodes: usize, } @@ -48,7 +43,6 @@ struct GraphAge { impl Graph { fn len(&self) -> GraphAge { GraphAge { - link_count: self.link_count, len_edges: self.edges.len(), len_nodes: self.nodes.len(), } @@ -59,7 +53,6 @@ impl Graph { pub fn new() -> Graph { Graph { edges: vec![], - link_count: 0, nodes: Default::default(), } } @@ -71,20 +64,13 @@ impl Graph { self.nodes.pop(); } - // the prefix we are resetting to had `age.link_count`, so remove all newer links - assert!(self.link_count >= age.link_count); + // the prefix we are resetting to had `age.len_edges`, so remove all links pointing to newer edges for (_, lookup) in self.nodes.iter_mut() { // todo: this dose not need to look at every link to remove edges // but this is only called when a lockfile is not being used // so it dose not need to be the fastest - lookup.retain(|_, (nodes_idx, edge_idx)| { - if *edge_idx >= Some(age.len_edges) { - *edge_idx = None; - } - *nodes_idx <= age.link_count - }); + lookup.retain(|_, idx| *idx < age.len_edges); } - self.link_count = age.link_count; // the prefix we are resetting to had `age.len_edges`, so assert!(self.edges.len() >= age.len_edges); @@ -108,6 +94,9 @@ impl Graph { self.nodes.entry(node).or_insert_with(IndexMap::new); } + /// connect `node`to `child` with out associating any data. + /// Note that if this and `add_edge` are used on the same graph + /// odd things may happen when `reset_to` is called. pub fn link(&mut self, node: N, child: N) { use indexmap::map::Entry; // IndexMap happens to do exactly what we need to keep the ordering correct. @@ -118,9 +107,14 @@ impl Graph { .entry(child.clone()) { Entry::Vacant(entry) => { - // add the new link at the end and record the new link count - self.link_count += 1; - entry.insert((self.link_count, None)); + // add the new edge, and link and fix the new link count + self.edges.push(EdgeLink { + value: None, + next: None, + previous: None, + }); + let edge_index: EdgeIndex = self.edges.len() - 1; + entry.insert(edge_index); } Entry::Occupied(_) => { // this pare is already linked @@ -128,8 +122,12 @@ impl Graph { }; } + /// connect `node`to `child` associating it with `edge`. + /// Note that if this and `link` are used on the same graph + /// odd things may happen when `reset_to` is called. pub fn add_edge(&mut self, node: N, child: N, edge: E) { use indexmap::map::Entry; + let edge = Some(edge); // IndexMap happens to do exactly what we need to keep the ordering correct. match self .nodes @@ -145,43 +143,30 @@ impl Graph { previous: None, }); let edge_index: EdgeIndex = self.edges.len() - 1; - self.link_count += 1; - entry.insert((self.link_count, Some(edge_index))); + entry.insert(edge_index); } - Entry::Occupied(mut entry) => { + Entry::Occupied(entry) => { // this pare is already linked - let edge_index = *entry.get(); - match edge_index { - (link_count, None) => { - // but when it was linked before it did not have edge data, so add it. + let mut edge_index = *entry.get(); + loop { + // follow the linked list + if self.edges[edge_index].value == edge { + return; + } + if self.edges[edge_index].next.is_none() { + // we found the end, add the new edge self.edges.push(EdgeLink { value: edge, next: None, - previous: None, + previous: Some(edge_index), }); - let edge_index: EdgeIndex = self.edges.len() - 1; - entry.insert((link_count, Some(edge_index))); + let new_index: NonZeroEdgeIndex = + NonZeroUsize::new(self.edges.len() - 1).unwrap(); + // make the list point to the new edge + self.edges[edge_index].next = Some(new_index); + return; } - (_, Some(mut edge_index)) => loop { - // follow the linked list - if self.edges[edge_index].value == edge { - return; - } - if self.edges[edge_index].next.is_none() { - // we found the end, add the new edge - self.edges.push(EdgeLink { - value: edge, - next: None, - previous: Some(edge_index), - }); - let new_index: NonZeroEdgeIndex = - NonZeroUsize::new(self.edges.len() - 1).unwrap(); - // make the list point to the new edge - self.edges[edge_index].next = Some(new_index); - return; - } - edge_index = self.edges[edge_index].next.unwrap().get(); - }, + edge_index = self.edges[edge_index].next.unwrap().get(); } } } @@ -202,23 +187,19 @@ impl Graph { pub fn edge(&self, from: &N, to: &N) -> Edges<'_, E> { Edges { graph: &self.edges, - index: self - .nodes - .get(from) - .and_then(|x| x.get(to).copied()) - .and_then(|x| x.1), + index: self.nodes.get(from).and_then(|x| x.get(to).copied()), } } pub fn edges(&self, from: &N) -> impl Iterator)> { let edges = &self.edges; self.nodes.get(from).into_iter().flat_map(move |x| { - x.iter().map(move |(to, (_, idx))| { + x.iter().map(move |(to, idx)| { ( to, Edges { graph: edges, - index: *idx, + index: Some(*idx), }, ) }) @@ -354,7 +335,10 @@ pub struct Edges<'a, E: Clone> { impl<'a, E: Clone> Edges<'a, E> { pub fn is_empty(&self) -> bool { - self.index.filter(|&idx| idx < self.graph.len()).is_none() + self.index + .and_then(|idx| self.graph.get(idx)) + .and_then(|l| l.value.as_ref()) + .is_none() } } @@ -362,13 +346,14 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { type Item = &'a E; fn next(&mut self) -> Option<&'a E> { - let old_index = self.index; - old_index - .and_then(|old_index| self.graph.get(old_index)) - .map(|edge_link| { - self.index = edge_link.next.map(|i| i.get()); - &edge_link.value - }) + while let Some(edge_link) = self.index.and_then(|old_index| self.graph.get(old_index)) { + self.index = edge_link.next.map(|i| i.get()); + if let Some(value) = edge_link.value.as_ref() { + return Some(value); + } + } + self.index = None; + None } } @@ -402,7 +387,6 @@ impl StackGraph { let inner = RefCell::borrow(&self.inner); assert!(self.age.len_edges <= inner.len().len_edges); assert!(self.age.len_nodes <= inner.len().len_nodes); - assert!(self.age.link_count <= inner.link_count); StackGraphView { inner, age: self.age, @@ -425,6 +409,9 @@ impl StackGraph { }; } + /// connect `node`to `child` with out associating any data. + /// Note that if this and `add_edge` are used on the same graph + /// odd things may happen when `reset_to` is called. pub fn link(&mut self, node: N, child: N) { self.age = { let mut g = self.activate(); @@ -433,6 +420,9 @@ impl StackGraph { }; } + /// connect `node`to `child` associating it with `edge`. + /// Note that if this and `link` are used on the same graph + /// odd things may happen when `reset_to` is called. pub fn add_edge(&mut self, node: N, child: N, edge: E) { self.age = { let mut g = self.activate(); @@ -466,8 +456,8 @@ impl StackGraph { .get_full(p) .filter(|(i, _, _)| *i < self.age.len_nodes) }) { - for (p, (c, _)) in iter.iter() { - if *c <= self.age.link_count { + for (p, idx) in iter.iter() { + if *idx < self.age.len_edges { if p == to { return true; } @@ -496,8 +486,8 @@ impl fmt for (from, e) in self.borrow().inner.nodes.iter().take(self.age.len_nodes) { writeln!(fmt, " - {}", from)?; - for (to, (c, _)) in e.iter() { - if *c <= self.age.link_count { + for (to, idx) in e.iter() { + if *idx < self.age.len_edges { writeln!(fmt, " - {}", to)?; for edge in self.borrow().edge(from, to) { writeln!(fmt, " - {:?}", edge)?; @@ -530,8 +520,7 @@ impl<'a, N: Eq + Hash + Clone, E: Clone + PartialEq> StackGraphView<'a, N, E> { .nodes .get(from) .and_then(|x| x.get(to).copied()) - .filter(|(c, _)| c <= &self.age.link_count) - .and_then(|x| x.1), + .filter(|idx| idx < &self.age.len_edges), } } @@ -543,17 +532,15 @@ impl<'a, N: Eq + Hash + Clone, E: Clone + PartialEq> StackGraphView<'a, N, E> { .filter(|(i, _, _)| *i < self.age.len_nodes) .into_iter() .flat_map(move |(_, _, x)| { - x.iter() - .filter(move |(_, (c, _))| c <= &self.age.link_count) - .map(move |(to, (_, idx))| { - ( - to, - Edges { - graph: edges, - index: *idx, - }, - ) - }) + x.iter().map(move |(to, idx)| { + ( + to, + Edges { + graph: edges, + index: Some(*idx), + }, + ) + }) }) } @@ -568,7 +555,7 @@ impl<'a, N: Eq + Hash + Clone, E: Clone + PartialEq> StackGraphView<'a, N, E> { .filter(|(i, _, _)| *i < self.age.len_nodes) .and_then(|(_, _, p)| { p.iter() - .filter(|(_, (c, _))| c <= &self.age.link_count) + .filter(|(_, idx)| **idx < self.age.len_edges) // Note that we can have "cycles" introduced through dev-dependency // edges, so make sure we don't loop infinitely. .find(|&(node, _)| !result.contains(&node)) From 72431cb83b6af13e1b13ca7d7c0a96f9d0d9cefc Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 25 Jul 2019 12:23:55 -0400 Subject: [PATCH 10/20] make `reset_to` `O(delta)` --- src/cargo/util/graph.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 466e1707dd6..f0669e4b3f5 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -66,10 +66,14 @@ impl Graph { // the prefix we are resetting to had `age.len_edges`, so remove all links pointing to newer edges for (_, lookup) in self.nodes.iter_mut() { - // todo: this dose not need to look at every link to remove edges - // but this is only called when a lockfile is not being used - // so it dose not need to be the fastest - lookup.retain(|_, idx| *idx < age.len_edges); + while lookup.len() >= 1 + && lookup + .get_index(lookup.len() - 1) + .filter(|(_, idx)| idx >= &&age.len_edges) + .is_some() + { + lookup.pop(); + } } // the prefix we are resetting to had `age.len_edges`, so From f4538653dfe4b51267fb8c7c8f5d3bc8d95d1a42 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 25 Jul 2019 17:25:52 -0400 Subject: [PATCH 11/20] start on small cleanups --- src/cargo/util/graph.rs | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index f0669e4b3f5..159221d28b1 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -106,9 +106,9 @@ impl Graph { // IndexMap happens to do exactly what we need to keep the ordering correct. match self .nodes - .entry(node.clone()) + .entry(node) .or_insert_with(IndexMap::new) - .entry(child.clone()) + .entry(child) { Entry::Vacant(entry) => { // add the new edge, and link and fix the new link count @@ -121,23 +121,21 @@ impl Graph { entry.insert(edge_index); } Entry::Occupied(_) => { - // this pare is already linked + // this pair is already linked } }; } /// connect `node`to `child` associating it with `edge`. - /// Note that if this and `link` are used on the same graph - /// odd things may happen when `reset_to` is called. pub fn add_edge(&mut self, node: N, child: N, edge: E) { use indexmap::map::Entry; let edge = Some(edge); // IndexMap happens to do exactly what we need to keep the ordering correct. match self .nodes - .entry(node.clone()) + .entry(node) .or_insert_with(IndexMap::new) - .entry(child.clone()) + .entry(child) { Entry::Vacant(entry) => { // add the new edge, and link and fix the new link count @@ -350,7 +348,11 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { type Item = &'a E; fn next(&mut self) -> Option<&'a E> { - while let Some(edge_link) = self.index.and_then(|old_index| self.graph.get(old_index)) { + while let Some(edge_link) = self.index.and_then(|idx| { + // Check that the `idx` points to something in `self.graph`. It may not if we are + // looking at a smaller prefix of a larger graph. + self.graph.get(idx) + }) { self.index = edge_link.next.map(|i| i.get()); if let Some(value) = edge_link.value.as_ref() { return Some(value); @@ -414,8 +416,6 @@ impl StackGraph { } /// connect `node`to `child` with out associating any data. - /// Note that if this and `add_edge` are used on the same graph - /// odd things may happen when `reset_to` is called. pub fn link(&mut self, node: N, child: N) { self.age = { let mut g = self.activate(); @@ -425,8 +425,6 @@ impl StackGraph { } /// connect `node`to `child` associating it with `edge`. - /// Note that if this and `link` are used on the same graph - /// odd things may happen when `reset_to` is called. pub fn add_edge(&mut self, node: N, child: N, edge: E) { self.age = { let mut g = self.activate(); From f8c224b060761b4f0d87f90f9a1c9c1dcb32ea5b Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 25 Jul 2019 17:30:56 -0400 Subject: [PATCH 12/20] remove the `NonZeroUsize` optimization --- src/cargo/util/graph.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 159221d28b1..480b3d28cf2 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -4,17 +4,15 @@ use std::cell::{Ref, RefCell, RefMut}; use std::collections::HashSet; use std::fmt; use std::hash::Hash; -use std::num::NonZeroUsize; use std::rc::Rc; type EdgeIndex = usize; -type NonZeroEdgeIndex = NonZeroUsize; #[derive(Clone, Debug)] struct EdgeLink { value: Option, /// the index into the edge list of the next edge related to the same (from, to) nodes - next: Option, + next: Option, // can be `NonZeroUsize` but not worth the boilerplate /// the index into the edge list of the previous edge related to the same (from, to) nodes previous: Option, } @@ -162,13 +160,12 @@ impl Graph { next: None, previous: Some(edge_index), }); - let new_index: NonZeroEdgeIndex = - NonZeroUsize::new(self.edges.len() - 1).unwrap(); + let new_index: EdgeIndex = self.edges.len() - 1; // make the list point to the new edge self.edges[edge_index].next = Some(new_index); return; } - edge_index = self.edges[edge_index].next.unwrap().get(); + edge_index = self.edges[edge_index].next.unwrap(); } } } @@ -353,7 +350,7 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { // looking at a smaller prefix of a larger graph. self.graph.get(idx) }) { - self.index = edge_link.next.map(|i| i.get()); + self.index = edge_link.next; if let Some(value) = edge_link.value.as_ref() { return Some(value); } From ca19f42974285623e2998f8dbf272f1eae34d5bb Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 25 Jul 2019 17:57:59 -0400 Subject: [PATCH 13/20] The "Stack Discipline" is checked on best effort basis --- src/cargo/util/graph.rs | 80 ++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 36 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 480b3d28cf2..705cac50729 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -326,40 +326,6 @@ impl PartialEq for Graph } impl Eq for Graph {} -#[derive(Clone, Debug)] -pub struct Edges<'a, E: Clone> { - graph: &'a [EdgeLink], - index: Option, -} - -impl<'a, E: Clone> Edges<'a, E> { - pub fn is_empty(&self) -> bool { - self.index - .and_then(|idx| self.graph.get(idx)) - .and_then(|l| l.value.as_ref()) - .is_none() - } -} - -impl<'a, E: Clone> Iterator for Edges<'a, E> { - type Item = &'a E; - - fn next(&mut self) -> Option<&'a E> { - while let Some(edge_link) = self.index.and_then(|idx| { - // Check that the `idx` points to something in `self.graph`. It may not if we are - // looking at a smaller prefix of a larger graph. - self.graph.get(idx) - }) { - self.index = edge_link.next; - if let Some(value) = edge_link.value.as_ref() { - return Some(value); - } - } - self.index = None; - None - } -} - /// This is a directed Graph structure, that builds on the `Graph`'s "append only" internals /// to provide: /// - `O(1)` clone @@ -370,6 +336,13 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { /// of the graph. /// - You can drop bigger modified clones, to allow a smaller clone to be activated for modifying. /// this "backtracking" operation can be `O(n)` +/// +/// The "Stack Discipline" is checked on best effort basis. If you attempt to read or write to a +/// bigger clone after modifying a smaller clone it will probably panic. It may not panic if you +/// arrange to have increased the size back to the size of the bigger clone, this is a kind of ABA problem. +/// (If this terns out to be to hard to get right we can use "generational indices" to all ways panic.) +/// This module dose not use `unsafe` so violating the "Stack Discipline" may return the wrong +/// answer but will not trigger UB. #[derive(Clone)] pub struct StackGraph { inner: Rc>>, @@ -388,8 +361,8 @@ impl StackGraph { pub fn borrow(&self) -> StackGraphView<'_, N, E> { let inner = RefCell::borrow(&self.inner); - assert!(self.age.len_edges <= inner.len().len_edges); - assert!(self.age.len_nodes <= inner.len().len_nodes); + assert!(self.age.len_edges <= inner.len().len_edges, "The internal Graph was reset to something smaller before you tried to read from the StackGraphView"); + assert!(self.age.len_nodes <= inner.len().len_nodes, "The internal Graph was reset to something smaller before you tried to read from the StackGraphView"); StackGraphView { inner, age: self.age, @@ -401,6 +374,7 @@ impl StackGraph { if self.age != inner.len() { inner.reset_to(self.age); } + assert_eq!(inner.len(), self.age, "The internal Graph was reset to something smaller before you tried to write to the StackGraphView"); inner } @@ -501,6 +475,40 @@ impl fmt } } +#[derive(Clone, Debug)] +pub struct Edges<'a, E: Clone> { + graph: &'a [EdgeLink], + index: Option, +} + +impl<'a, E: Clone> Edges<'a, E> { + pub fn is_empty(&self) -> bool { + self.index + .and_then(|idx| self.graph.get(idx)) + .and_then(|l| l.value.as_ref()) + .is_none() + } +} + +impl<'a, E: Clone> Iterator for Edges<'a, E> { + type Item = &'a E; + + fn next(&mut self) -> Option<&'a E> { + while let Some(edge_link) = self.index.and_then(|idx| { + // Check that the `idx` points to something in `self.graph`. It may not if we are + // looking at a smaller prefix of a larger graph. + self.graph.get(idx) + }) { + self.index = edge_link.next; + if let Some(value) = edge_link.value.as_ref() { + return Some(value); + } + } + self.index = None; + None + } +} + pub struct StackGraphView<'a, N: Clone, E: Clone> { inner: Ref<'a, Graph>, age: GraphAge, From af7e6b6b1bbdf775be1818115bad9dbff94d2a55 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 26 Jul 2019 14:13:38 -0400 Subject: [PATCH 14/20] document why `reset_to` works --- src/cargo/util/graph.rs | 42 +++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 705cac50729..d44909066d1 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -31,10 +31,15 @@ pub struct Graph { nodes: indexmap::IndexMap>, } -/// All the data needed to query the prefix of a `Graph` +/// All the data needed to query the prefix of a `Graph`. The only way for eny of the `len_` in +/// this to decrease is to call `reset_to`. All other modifications of a graph will increase at +/// least one of the `len_`. #[derive(Copy, Clone, PartialEq, Debug)] struct GraphAge { + /// The number of stored edges, increased when `add_edge` or `link` is called. len_edges: usize, + /// The number of stored nodes, increased when `add` is called or + /// if `add_edge` or `link` is called with a previously unseen node. len_nodes: usize, } @@ -55,15 +60,20 @@ impl Graph { } } + /// This resets a Graph to the same state as it was when the passed `age` was made. + /// All the other `&mut` methods are guaranteed to increase the `len` of the Graph. + /// So the reset can be accomplished by throwing out all newer items and fixing internal pointers. fn reset_to(&mut self, age: GraphAge) { // the prefix we are resetting to had `age.len_nodes`, so remove all newer nodes assert!(self.nodes.len() >= age.len_nodes); + // IndexMap dose not have a `truncate` so we roll our own while self.nodes.len() > age.len_nodes { self.nodes.pop(); } // the prefix we are resetting to had `age.len_edges`, so remove all links pointing to newer edges for (_, lookup) in self.nodes.iter_mut() { + // IndexMap dose not have a `last` so we roll our own while lookup.len() >= 1 && lookup .get_index(lookup.len() - 1) @@ -76,7 +86,7 @@ impl Graph { // the prefix we are resetting to had `age.len_edges`, so assert!(self.edges.len() >= age.len_edges); - // remove all newer edges + // remove all newer edges and record the references that need to be fixed let to_fix: Vec = self .edges .drain(age.len_edges..) @@ -84,7 +94,7 @@ impl Graph { .filter(|idx| idx <= &age.len_edges) .collect(); - // fix references into the newer edges we are about to remove + // fix references into the newer edges we remove for idx in to_fix { self.edges[idx].next = None; } @@ -93,15 +103,15 @@ impl Graph { pub fn add(&mut self, node: N) { // IndexMap happens to do exactly what we need to keep the ordering correct. + // This can be undone as it will increase the `len_nodes` if `node` is new. self.nodes.entry(node).or_insert_with(IndexMap::new); } /// connect `node`to `child` with out associating any data. - /// Note that if this and `add_edge` are used on the same graph - /// odd things may happen when `reset_to` is called. pub fn link(&mut self, node: N, child: N) { use indexmap::map::Entry; // IndexMap happens to do exactly what we need to keep the ordering correct. + // This can be undone as it will increase the `len_nodes` if `node` is new. match self .nodes .entry(node) @@ -109,7 +119,8 @@ impl Graph { .entry(child) { Entry::Vacant(entry) => { - // add the new edge, and link and fix the new link count + // add the new edge and link + // This can be undone as it will increase the `len_edges`. self.edges.push(EdgeLink { value: None, next: None, @@ -129,6 +140,7 @@ impl Graph { use indexmap::map::Entry; let edge = Some(edge); // IndexMap happens to do exactly what we need to keep the ordering correct. + // This can be undone as it will increase the `len_nodes` if `node` is new. match self .nodes .entry(node) @@ -136,7 +148,8 @@ impl Graph { .entry(child) { Entry::Vacant(entry) => { - // add the new edge, and link and fix the new link count + // add the new edge and link + // This can be undone as it will increase the `len_edges`. self.edges.push(EdgeLink { value: edge, next: None, @@ -155,6 +168,7 @@ impl Graph { } if self.edges[edge_index].next.is_none() { // we found the end, add the new edge + // This can be undone as it will increase the `len_edges`. self.edges.push(EdgeLink { value: edge, next: None, @@ -335,7 +349,7 @@ impl Eq for Graph {} /// - It can only be modified using a strict "Stack Discipline", only modifying the biggest clone /// of the graph. /// - You can drop bigger modified clones, to allow a smaller clone to be activated for modifying. -/// this "backtracking" operation can be `O(n)` +/// this "backtracking" operation can be `O(delta)` /// /// The "Stack Discipline" is checked on best effort basis. If you attempt to read or write to a /// bigger clone after modifying a smaller clone it will probably panic. It may not panic if you @@ -403,7 +417,12 @@ impl StackGraph { g.len() }; } +} +/// Methods for viewing the prefix of a `Graph` as stored in a `StackGraph`. +/// Other views of the inner `Graph` may have added things after this `StackGraph` was created. +/// So, we need to filter everything to only the prefix recorded by `self.age`. +impl StackGraph { pub fn contains(&self, k: &Q) -> bool where N: Borrow, @@ -475,8 +494,12 @@ impl fmt } } +/// And Iterator of the edges related to one link in a `Graph` or a `StackGraphView`. #[derive(Clone, Debug)] pub struct Edges<'a, E: Clone> { + /// The arena the linked list is stored in. If we are used with a `StackGraphView` + /// then this is only the related prefix of the arena. So we need to filter out + /// pointers past the part we are given. graph: &'a [EdgeLink], index: Option, } @@ -509,6 +532,9 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { } } +/// A RAII gard that allows getting `&` references to the prefix of a `Graph` as stored in a `StackGraph`. +/// Other views of the inner `Graph` may have added things after this `StackGraph` was created. +/// So, we need to filter everything to only the prefix recorded by `self.age`. pub struct StackGraphView<'a, N: Clone, E: Clone> { inner: Ref<'a, Graph>, age: GraphAge, From d52b7d2afea6d67cf4493bb5e26bcccca61d021f Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 27 Jul 2019 17:51:38 -0400 Subject: [PATCH 15/20] add module documentation --- src/cargo/util/graph.rs | 96 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index d44909066d1..fa836029608 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -1,3 +1,59 @@ +//! This module implements some Graph data structures optimized for Cargo's uses. +//! The module documentation here will try to justify why Cargo needs the odd set of requirements +//! that the structures provide, but the how is documented on each type. +//! +//! Cargo uses these types for several things in the codebase, but by far the most important is +//! the "dependency graph". It is a graph that records for each package all the packages that it +//! depends on. It is used to decide what order to build the packages, for what targets, and when +//! they need to be rebuilt (among many other things). One small complication is that we may have +//! more then one edge between the same two packages because the same package might be present +//! in both `[dependencies]` and `[build-dependencies]`. +//! This module must provide a way to read it efficiently with no fuss nor complications. The +//! "dependency graph" is constructed in two ways, when reading a lockfile and by running the resolver. +//! When reading a lockfile the only complication is that the lockfile has what is depended on but +//! not why. This module must provide a way to link two packages with out providing an example +//! of the type of the edge. +//! Running the resolver is the source of our most unusual requirements. +//! The fundamental pattern of the resolver is like: +//! +//! ```ignore +//! fn resolve_next(dependency_graph_so_far: _, mut unresolved_dependencies: _) -> Result { +//! if let Some(unresolved) = unresolved_dependencies.pop() { +//! let combined_problem = Problem::new(); +//! for candidate in unresolved { +//! let mut dgsf = dependency_graph_so_far.clone(); +//! let mut ud = unresolved_dependencies.clone(); +//! if let Err(prob) = activate(&mut dgsf, &mut ud, candidate) { +//! combined_problem.extend(prob); +//! continue +//! }; +//! match resolve_next(dgsf, ud) { +//! Ok(dg) => return Ok(dg), +//! Err(prob) => { +//! if dependency_graph_so_far.has(prob) { +//! return Err(prob) +//! } +//! } +//! } +//! } +//! Err(combined_problem) +//! } else { +//! Ok(finalize(dependency_graph_so_far)) +//! } +//! } +//! ``` +//! +//! The real resolver is not recursive to avoid blowing the stack, and has lots of other state to +//! maintain. The most expensive (non recursive) call in this algorithm is the +//! `dependency_graph_so_far.clone();`. To make this more annoying the first thing we try will +//! probably work, and any work we do to prepare for the next iteration is wasted. If we had a +//! `undo_activate` we could be much more efficient, completely remove the `.clone()` and just +//! `undo_activate` if things tern out to not work. Unfortunately, making shore `undo_activate` +//! handles all the corner cases correctly is not practical for the resolver. However, this is +//! possible for a graph like thing, a `clone` means record the size of the graph. a `&mut self` +//! method means undo everything newer and do the mutation, a `&self` method means only look at the +//! older part. This module provides a `StackGraph` type to encapsulate this pattern. + use indexmap::IndexMap; use std::borrow::Borrow; use std::cell::{Ref, RefCell, RefMut}; @@ -91,7 +147,7 @@ impl Graph { .edges .drain(age.len_edges..) .filter_map(|e| e.previous) - .filter(|idx| idx <= &age.len_edges) + .filter(|idx| idx < &age.len_edges) .collect(); // fix references into the newer edges we remove @@ -363,6 +419,41 @@ pub struct StackGraph { age: GraphAge, } +#[test] +fn demonstrate_stack_graph_can_read_all_clones() { + let mut graph = StackGraph::new(); + let mut stack = Vec::new(); + graph.add_edge(1, 2, 1); + stack.push(graph.clone()); + graph.add_edge(2, 3, 2); + stack.push(graph.clone()); + graph.add_edge(2, 3, 3); + stack.push(graph.clone()); + assert_eq!( + stack.iter().map(|g| g.contains(&2)).collect::>(), + [false, true, true] + ); + assert_eq!(stack[1].borrow().edge(&2, &3).collect::>(), [&2]); + assert_eq!(stack[2].borrow().edge(&2, &3).collect::>(), [&2, &3]); +} + +#[test] +#[should_panic] +fn demonstrate_stack_graph_some_violations_panic() { + let mut graph = StackGraph::new(); + let mut stack = Vec::new(); + graph.add_edge(1, 2, 1); + stack.push(graph.clone()); + graph.add_edge(2, 3, 2); + stack.push(graph.clone()); + graph.add_edge(2, 3, 3); + stack.push(graph.clone()); + // violate stack discipline + stack[0].clone().add_edge(2, 3, 4); + + stack[2].borrow(); +} + impl StackGraph { pub fn new() -> StackGraph { let inner = Graph::new(); @@ -533,7 +624,8 @@ impl<'a, E: Clone> Iterator for Edges<'a, E> { } /// A RAII gard that allows getting `&` references to the prefix of a `Graph` as stored in a `StackGraph`. -/// Other views of the inner `Graph` may have added things after this `StackGraph` was created. +/// Other clones of the inner `StackGraph` may have added things after this `StackGraph` was created +/// and before this `StackGraphView` was `.borrow()`ed. /// So, we need to filter everything to only the prefix recorded by `self.age`. pub struct StackGraphView<'a, N: Clone, E: Clone> { inner: Ref<'a, Graph>, From f7d504124f91da8ebc15b76e14d8ffcaf42fc41f Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 27 Jul 2019 23:34:46 -0400 Subject: [PATCH 16/20] Clone more then needed to remove the need for "Stack Discipline" --- src/cargo/util/graph.rs | 52 ++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index fa836029608..f13c545d2d1 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -400,19 +400,10 @@ impl Eq for Graph {} /// to provide: /// - `O(1)` clone /// - the clone has no overhead to read the `Graph` as it was -/// - no overhead over using the `Graph` directly when modifying -/// Is this too good to be true? There are two caveats: -/// - It can only be modified using a strict "Stack Discipline", only modifying the biggest clone -/// of the graph. -/// - You can drop bigger modified clones, to allow a smaller clone to be activated for modifying. -/// this "backtracking" operation can be `O(delta)` -/// -/// The "Stack Discipline" is checked on best effort basis. If you attempt to read or write to a -/// bigger clone after modifying a smaller clone it will probably panic. It may not panic if you -/// arrange to have increased the size back to the size of the bigger clone, this is a kind of ABA problem. -/// (If this terns out to be to hard to get right we can use "generational indices" to all ways panic.) -/// This module dose not use `unsafe` so violating the "Stack Discipline" may return the wrong -/// answer but will not trigger UB. +/// - no overhead over using the `Graph` directly when modifying the biggest clone +/// Is this too good to be true? +/// - If a modification (`&mut` method) is done to a smaller older clone then a full `O(N)` +/// deep clone will happen internally. #[derive(Clone)] pub struct StackGraph { inner: Rc>>, @@ -429,29 +420,16 @@ fn demonstrate_stack_graph_can_read_all_clones() { stack.push(graph.clone()); graph.add_edge(2, 3, 3); stack.push(graph.clone()); + // violate stack discipline, so a deep clone is needed + graph = stack[0].clone(); + graph.add_edge(2, 3, 4); assert_eq!( stack.iter().map(|g| g.contains(&2)).collect::>(), [false, true, true] ); assert_eq!(stack[1].borrow().edge(&2, &3).collect::>(), [&2]); assert_eq!(stack[2].borrow().edge(&2, &3).collect::>(), [&2, &3]); -} - -#[test] -#[should_panic] -fn demonstrate_stack_graph_some_violations_panic() { - let mut graph = StackGraph::new(); - let mut stack = Vec::new(); - graph.add_edge(1, 2, 1); - stack.push(graph.clone()); - graph.add_edge(2, 3, 2); - stack.push(graph.clone()); - graph.add_edge(2, 3, 3); - stack.push(graph.clone()); - // violate stack discipline - stack[0].clone().add_edge(2, 3, 4); - - stack[2].borrow(); + assert_eq!(graph.borrow().edge(&2, &3).collect::>(), [&4]); } impl StackGraph { @@ -475,11 +453,17 @@ impl StackGraph { } fn activate(&mut self) -> RefMut<'_, Graph> { - let mut inner = RefCell::borrow_mut(&mut self.inner); - if self.age != inner.len() { + let inner = if RefCell::borrow(&mut self.inner).len() == self.age { + // we are the biggest clone, so we can add to inner directly. + RefCell::borrow_mut(&mut self.inner) + } else { + // some other clone already modified inner. If the other bigger clone still exists + // then we need to deep clone. We dont know if it exists, so clone to be conservative. + let new = Rc::make_mut(&mut self.inner); + let mut inner = RefCell::borrow_mut(new); inner.reset_to(self.age); - } - assert_eq!(inner.len(), self.age, "The internal Graph was reset to something smaller before you tried to write to the StackGraphView"); + inner + }; inner } From a3589664af2f64728d2145477b8f816e00f0a96c Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 30 Jul 2019 14:59:38 -0400 Subject: [PATCH 17/20] Fuller check to remove deep clones --- src/cargo/util/graph.rs | 87 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 10 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index f13c545d2d1..ac072abc651 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -48,7 +48,7 @@ //! `dependency_graph_so_far.clone();`. To make this more annoying the first thing we try will //! probably work, and any work we do to prepare for the next iteration is wasted. If we had a //! `undo_activate` we could be much more efficient, completely remove the `.clone()` and just -//! `undo_activate` if things tern out to not work. Unfortunately, making shore `undo_activate` +//! `undo_activate` if things tern out to not work. Unfortunately, making sure `undo_activate` //! handles all the corner cases correctly is not practical for the resolver. However, this is //! possible for a graph like thing, a `clone` means record the size of the graph. a `&mut self` //! method means undo everything newer and do the mutation, a `&self` method means only look at the @@ -57,7 +57,7 @@ use indexmap::IndexMap; use std::borrow::Borrow; use std::cell::{Ref, RefCell, RefMut}; -use std::collections::HashSet; +use std::collections::{BTreeSet, HashSet}; use std::fmt; use std::hash::Hash; use std::rc::Rc; @@ -75,7 +75,7 @@ struct EdgeLink { /// This is a directed Graph structure. Each edge can have an `E` associated with it, /// but may have more then one or none. Furthermore, it is designed to be "append only" so that -/// it can be queried as it would have bean when it was smaller. This allows a `reset_to` method +/// it can be queried as it would have been when it was smaller. This allows a `reset_to` method /// that efficiently undoes the most reason modifications. #[derive(Clone)] pub struct Graph { @@ -90,7 +90,7 @@ pub struct Graph { /// All the data needed to query the prefix of a `Graph`. The only way for eny of the `len_` in /// this to decrease is to call `reset_to`. All other modifications of a graph will increase at /// least one of the `len_`. -#[derive(Copy, Clone, PartialEq, Debug)] +#[derive(Copy, Clone, PartialEq, Debug, Ord, PartialOrd, Eq)] struct GraphAge { /// The number of stored edges, increased when `add_edge` or `link` is called. len_edges: usize, @@ -398,16 +398,24 @@ impl Eq for Graph {} /// This is a directed Graph structure, that builds on the `Graph`'s "append only" internals /// to provide: -/// - `O(1)` clone +/// - `O(ln(number of clones))` clone, no dependence on the size of the graph /// - the clone has no overhead to read the `Graph` as it was /// - no overhead over using the `Graph` directly when modifying the biggest clone /// Is this too good to be true? /// - If a modification (`&mut` method) is done to a smaller older clone then a full `O(N)` /// deep clone will happen internally. -#[derive(Clone)] pub struct StackGraph { + /// The `Graph` shared by all clones, this `StackGraph` refers to only the prefix of size `age` inner: Rc>>, + /// The shared list of all extant references to the same `Graph`. + /// The largest one is allowed to reset and expend the `Graph` without doing a deep clone. + /// There can be more then one clone at the same age, so each one is associated with a unique arbitrary number. + /// This all so stores the largest arbitrary number so far used. + other_refs: Rc, usize)>>, + /// The size of the `Graph` that this `StackGraph` refers to. age: GraphAge, + /// The arbitrary number that is unique to this clone. + this_ref: usize, } #[test] @@ -432,12 +440,42 @@ fn demonstrate_stack_graph_can_read_all_clones() { assert_eq!(graph.borrow().edge(&2, &3).collect::>(), [&4]); } +impl Clone for StackGraph { + fn clone(&self) -> Self { + let count; + { + let mut other_refs = RefCell::borrow_mut(&self.other_refs); + other_refs.1 += 1; + count = other_refs.1; + other_refs.0.insert((self.age, count)); + } + StackGraph { + inner: Rc::clone(&self.inner), + other_refs: Rc::clone(&self.other_refs), + this_ref: count, + age: self.age, + } + } +} + +impl Drop for StackGraph { + fn drop(&mut self) { + RefCell::borrow_mut(&self.other_refs) + .0 + .remove(&(self.age, self.this_ref)); + } +} + impl StackGraph { pub fn new() -> StackGraph { let inner = Graph::new(); let age = inner.len(); + let mut other_refs = BTreeSet::new(); + other_refs.insert((age, 0)); StackGraph { inner: Rc::new(RefCell::new(inner)), + other_refs: Rc::new(RefCell::new((other_refs, 0))), + this_ref: 0, age, } } @@ -452,13 +490,33 @@ impl StackGraph { } } + /// Gets mutable access to the inner `Graph`. Uses `other_refs` to determine if a deep clone is + /// needed, and runs `reset_to` to get the `Graph` into the state it was in. + /// + /// It is the responsibility of the caller to re-add this clone to `other_refs` after the new age is determined. fn activate(&mut self) -> RefMut<'_, Graph> { - let inner = if RefCell::borrow(&mut self.inner).len() == self.age { + let inner = if { + let mut borrow = RefCell::borrow_mut(&self.other_refs); + borrow.0.remove(&(self.age, self.this_ref)); + borrow + .0 + .iter() + .rev() + .next() + .map(|(a, _)| a <= &self.age) + .unwrap_or(true) + } { // we are the biggest clone, so we can add to inner directly. - RefCell::borrow_mut(&mut self.inner) + let mut inner = RefCell::borrow_mut(&mut self.inner); + if inner.len() != self.age { + // clean up after the larger clone that has since been dropped + inner.reset_to(self.age); + } + inner } else { - // some other clone already modified inner. If the other bigger clone still exists - // then we need to deep clone. We dont know if it exists, so clone to be conservative. + // a bigger clone still exists so do a deep clone. + self.other_refs = Rc::new(RefCell::new((BTreeSet::new(), 0))); + self.this_ref = 0; let new = Rc::make_mut(&mut self.inner); let mut inner = RefCell::borrow_mut(new); inner.reset_to(self.age); @@ -473,6 +531,9 @@ impl StackGraph { g.add(node); g.len() }; + RefCell::borrow_mut(&self.other_refs) + .0 + .insert((self.age, self.this_ref)); } /// connect `node`to `child` with out associating any data. @@ -482,6 +543,9 @@ impl StackGraph { g.link(node, child); g.len() }; + RefCell::borrow_mut(&self.other_refs) + .0 + .insert((self.age, self.this_ref)); } /// connect `node`to `child` associating it with `edge`. @@ -491,6 +555,9 @@ impl StackGraph { g.add_edge(node, child, edge); g.len() }; + RefCell::borrow_mut(&self.other_refs) + .0 + .insert((self.age, self.this_ref)); } } From 2340384b330e7d82b1f237c6a5ed460b919a5ed6 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 22 Aug 2019 13:02:37 -0400 Subject: [PATCH 18/20] Better way to do a multi set --- src/cargo/util/graph.rs | 70 ++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 36 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index ac072abc651..b01a7180e20 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -57,7 +57,7 @@ use indexmap::IndexMap; use std::borrow::Borrow; use std::cell::{Ref, RefCell, RefMut}; -use std::collections::{BTreeSet, HashSet}; +use std::collections::{BTreeMap, HashSet}; use std::fmt; use std::hash::Hash; use std::rc::Rc; @@ -409,13 +409,10 @@ pub struct StackGraph { inner: Rc>>, /// The shared list of all extant references to the same `Graph`. /// The largest one is allowed to reset and expend the `Graph` without doing a deep clone. - /// There can be more then one clone at the same age, so each one is associated with a unique arbitrary number. - /// This all so stores the largest arbitrary number so far used. - other_refs: Rc, usize)>>, + /// There can be more then one clone at the same age, so each age is associated with a count. + other_refs: Rc>>, /// The size of the `Graph` that this `StackGraph` refers to. age: GraphAge, - /// The arbitrary number that is unique to this clone. - this_ref: usize, } #[test] @@ -442,17 +439,12 @@ fn demonstrate_stack_graph_can_read_all_clones() { impl Clone for StackGraph { fn clone(&self) -> Self { - let count; - { - let mut other_refs = RefCell::borrow_mut(&self.other_refs); - other_refs.1 += 1; - count = other_refs.1; - other_refs.0.insert((self.age, count)); - } + *RefCell::borrow_mut(&self.other_refs) + .entry(self.age) + .or_insert(0) += 1; StackGraph { inner: Rc::clone(&self.inner), other_refs: Rc::clone(&self.other_refs), - this_ref: count, age: self.age, } } @@ -460,9 +452,13 @@ impl Clone for StackGraph { impl Drop for StackGraph { fn drop(&mut self) { - RefCell::borrow_mut(&self.other_refs) - .0 - .remove(&(self.age, self.this_ref)); + let mut borrow = RefCell::borrow_mut(&self.other_refs); + if let std::collections::btree_map::Entry::Occupied(mut val) = borrow.entry(self.age) { + *val.get_mut() -= 1; + if val.get() == &0 { + val.remove(); + } + } } } @@ -470,12 +466,11 @@ impl StackGraph { pub fn new() -> StackGraph { let inner = Graph::new(); let age = inner.len(); - let mut other_refs = BTreeSet::new(); - other_refs.insert((age, 0)); + let mut other_refs = BTreeMap::new(); + other_refs.insert(age, 1); StackGraph { inner: Rc::new(RefCell::new(inner)), - other_refs: Rc::new(RefCell::new((other_refs, 0))), - this_ref: 0, + other_refs: Rc::new(RefCell::new(other_refs)), age, } } @@ -497,13 +492,17 @@ impl StackGraph { fn activate(&mut self) -> RefMut<'_, Graph> { let inner = if { let mut borrow = RefCell::borrow_mut(&self.other_refs); - borrow.0.remove(&(self.age, self.this_ref)); + if let std::collections::btree_map::Entry::Occupied(mut val) = borrow.entry(self.age) { + *val.get_mut() -= 1; + if val.get() == &0 { + val.remove(); + } + } borrow - .0 - .iter() + .keys() .rev() .next() - .map(|(a, _)| a <= &self.age) + .map(|a| a <= &self.age) .unwrap_or(true) } { // we are the biggest clone, so we can add to inner directly. @@ -515,8 +514,7 @@ impl StackGraph { inner } else { // a bigger clone still exists so do a deep clone. - self.other_refs = Rc::new(RefCell::new((BTreeSet::new(), 0))); - self.this_ref = 0; + self.other_refs = Rc::new(RefCell::new(BTreeMap::new())); let new = Rc::make_mut(&mut self.inner); let mut inner = RefCell::borrow_mut(new); inner.reset_to(self.age); @@ -531,9 +529,9 @@ impl StackGraph { g.add(node); g.len() }; - RefCell::borrow_mut(&self.other_refs) - .0 - .insert((self.age, self.this_ref)); + *RefCell::borrow_mut(&self.other_refs) + .entry(self.age) + .or_insert(0) += 1; } /// connect `node`to `child` with out associating any data. @@ -543,9 +541,9 @@ impl StackGraph { g.link(node, child); g.len() }; - RefCell::borrow_mut(&self.other_refs) - .0 - .insert((self.age, self.this_ref)); + *RefCell::borrow_mut(&self.other_refs) + .entry(self.age) + .or_insert(0) += 1; } /// connect `node`to `child` associating it with `edge`. @@ -555,9 +553,9 @@ impl StackGraph { g.add_edge(node, child, edge); g.len() }; - RefCell::borrow_mut(&self.other_refs) - .0 - .insert((self.age, self.this_ref)); + *RefCell::borrow_mut(&self.other_refs) + .entry(self.age) + .or_insert(0) += 1; } } From 451f76d1937d717238bffc8e337c043a97059334 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 22 Aug 2019 14:55:26 -0400 Subject: [PATCH 19/20] remove some duplication --- src/cargo/core/resolver/errors.rs | 14 +++--- src/cargo/util/graph.rs | 75 +++++++++++-------------------- 2 files changed, 33 insertions(+), 56 deletions(-) diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs index e835c183b3c..94762aa0120 100644 --- a/src/cargo/core/resolver/errors.rs +++ b/src/cargo/core/resolver/errors.rs @@ -77,11 +77,11 @@ pub(super) fn activation_error( candidates: &[Summary], config: Option<&Config>, ) -> ResolveError { + let parents = cx.parents.clone_into_graph(); let to_resolve_err = |err| { ResolveError::new( err, - cx.parents - .borrow() + parents .path_to_bottom(&parent.package_id()) .into_iter() .cloned() @@ -93,7 +93,7 @@ pub(super) fn activation_error( let mut msg = format!("failed to select a version for `{}`.", dep.package_name()); msg.push_str("\n ... required by "); msg.push_str(&describe_path( - &cx.parents.borrow().path_to_bottom(&parent.package_id()), + &parents.path_to_bottom(&parent.package_id()), )); msg.push_str("\nversions that meet the requirements `"); @@ -125,7 +125,7 @@ pub(super) fn activation_error( msg.push_str(link); msg.push_str("` as well:\n"); } - msg.push_str(&describe_path(&cx.parents.borrow().path_to_bottom(p))); + msg.push_str(&describe_path(&parents.path_to_bottom(p))); } let (features_errors, mut other_errors): (Vec<_>, Vec<_>) = other_errors @@ -179,7 +179,7 @@ pub(super) fn activation_error( for &(p, _) in other_errors.iter() { msg.push_str("\n\n previously selected "); - msg.push_str(&describe_path(&cx.parents.borrow().path_to_bottom(p))); + msg.push_str(&describe_path(&parents.path_to_bottom(p))); } msg.push_str("\n\nfailed to select a version for `"); @@ -231,7 +231,7 @@ pub(super) fn activation_error( ); msg.push_str("required by "); msg.push_str(&describe_path( - &cx.parents.borrow().path_to_bottom(&parent.package_id()), + &parents.path_to_bottom(&parent.package_id()), )); // If we have a path dependency with a locked version, then this may @@ -308,7 +308,7 @@ pub(super) fn activation_error( } msg.push_str("required by "); msg.push_str(&describe_path( - &cx.parents.borrow().path_to_bottom(&parent.package_id()), + &parents.path_to_bottom(&parent.package_id()), )); msg diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index b01a7180e20..0c34c8943d6 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -450,15 +450,18 @@ impl Clone for StackGraph { } } +fn remove_from_other_refs(borrow: &mut RefMut<'_, BTreeMap>, age: GraphAge) { + if let std::collections::btree_map::Entry::Occupied(mut val) = borrow.entry(age) { + *val.get_mut() -= 1; + if val.get() == &0 { + val.remove(); + } + } +} + impl Drop for StackGraph { fn drop(&mut self) { - let mut borrow = RefCell::borrow_mut(&self.other_refs); - if let std::collections::btree_map::Entry::Occupied(mut val) = borrow.entry(self.age) { - *val.get_mut() -= 1; - if val.get() == &0 { - val.remove(); - } - } + remove_from_other_refs(&mut RefCell::borrow_mut(&self.other_refs), self.age); } } @@ -488,20 +491,14 @@ impl StackGraph { /// Gets mutable access to the inner `Graph`. Uses `other_refs` to determine if a deep clone is /// needed, and runs `reset_to` to get the `Graph` into the state it was in. /// - /// It is the responsibility of the caller to re-add this clone to `other_refs` after the new age is determined. + /// It is the responsibility of the caller to set `self.age` and call `add_to_other_refs` after the new age is determined. fn activate(&mut self) -> RefMut<'_, Graph> { let inner = if { let mut borrow = RefCell::borrow_mut(&self.other_refs); - if let std::collections::btree_map::Entry::Occupied(mut val) = borrow.entry(self.age) { - *val.get_mut() -= 1; - if val.get() == &0 { - val.remove(); - } - } + remove_from_other_refs(&mut borrow, self.age); borrow .keys() - .rev() - .next() + .next_back() .map(|a| a <= &self.age) .unwrap_or(true) } { @@ -523,15 +520,23 @@ impl StackGraph { inner } + fn add_to_other_refs(&mut self) { + *RefCell::borrow_mut(&self.other_refs) + .entry(self.age) + .or_insert(0) += 1; + } + + pub fn clone_into_graph(&self) -> Graph { + self.clone().activate().clone() + } + pub fn add(&mut self, node: N) { self.age = { let mut g = self.activate(); g.add(node); g.len() }; - *RefCell::borrow_mut(&self.other_refs) - .entry(self.age) - .or_insert(0) += 1; + self.add_to_other_refs(); } /// connect `node`to `child` with out associating any data. @@ -541,9 +546,7 @@ impl StackGraph { g.link(node, child); g.len() }; - *RefCell::borrow_mut(&self.other_refs) - .entry(self.age) - .or_insert(0) += 1; + self.add_to_other_refs(); } /// connect `node`to `child` associating it with `edge`. @@ -553,9 +556,7 @@ impl StackGraph { g.add_edge(node, child, edge); g.len() }; - *RefCell::borrow_mut(&self.other_refs) - .entry(self.age) - .or_insert(0) += 1; + self.add_to_other_refs(); } } @@ -717,28 +718,4 @@ impl<'a, N: Eq + Hash + Clone, E: Clone + PartialEq> StackGraphView<'a, N, E> { }) }) } - - /// Resolves one of the paths from the given dependent package down to - /// a leaf. - pub fn path_to_bottom<'s>(&'s self, mut pkg: &'s N) -> Vec<&'s N> { - let mut result = vec![pkg]; - while let Some(p) = self - .inner - .nodes - .get_full(pkg) - .filter(|(i, _, _)| *i < self.age.len_nodes) - .and_then(|(_, _, p)| { - p.iter() - .filter(|(_, idx)| **idx < self.age.len_edges) - // Note that we can have "cycles" introduced through dev-dependency - // edges, so make sure we don't loop infinitely. - .find(|&(node, _)| !result.contains(&node)) - .map(|(p, _)| p) - }) - { - result.push(p); - pkg = p; - } - result - } } From fa8c4ee5980cb07a6dfc16b53147d23b4a208031 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Mon, 26 Aug 2019 12:12:55 -0400 Subject: [PATCH 20/20] improve some documentation --- src/cargo/util/graph.rs | 53 +++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 0c34c8943d6..492191ee97a 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -41,18 +41,35 @@ //! Ok(finalize(dependency_graph_so_far)) //! } //! } +//! +//! fn activate(&mut dependency_graph: _, &mut unresolved_dependencies: _, candidate:_) -> Result<(), Problem> { +//! // check that we can use candidate given the other selected packages +//! if semver conflict { return Err(Problem::Semver); }; +//! if links conflict { return Err(Problem::Links); }; +//! ... +//! // update other state +//! if used a [replace] section { update that lookup table }; +//! if used a [patch] section { update that lookup table }; +//! update activated features +//! ... +//! // add candidate to dependency_graph +//! dependency_graph.add_edge(candidate.parent, candidate.id, candidate.dependency); +//! // add candidate's dependencies to the list to be resolved +//! unresolved_dependencies.extend(candidate.dependencies); +//! Ok(()) +//! } //! ``` //! -//! The real resolver is not recursive to avoid blowing the stack, and has lots of other state to -//! maintain. The most expensive (non recursive) call in this algorithm is the -//! `dependency_graph_so_far.clone();`. To make this more annoying the first thing we try will -//! probably work, and any work we do to prepare for the next iteration is wasted. If we had a -//! `undo_activate` we could be much more efficient, completely remove the `.clone()` and just -//! `undo_activate` if things tern out to not work. Unfortunately, making sure `undo_activate` -//! handles all the corner cases correctly is not practical for the resolver. However, this is -//! possible for a graph like thing, a `clone` means record the size of the graph. a `&mut self` -//! method means undo everything newer and do the mutation, a `&self` method means only look at the -//! older part. This module provides a `StackGraph` type to encapsulate this pattern. +//! The real resolver is not recursive to avoid blowing the stack. The most expensive +//! (non recursive) call in this algorithm is the `dependency_graph_so_far.clone();`. +//! To make this more annoying the first thing we try will probably work, and any work we do to +//! prepare for the next iteration is wasted. If we had a `undo_activate` we could be much more efficient, +//! completely remove the `.clone()` and just `undo_activate` if things tern out to not work. +//! Unfortunately, making sure `undo_activate` handles all the corner cases correctly is not +//! practical for the resolver. However, this is possible for a graph like thing, a `clone` means +//! record the size of the graph. a `&mut self` method means undo everything newer and do the +//! mutation, a `&self` method means only look at the older part. This module provides a +//! `StackGraph` type to encapsulate this pattern. use indexmap::IndexMap; use std::borrow::Borrow; @@ -64,6 +81,10 @@ use std::rc::Rc; type EdgeIndex = usize; +/// This stores one Edge or Link element. If the two Nodes have data to connect them then we say it +/// is an Edge and the `value` is `Some(E)`, if the two Nodes did not have data then we say it is a +/// Link and the `value` is `None`. If there are other `EdgeLink`s associated with the same two Nodes, +/// then we make a index based doubly linked list using the `next` and `previous` fields. #[derive(Clone, Debug)] struct EdgeLink { value: Option, @@ -79,11 +100,13 @@ struct EdgeLink { /// that efficiently undoes the most reason modifications. #[derive(Clone)] pub struct Graph { - /// an index based linked list of the edge data for links. This maintains insertion order. + /// An index based linked list of the edge data. This maintains insertion order. + /// If the connection has data associated with it then that Edge is stored here. + /// If the connection has no data associated with it then that Link is represented with a dummy here. edges: Vec>, - /// a hashmap that stores the set of nodes. This is an `IndexMap` so it maintains insertion order. - /// For each node it stores all the other nodes that it links to. - /// For each link it stores the first index into `edges`. + /// A map from a "from" node to (a map of "to" node to the starting index in the `edges`). + /// The inner map may be empty to represent a Node that was `add` but not connected to anything. + /// The maps are `IndexMap`s so they maintains insertion order. nodes: indexmap::IndexMap>, } @@ -402,7 +425,7 @@ impl Eq for Graph {} /// - the clone has no overhead to read the `Graph` as it was /// - no overhead over using the `Graph` directly when modifying the biggest clone /// Is this too good to be true? -/// - If a modification (`&mut` method) is done to a smaller older clone then a full `O(N)` +/// - If a modification (`&mut` method) is done to a clone that is not the largest then a full `O(N)` /// deep clone will happen internally. pub struct StackGraph { /// The `Graph` shared by all clones, this `StackGraph` refers to only the prefix of size `age`