change the priority in witch we build crates#7390
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/cargo/util/dependency_queue.rs
Outdated
| ) -> HashSet<N> { | ||
| let in_progress: HashSet<N> = HashSet::new(); | ||
| if let Some(depth) = results.get(key) { | ||
| assert_ne!(depth, &in_progress, "cycle in DependencyQueue"); |
There was a problem hiding this comment.
Instead of assert_ne could this change to just asserting that the set is nonempty?
src/cargo/util/dependency_queue.rs
Outdated
| .max() | ||
| .unwrap_or(0); | ||
| { | ||
| set.extend(depth(dep, map, results).into_iter()) |
There was a problem hiding this comment.
I think the .into_iter here can be elided
src/cargo/util/dependency_queue.rs
Outdated
| } | ||
|
|
||
| *results.get_mut(key).unwrap() = depth; | ||
| *results.get_mut(key).unwrap() = set.clone(); |
There was a problem hiding this comment.
Could this perhaps avoid .clone() by returning &HashSet<N> inside the HashMap itself?
There was a problem hiding this comment.
as in, depth returns a &HashSet<N>
There was a problem hiding this comment.
WIth witch lifetime? I tried 'results, but it did not go well. Is there something else worth trying? Is it worth Rc<HashSet<N>>?
There was a problem hiding this comment.
I was thinking of a diff like:
diff --git a/src/cargo/util/dependency_queue.rs b/src/cargo/util/dependency_queue.rs
index 21886c0ee..946549507 100644
--- a/src/cargo/util/dependency_queue.rs
+++ b/src/cargo/util/dependency_queue.rs
@@ -88,14 +88,15 @@ impl<N: Hash + Eq + Clone, E: Eq + Hash + Clone, V> DependencyQueue<N, E, V> {
}
self.priority = out.into_iter().map(|(n, set)| (n, set.len())).collect();
- fn depth<N: Hash + Eq + Clone, E: Hash + Eq + Clone>(
+ fn depth<'a, N: Hash + Eq + Clone, E: Hash + Eq + Clone>(
key: &N,
map: &HashMap<N, HashMap<E, HashSet<N>>>,
- results: &mut HashMap<N, HashSet<N>>,
- ) -> HashSet<N> {
- if let Some(depth) = results.get(key) {
+ results: &'a mut HashMap<N, HashSet<N>>,
+ ) -> &'a HashSet<N> {
+ if results.contains_key(key) {
+ let depth = &results[key];
assert!(!depth.is_empty(), "cycle in DependencyQueue");
- return depth.clone();
+ return depth;
}
results.insert(key.clone(), HashSet::new());
@@ -108,12 +109,12 @@ impl<N: Hash + Eq + Clone, E: Eq + Hash + Clone, V> DependencyQueue<N, E, V> {
.flat_map(|it| it.values())
.flat_map(|set| set)
{
- set.extend(depth(dep, map, results))
+ set.extend(depth(dep, map, results).iter().cloned())
}
- *results.get_mut(key).unwrap() = set.clone();
-
- set
+ let slot = results.get_mut(key).unwrap();
+ *slot = set;
+ return &*slot;
}
}if that feels to complicated though it's definitely too minor to use Rc, so we can just go w/ what's here as-is.
There was a problem hiding this comment.
You are a wizard! Hat's off!
|
@bors: r+ |
|
📌 Commit 384056e has been approved by |
bors
added
S-waiting-on-bors
change the priority in witch we build crates On my windows 4 core, this made clean builds (of 3596cb8) go from 257-223s to 210-205s. So that looks like an improvement.
|
☀️ Test successful - checks-azure |
6 participants
On my windows 4 core, this made clean builds (of 3596cb8) go from 257-223s to 210-205s. So that looks like an improvement.