Skip to content

Commit 997b3dc

Browse files
Eh2406Eh2406
committed
Move caching to a support struct and file
1 parent aac04b6 commit 997b3dc

File tree

2 files changed

+121
-88
lines changed

2 files changed

+121
-88
lines changed

‎src/cargo/core/resolver/conflict_cache.rs

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
use std::collections::{HashMap, HashSet};
2+
3+
use core::{Dependency, PackageId};
4+
use core::resolver::{ConflictReason, Context};
5+
6+
pub(super) struct ConflictCache {
7+
// `con_from_dep` is a cache of the reasons for each time we
8+
// backtrack. For example after several backtracks we may have:
9+
//
10+
// con_from_dep[`foo = "^1.0.2"`] = vec![
11+
// map!{`foo=1.0.1`: Semver},
12+
// map!{`foo=1.0.0`: Semver},
13+
// ];
14+
//
15+
// This can be read as "we cannot find a candidate for dep `foo = "^1.0.2"`
16+
// if either `foo=1.0.1` OR `foo=1.0.0` are activated".
17+
//
18+
// Another example after several backtracks we may have:
19+
//
20+
// con_from_dep[`foo = ">=0.8.2, <=0.9.3"`] = vec![
21+
// map!{`foo=0.8.1`: Semver, `foo=0.9.4`: Semver},
22+
// ];
23+
//
24+
// This can be read as "we cannot find a candidate for dep `foo = ">=0.8.2,
25+
// <=0.9.3"` if both `foo=0.8.1` AND `foo=0.9.4` are activated".
26+
//
27+
// This is used to make sure we don't queue work we know will fail. See the
28+
// discussion in https://github.com/rust-lang/cargo/pull/5168 for why this
29+
// is so important, and there can probably be a better data structure here
30+
// but for now this works well enough!
31+
//
32+
// Also, as a final note, this map is *not* ever removed from. This remains
33+
// as a global cache which we never delete from. Any entry in this map is
34+
// unconditionally true regardless of our resolution history of how we got
35+
// here.
36+
con_from_dep: HashMap<Dependency, Vec<HashMap<PackageId, ConflictReason>>>,
37+
// `past_conflict_triggers` is an
38+
// of `past_conflicting_activations`.
39+
// For every `PackageId` this lists the `Dependency`s that mention it in `past_conflicting_activations`.
40+
dep_from_pid: HashMap<PackageId, HashSet<Dependency>>,
41+
}
42+
43+
impl ConflictCache {
44+
pub(super) fn new() -> ConflictCache {
45+
ConflictCache {
46+
con_from_dep: HashMap::new(),
47+
dep_from_pid: HashMap::new(),
48+
}
49+
}
50+
pub(super) fn filter_conflicting<F>(
51+
&self,
52+
cx: &Context,
53+
dep: &Dependency,
54+
filter: F,
55+
) -> Option<&HashMap<PackageId, ConflictReason>>
56+
where
57+
for<'r> F: FnMut(&'r &HashMap<PackageId, ConflictReason>) -> bool,
58+
{
59+
self.con_from_dep.get(dep).and_then(|past_bad| {
60+
past_bad
61+
.iter()
62+
.filter(filter)
63+
.find(|conflicting| cx.is_conflicting(None, conflicting))
64+
})
65+
}
66+
pub(super) fn conflicting(
67+
&self,
68+
cx: &Context,
69+
dep: &Dependency,
70+
) -> Option<&HashMap<PackageId, ConflictReason>> {
71+
self.filter_conflicting(cx, dep, |_| true)
72+
}
73+
pub(super) fn insert(&mut self, dep: &Dependency, con: &HashMap<PackageId, ConflictReason>) {
74+
let past = self.con_from_dep
75+
.entry(dep.clone())
76+
.or_insert_with(Vec::new);
77+
if !past.contains(con) {
78+
trace!("{} adding a skip {:?}", dep.name(), con);
79+
past.push(con.clone());
80+
for c in con.keys() {
81+
self.dep_from_pid
82+
.entry(c.clone())
83+
.or_insert_with(HashSet::new)
84+
.insert(dep.clone());
85+
}
86+
}
87+
}
88+
pub(super) fn get_dep_from_pid(&self, pid: &PackageId) -> Option<&HashSet<Dependency>> {
89+
self.dep_from_pid.get(pid)
90+
}
91+
}

‎src/cargo/core/resolver/mod.rs

Lines changed: 30 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve
7272
pub use self::encode::{Metadata, WorkspaceResolve};
7373

7474
mod encode;
75+
mod conflict_cache;
7576

7677
/// Represents a fully resolved package dependency graph. Each node in the graph
7778
/// is a package and edges represent dependencies between packages.
@@ -594,6 +595,15 @@ impl DepsFrame {
594595
.map(|(_, (_, candidates, _))| candidates.len())
595596
.unwrap_or(0)
596597
}
598+
599+
fn flatten<'s>(&'s self) -> Box<Iterator<Item = (PackageId, Dependency)> + 's> {
600+
// TODO: with impl Trait the Box can be removed
601+
Box::new(
602+
self.remaining_siblings
603+
.clone()
604+
.map(move |(_, (d, _, _))| (self.parent.package_id().clone(), d)),
605+
)
606+
}
597607
}
598608

599609
impl PartialEq for DepsFrame {
@@ -975,42 +985,8 @@ fn activate_deps_loop(
975985
let mut remaining_deps = BinaryHeap::new();
976986

977987
// `past_conflicting_activations` is a cache of the reasons for each time we
978-
// backtrack. For example after several backtracks we may have:
979-
//
980-
// past_conflicting_activations[`foo = "^1.0.2"`] = vec![
981-
// map!{`foo=1.0.1`: Semver},
982-
// map!{`foo=1.0.0`: Semver},
983-
// ];
984-
//
985-
// This can be read as "we cannot find a candidate for dep `foo = "^1.0.2"`
986-
// if either `foo=1.0.1` OR `foo=1.0.0` are activated".
987-
//
988-
// Another example after several backtracks we may have:
989-
//
990-
// past_conflicting_activations[`foo = ">=0.8.2, <=0.9.3"`] = vec![
991-
// map!{`foo=0.8.1`: Semver, `foo=0.9.4`: Semver},
992-
// ];
993-
//
994-
// This can be read as "we cannot find a candidate for dep `foo = ">=0.8.2,
995-
// <=0.9.3"` if both `foo=0.8.1` AND `foo=0.9.4` are activated".
996-
//
997-
// This is used to make sure we don't queue work we know will fail. See the
998-
// discussion in https://github.com/rust-lang/cargo/pull/5168 for why this
999-
// is so important, and there can probably be a better data structure here
1000-
// but for now this works well enough!
1001-
//
1002-
// Also, as a final note, this map is *not* ever removed from. This remains
1003-
// as a global cache which we never delete from. Any entry in this map is
1004-
// unconditionally true regardless of our resolution history of how we got
1005-
// here.
1006-
let mut past_conflicting_activations: HashMap<
1007-
Dependency,
1008-
Vec<HashMap<PackageId, ConflictReason>>,
1009-
> = HashMap::new();
1010-
1011-
// `past_conflict_triggers` is an inverse-index of `past_conflicting_activations`.
1012-
// For every `PackageId` this lists the `Dependency`s that mention it in `past_conflicting_activations`.
1013-
let mut past_conflict_triggers: HashMap<PackageId, HashSet<Dependency>> = HashMap::new();
988+
// backtrack.
989+
let mut past_conflicting_activations = conflict_cache::ConflictCache::new();
1014990

1015991
// Activate all the initial summaries to kick off some work.
1016992
for &(ref summary, ref method) in summaries {
@@ -1100,12 +1076,7 @@ fn activate_deps_loop(
11001076

11011077
let just_here_for_the_error_messages = just_here_for_the_error_messages
11021078
&& past_conflicting_activations
1103-
.get(&dep)
1104-
.and_then(|past_bad| {
1105-
past_bad
1106-
.iter()
1107-
.find(|conflicting| cx.is_conflicting(None, conflicting))
1108-
})
1079+
.conflicting(&cx, &dep)
11091080
.is_some();
11101081

11111082
let mut remaining_candidates = RemainingCandidates::new(&candidates);
@@ -1157,25 +1128,7 @@ fn activate_deps_loop(
11571128
// local is set to `true` then our `conflicting_activations` may
11581129
// not be right, so we can't push into our global cache.
11591130
if !just_here_for_the_error_messages && !backtracked {
1160-
let past = past_conflicting_activations
1161-
.entry(dep.clone())
1162-
.or_insert_with(Vec::new);
1163-
if !past.contains(&conflicting_activations) {
1164-
trace!(
1165-
"{}[{}]>{} adding a skip {:?}",
1166-
parent.name(),
1167-
cur,
1168-
dep.name(),
1169-
conflicting_activations
1170-
);
1171-
past.push(conflicting_activations.clone());
1172-
for c in conflicting_activations.keys() {
1173-
past_conflict_triggers
1174-
.entry(c.clone())
1175-
.or_insert_with(HashSet::new)
1176-
.insert(dep.clone());
1177-
}
1178-
}
1131+
past_conflicting_activations.insert(&dep, &conflicting_activations);
11791132
}
11801133

11811134
match find_candidate(&mut backtrack_stack, &parent, &conflicting_activations) {
@@ -1280,11 +1233,10 @@ fn activate_deps_loop(
12801233
if let Some(conflicting) = frame
12811234
.remaining_siblings
12821235
.clone()
1283-
.filter_map(|(_, (new_dep, _, _))| {
1284-
past_conflicting_activations.get(&new_dep)
1236+
.filter_map(|(_, (ref new_dep, _, _))| {
1237+
past_conflicting_activations.conflicting(&cx, &new_dep)
12851238
})
1286-
.flat_map(|x| x)
1287-
.find(|con| cx.is_conflicting(None, con))
1239+
.next()
12881240
{
12891241
let mut conflicting = conflicting.clone();
12901242

@@ -1309,33 +1261,23 @@ fn activate_deps_loop(
13091261
// ourselves are incompatible with that dep, so we know that deps
13101262
// parent conflict with us.
13111263
if !has_past_conflicting_dep {
1312-
if let Some(rel_deps) = past_conflict_triggers.get(&pid) {
1264+
if let Some(rel_deps) = past_conflicting_activations.get_dep_from_pid(&pid)
1265+
{
13131266
if let Some((other_parent, conflict)) = remaining_deps
13141267
.iter()
1315-
.flat_map(|other| {
1316-
other
1317-
.remaining_siblings
1318-
// search thru all `remaining_deps`
1319-
.clone()
1320-
// for deps related to us
1321-
.filter(|&(_, (ref d, _, _))| rel_deps.contains(d))
1322-
.map(move |(_, (d, _, _))| {
1323-
(other.parent.package_id().clone(), d)
1324-
})
1325-
})
1326-
.filter_map(|(other_parent, other_deps)| {
1268+
.flat_map(|other| other.flatten())
1269+
// for deps related to us
1270+
.filter(|&(_, ref other_dep)| rel_deps.contains(other_dep))
1271+
.filter_map(|(other_parent, other_dep)| {
13271272
past_conflicting_activations
1328-
.get(&other_deps)
1329-
.map(|v| (other_parent, v))
1330-
})
1331-
.flat_map(|(other_parent, past_bad)| {
1332-
past_bad
1333-
.iter()
1334-
// for deps that refer to us
1335-
.filter(|con| con.contains_key(&pid))
1336-
.map(move |c| (other_parent.clone(), c))
1273+
.filter_conflicting(
1274+
&cx,
1275+
&other_dep,
1276+
|con| con.contains_key(&pid)
1277+
)
1278+
.map(|con| (other_parent, con))
13371279
})
1338-
.find(|&(_, ref con)| cx.is_conflicting(None, con))
1280+
.next()
13391281
{
13401282
let mut conflict = conflict.clone();
13411283
let rel = conflict.get(&pid).unwrap().clone();

0 commit comments

Comments
 (0)