Skip to content

Commit eeb757e

Browse files
committed
Auto merge of #7993 - Eh2406:deduplicate-eges, r=ehuss
De-duplicate edges This is a quick fix for #7985. It is possible to have more than one dependency that connects two packages, if one is a dev and one a regular. The code has use a `Vec` to represent that potential multiplicity. This switches it to a `HashSet` to fix #7985. But if there is only a handful of ways we can have more than one then perhaps we can do something with less indirection/allocations. Note that #7168 (which was already abandoned) will need to be redesigned for whatever we do for this.
2 parents 94093c2 + bd4ae6e commit eeb757e

File tree

4 files changed

+21
-28
lines changed

4 files changed

+21
-28
lines changed

src/cargo/core/resolver/context.rs

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
use std::collections::HashMap;
22
use std::num::NonZeroU64;
3-
use std::rc::Rc;
43

54
use anyhow::format_err;
65
use log::debug;
@@ -35,7 +34,7 @@ pub struct Context {
3534

3635
/// a way to look up for a package in activations what packages required it
3736
/// and all of the exact deps that it fulfilled.
38-
pub parents: Graph<PackageId, Rc<Vec<Dependency>>>,
37+
pub parents: Graph<PackageId, im_rc::HashSet<Dependency>>,
3938
}
4039

4140
/// When backtracking it can be useful to know how far back to go.
@@ -255,8 +254,8 @@ impl Context {
255254
.collect()
256255
}
257256

258-
pub fn graph(&self) -> Graph<PackageId, Vec<Dependency>> {
259-
let mut graph: Graph<PackageId, Vec<Dependency>> = Graph::new();
257+
pub fn graph(&self) -> Graph<PackageId, std::collections::HashSet<Dependency>> {
258+
let mut graph: Graph<PackageId, std::collections::HashSet<Dependency>> = Graph::new();
260259
self.activations
261260
.values()
262261
.for_each(|(r, _)| graph.add(r.package_id()));
@@ -265,14 +264,14 @@ impl Context {
265264
for (o, e) in self.parents.edges(i) {
266265
let old_link = graph.link(*o, *i);
267266
assert!(old_link.is_empty());
268-
*old_link = e.to_vec();
267+
*old_link = e.iter().cloned().collect();
269268
}
270269
}
271270
graph
272271
}
273272
}
274273

275-
impl Graph<PackageId, Rc<Vec<Dependency>>> {
274+
impl Graph<PackageId, im_rc::HashSet<Dependency>> {
276275
pub fn parents_of(&self, p: PackageId) -> impl Iterator<Item = (PackageId, bool)> + '_ {
277276
self.edges(&p)
278277
.map(|(grand, d)| (*grand, d.iter().any(|x| x.is_public())))
@@ -338,7 +337,7 @@ impl PublicDependency {
338337
parent_pid: PackageId,
339338
is_public: bool,
340339
age: ContextAge,
341-
parents: &Graph<PackageId, Rc<Vec<Dependency>>>,
340+
parents: &Graph<PackageId, im_rc::HashSet<Dependency>>,
342341
) {
343342
// one tricky part is that `candidate_pid` may already be active and
344343
// have public dependencies of its own. So we not only need to mark
@@ -383,7 +382,7 @@ impl PublicDependency {
383382
b_id: PackageId,
384383
parent: PackageId,
385384
is_public: bool,
386-
parents: &Graph<PackageId, Rc<Vec<Dependency>>>,
385+
parents: &Graph<PackageId, im_rc::HashSet<Dependency>>,
387386
) -> Result<
388387
(),
389388
(

src/cargo/core/resolver/mod.rs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -609,12 +609,11 @@ fn activate(
609609
cx.age += 1;
610610
if let Some((parent, dep)) = parent {
611611
let parent_pid = parent.package_id();
612-
Rc::make_mut(
613-
// add a edge from candidate to parent in the parents graph
614-
cx.parents.link(candidate_pid, parent_pid),
615-
)
616-
// and associate dep with that edge
617-
.push(dep.clone());
612+
// add a edge from candidate to parent in the parents graph
613+
cx.parents
614+
.link(candidate_pid, parent_pid)
615+
// and associate dep with that edge
616+
.insert(dep.clone());
618617
if let Some(public_dependency) = cx.public_dependency.as_mut() {
619618
public_dependency.add_edge(
620619
candidate_pid,

src/cargo/core/resolver/resolve.rs

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ pub struct Resolve {
1818
/// A graph, whose vertices are packages and edges are dependency specifications
1919
/// from `Cargo.toml`. We need a `Vec` here because the same package
2020
/// might be present in both `[dependencies]` and `[build-dependencies]`.
21-
graph: Graph<PackageId, Vec<Dependency>>,
21+
graph: Graph<PackageId, HashSet<Dependency>>,
2222
/// Replacements from the `[replace]` table.
2323
replacements: HashMap<PackageId, PackageId>,
2424
/// Inverted version of `replacements`.
@@ -70,7 +70,7 @@ pub enum ResolveVersion {
7070

7171
impl Resolve {
7272
pub fn new(
73-
graph: Graph<PackageId, Vec<Dependency>>,
73+
graph: Graph<PackageId, HashSet<Dependency>>,
7474
replacements: HashMap<PackageId, PackageId>,
7575
features: HashMap<PackageId, Vec<InternedString>>,
7676
checksums: HashMap<PackageId, Option<String>>,
@@ -264,18 +264,16 @@ unable to verify that `{0}` is the same as when the lockfile was generated
264264
self.graph.iter().cloned()
265265
}
266266

267-
pub fn deps(&self, pkg: PackageId) -> impl Iterator<Item = (PackageId, &[Dependency])> {
267+
pub fn deps(&self, pkg: PackageId) -> impl Iterator<Item = (PackageId, &HashSet<Dependency>)> {
268268
self.deps_not_replaced(pkg)
269269
.map(move |(id, deps)| (self.replacement(id).unwrap_or(id), deps))
270270
}
271271

272272
pub fn deps_not_replaced(
273273
&self,
274274
pkg: PackageId,
275-
) -> impl Iterator<Item = (PackageId, &[Dependency])> {
276-
self.graph
277-
.edges(&pkg)
278-
.map(|(id, deps)| (*id, deps.as_slice()))
275+
) -> impl Iterator<Item = (PackageId, &HashSet<Dependency>)> {
276+
self.graph.edges(&pkg).map(|(id, deps)| (*id, deps))
279277
}
280278

281279
pub fn replacement(&self, pkg: PackageId) -> Option<PackageId> {
@@ -325,8 +323,9 @@ unable to verify that `{0}` is the same as when the lockfile was generated
325323
to: PackageId,
326324
to_target: &Target,
327325
) -> CargoResult<String> {
326+
let empty_set: HashSet<Dependency> = HashSet::new();
328327
let deps = if from == to {
329-
&[]
328+
&empty_set
330329
} else {
331330
self.dependencies_listed(from, to)
332331
};
@@ -349,7 +348,7 @@ unable to verify that `{0}` is the same as when the lockfile was generated
349348
Ok(name)
350349
}
351350

352-
fn dependencies_listed(&self, from: PackageId, to: PackageId) -> &[Dependency] {
351+
fn dependencies_listed(&self, from: PackageId, to: PackageId) -> &HashSet<Dependency> {
353352
// We've got a dependency on `from` to `to`, but this dependency edge
354353
// may be affected by [replace]. If the `to` package is listed as the
355354
// target of a replacement (aka the key of a reverse replacement map)

src/cargo/ops/cargo_output_metadata.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -184,11 +184,7 @@ fn build_resolve_graph_r(
184184
CompileKind::Host => true,
185185
})
186186
.filter_map(|(dep_id, deps)| {
187-
let mut dep_kinds: Vec<_> = deps.iter().map(DepKindInfo::from).collect();
188-
// Duplicates may appear if the same package is used by different
189-
// members of a workspace with different features selected.
190-
dep_kinds.sort_unstable();
191-
dep_kinds.dedup();
187+
let dep_kinds: Vec<_> = deps.iter().map(DepKindInfo::from).collect();
192188
package_map
193189
.get(&dep_id)
194190
.and_then(|pkg| pkg.targets().iter().find(|t| t.is_lib()))

0 commit comments

Comments
 (0)