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

Replace std::sync::mpsc with a much simpler queue #7845

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Member

We don't need the complexity of most channels since this is not a
performance sensitive part of Cargo, nor is it likely to be so any time
soon. Coupled with recent bugs (#7840) we believe in std::sync::mpsc,
let's just not use that and use a custom queue type locally which should
be amenable to a blocking push soon too.

We don't need the complexity of most channels since this is not a
performance sensitive part of Cargo, nor is it likely to be so any time
soon. Coupled with recent bugs (rust-lang#7840) we believe in `std::sync::mpsc`,
let's just not use that and use a custom queue type locally which should
be amenable to a blocking push soon too.
@rust-highfive
Copy link

r? @Eh2406

(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 Jan 29, 2020
@alexcrichton
Copy link
Member Author

r? @ehuss

Also @ehuss do you have the project you used to benchmark over here? I'm also surprised by the results you found there and would suspect that this would have similar issues, but I'd love to help dig in as well.

@rust-highfive rust-highfive assigned ehuss and unassigned Eh2406 Jan 29, 2020
@ehuss
Copy link
Contributor

ehuss commented Jan 29, 2020

To benchmark, I just want a large number of Units to exercise the "Fresh" code path. I do this by creating a workspace with a large number of members.

mkdir big
cd big
for n in {1..500}; do cargo new --lib dep$n; done
cat >> Cargo.toml <<EOL
[workspace]
members = ["dep*"]
EOL

Run cargo build --all once to make everything fresh, then run time cargo build --all to see how long a fresh build takes. I also use hyperfine to average out multiple runs and see the variance. I also run against other projects like Cargo itself for more realistic samples.

The performance of this PR looks OK. However, applying the following patch causes it to hit the same jobserver issue described in #7844 (comment):

diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs
index 1cd5a54fd..58ce5fc0e 100644
--- a/src/cargo/core/compiler/job_queue.rs
+++ b/src/cargo/core/compiler/job_queue.rs
@@ -819,7 +819,7 @@ impl<'a, 'cfg> DrainState<'a, 'cfg> {
         match fresh {
             Freshness::Fresh => {
                 self.timings.add_fresh();
-                doit();
+                scope.spawn(move |_| doit());
             }
             Freshness::Dirty => {
                 self.timings.add_dirty();

I'll try to look at this closer soon. I'm intrigued and tempted by the simplicity of this PR, so I hope we can make it work out, but I'd like to coordinate with the changes in #7838.

@alexcrichton
Copy link
Member Author

Closing due to plan of action at #7844 (comment), we can reconsider if necessary in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants