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

Less duplication in activate #6967

Closed
wants to merge 4 commits into from

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented May 20, 2019

One of the "background context" things I re-explained in #6919 (comment) is why we have checks in RemainingCandidates::next and in flag_activated. "Why the duplication? Because cloning a BacktrackFrame is more expensive than doing the work twice." But thanks to a lot of work, and im.rs, the clone is now not so bad.

This PR attempts to go all in on removing this duplication. Anything to simplify the resolver is good! But, this removes or hamstrings several important optimizations that make NOP builds fast. Specifically #5132 will kick in far less often. Alex, can you look at the times after/before this PR on some larger projects? You have seen this be slow with perf record ./x.py build before, but servo or cargo may be easy and interesting.

Note that Master is at "Fast but duplicated" and this PR may be at "Slow but clean", the best solution may be somewhere in between.

@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 May 20, 2019
@Eh2406
Copy link
Contributor Author

Eh2406 commented May 31, 2019

This is waiting on someone with a fairly powerful and idle computer doing a comparison of time cargo generate-lockfile with a build of master vers a build with this PR on some large project like servo or like rustc. I don't have access to the compute and @alexcrichton appears not to have the time. But anyone is open to step in!

@ehuss
Copy link
Contributor

ehuss commented Jun 1, 2019

Did some quick measurements on my laptop (2.6GHz i7), average of 10 runs, with --offline:

Project master 6967
2000 crate stress test 2.127s 2.671s
rust-lang/rust 0.246s 0.270s
servo 0.620s 0.641s
cargo 0.106s 0.112s

@Eh2406
Copy link
Contributor Author

Eh2406 commented Jun 1, 2019

Thanks for doing the work! Adding a % column,

Project master 6967 %
2000 crate stress test 2.127s 2.671s 25%
rust-lang/rust 0.246s 0.270s 9%
servo 0.620s 0.641s 3%
cargo 0.106s 0.112s 5%

That is a pretty significant cost. :-(

@Eh2406
Copy link
Contributor Author

Eh2406 commented Jun 6, 2019

@ehuss Can you share the 2000 crate stress test I'd love to experiment with that. If I don't find anything in that to change things, this should probably be changed to just adding a comment explaining the connection between RemainingCandidates::next and in flag_activated.

@ehuss
Copy link
Contributor

ehuss commented Jun 7, 2019

Here you go: https://gist.github.com/ehuss/757952b83a239b37cb8811f90e45c6d6

@Eh2406 Eh2406 closed this Jun 11, 2019
@Eh2406 Eh2406 deleted the less-duplication-activate branch June 11, 2019 20:32
@Eh2406 Eh2406 mentioned this pull request Jul 23, 2019
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