make RemainingCandidates::next peekable. #5044
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
`candidates.next` always came with a `candidates.clone().next(prev_active).is_ok` so lets just make that part of `next`. no `clone` needed.
Eh2406
force-pushed
the
PeekAtRemainingCandidates
branch
from
83b167e to
4afbd85
Compare
|
Thinking about it some more, an alternative is to just skip the In the happy path this is a small performance hit, a clone for every time we only have 1 candidate. But how often does that happen? |
|
I started work on that alternative at:
Actually every time we are forced by a previous selection. |
There was a problem hiding this comment.
Ok! I'm back from traveling now so had a chance to sit down and read this. Looks great to me, thanks @Eh2406! Just a few tiny nits.
src/cargo/core/resolver/mod.rs
Outdated
| } | ||
| } | ||
| return Ok(b); | ||
| if let Some(r) = ::std::mem::replace(&mut self.has_another, Some(b)) { |
There was a problem hiding this comment.
nit: could std::mem be imported above?
src/cargo/core/resolver/mod.rs
Outdated
| frame.remaining_candidates.clone().next(prev_active).is_ok(), | ||
| ) | ||
| }; | ||
| let next= frame.remaining_candidates.next(frame.context_backup.prev_active(&frame.dep)); |
There was a problem hiding this comment.
nit: spacing on the = here
|
☔ The latest upstream changes (presumably #4978) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@alexcrichton, I fixed you nits. Then I noted that some of my cleanups from the |
…idates # Conflicts: # src/cargo/core/resolver/mod.rs
|
merge done. |
|
@bors: r+ |
|
📌 Commit 0be926d has been approved by |
make RemainingCandidates::next peekable. `candidates.next` always came with a `candidates.clone().next(prev_active).is_ok` so let's just make that part of `next`. no `clone` needed.
|
☀️ Test successful - status-appveyor, status-travis |
|
Looking at some profiles from master Cargo, I think this commit unfortunately makes us lose out on a previously important optimization which is when |
|
That is not good. This PR was not intended to change that optimization. I will help track down that regression any way I can. I don't see how this commit could be causing a problem with that, the only So I can dig in (when I next have time), what is the exact command you are testing with? With that I can bisect to Identify the problem commit. I suspect it is from #5000 or #5104 as they affected clones of context. In any case, there should be ways to make it all work and have that optimization. Just need to figure out how. |
|
Er sorry, I think I misdiagnosed! I do indeed believe this is #5000 as it moved the creation of As for a test case I was running On master though it's this line -- cargo/src/cargo/core/resolver/mod.rs Line 897 in ae796ad which happens unconditionally before |
|
Ya, that makes sense, and will be trickier. I will look into it as soon as work allows. |
|
Ok awesome, thanks! |
|
I've opened a tracking issue for this at #5130 |
candidates.nextalways came with acandidates.clone().next(prev_active).is_okso let's just make that part ofnext. nocloneneeded.