Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change the priority in witch we build crates #7390

Merged
merged 3 commits into from
Sep 19, 2019
Merged

change the priority in witch we build crates #7390

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35853c1
change the priority in witch we build crates
Eh2406 Sep 19, 2019
0bce400
minimize diff
Eh2406 Sep 19, 2019
384056e
less clones
alexcrichton Sep 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 27 additions & 22 deletions src/cargo/util/dependency_queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ pub struct DependencyQueue<N: Hash + Eq, E: Hash + Eq, V> {
/// easily indexable with just an `N`
reverse_dep_map: HashMap<N, HashMap<E, HashSet<N>>>,

/// Topological depth of each key
depth: HashMap<N, usize>,
/// The total number of packages that are transitively waiting on this package
priority: HashMap<N, usize>,
}

impl<N: Hash + Eq, E: Hash + Eq, V> Default for DependencyQueue<N, E, V> {
Expand All @@ -47,12 +47,14 @@ impl<N: Hash + Eq, E: Hash + Eq, V> DependencyQueue<N, E, V> {
DependencyQueue {
dep_map: HashMap::new(),
reverse_dep_map: HashMap::new(),
depth: HashMap::new(),
priority: HashMap::new(),
}
}
}

impl<N: Hash + Eq + Clone, E: Eq + Hash + Clone, V> DependencyQueue<N, E, V> {
impl<N: Hash + Eq + Clone + std::fmt::Debug + PartialEq, E: Eq + Hash + Clone, V>
DependencyQueue<N, E, V>
{
/// Adds a new ndoe and its dependencies to this queue.
///
/// The `key` specified is a new node in the dependency graph, and the node
Expand Down Expand Up @@ -82,36 +84,39 @@ impl<N: Hash + Eq + Clone, E: Eq + Hash + Clone, V> DependencyQueue<N, E, V> {
/// All nodes have been added, calculate some internal metadata and prepare
/// for `dequeue`.
pub fn queue_finished(&mut self) {
let mut out = HashMap::new();
for key in self.dep_map.keys() {
depth(key, &self.reverse_dep_map, &mut self.depth);
depth(key, &self.reverse_dep_map, &mut out);
}
self.priority = out.into_iter().map(|(n, set)| (n, set.len())).collect();

fn depth<N: Hash + Eq + Clone, E: Hash + Eq + Clone>(
fn depth<N: Hash + Eq + Clone + std::fmt::Debug + PartialEq, E: Hash + Eq + Clone>(
key: &N,
map: &HashMap<N, HashMap<E, HashSet<N>>>,
results: &mut HashMap<N, usize>,
) -> usize {
const IN_PROGRESS: usize = !0;

if let Some(&depth) = results.get(key) {
assert_ne!(depth, IN_PROGRESS, "cycle in DependencyQueue");
return depth;
results: &mut HashMap<N, HashSet<N>>,
) -> HashSet<N> {
let in_progress: HashSet<N> = HashSet::new();
if let Some(depth) = results.get(key) {
assert_ne!(depth, &in_progress, "cycle in DependencyQueue");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of assert_ne could this change to just asserting that the set is nonempty?

return depth.clone();
}
results.insert(key.clone(), in_progress);

results.insert(key.clone(), IN_PROGRESS);
let mut set = HashSet::new();
set.insert(key.clone());

let depth = 1 + map
for dep in map
.get(key)
.into_iter()
.flat_map(|it| it.values())
.flat_map(|set| set)
.map(|dep| depth(dep, map, results))
.max()
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
.unwrap_or(0);
{
set.extend(depth(dep, map, results).into_iter())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the .into_iter here can be elided

}

*results.get_mut(key).unwrap() = depth;
*results.get_mut(key).unwrap() = set.clone();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this perhaps avoid .clone() by returning &HashSet<N> inside the HashMap itself?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in, depth returns a &HashSet<N>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIth witch lifetime? I tried 'results, but it did not go well. Is there something else worth trying? Is it worth Rc<HashSet<N>>?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are a wizard! Hat's off!


depth
set
}
}

Expand All @@ -122,7 +127,7 @@ impl<N: Hash + Eq + Clone, E: Eq + Hash + Clone, V> DependencyQueue<N, E, V> {
pub fn dequeue(&mut self) -> Option<(N, V)> {
// Look at all our crates and find everything that's ready to build (no
// deps). After we've got that candidate set select the one which has
// the maximum depth in the dependency graph. This way we should
// the maximum priority in the dependency graph. This way we should
// hopefully keep CPUs hottest the longest by ensuring that long
// dependency chains are scheduled early on in the build process and the
// leafs higher in the tree can fill in the cracks later.
Expand All @@ -135,7 +140,7 @@ impl<N: Hash + Eq + Clone, E: Eq + Hash + Clone, V> DependencyQueue<N, E, V> {
.iter()
.filter(|(_, (deps, _))| deps.is_empty())
.map(|(key, _)| key.clone())
.max_by_key(|k| self.depth[k]);
.max_by_key(|k| self.priority[k]);
let key = match next {
Some(key) => key,
None => return None,
Expand Down