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.

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

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Sep 19, 2019

On my windows 4 core, this made clean builds (of 3596cb8) go from 257-223s to 210-205s. So that looks like an improvement.

@rust-highfive
Copy link

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2019
) -> 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?

.max()
.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!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 19, 2019

📌 Commit 384056e has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2019
@bors
Copy link
Contributor

bors commented Sep 19, 2019

⌛ Testing commit 384056e with merge b6c6f68...

bors added a commit that referenced this pull request Sep 19, 2019
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.
@bors
Copy link
Contributor

bors commented Sep 19, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing b6c6f68 to master...

@bors bors merged commit 384056e into rust-lang:master Sep 19, 2019
@Eh2406 Eh2406 deleted the better-graph branch September 19, 2019 21:35
@ehuss ehuss added this to the 1.39.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants