Resolver: A dep is equivalent to one of the things it can resolve to. #6776
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
Thanks for this @Eh2406!
At this point I think I'm not gonna try to pretend I fully understand what's going on here. This all looks reasonable at a surface level however and I'm pretty confident in the tests at this point (and you!). To that end I haven't fully internalized and reviewed generalize_conflicting yet, but that could be said about my understanding for most of the resolver :)
I've got a few nits below, but otherwise looks great to me to land!
| /// Versions `a` and `b` are compatible if their left-most nonzero digit is the | ||
| /// same. | ||
| #[derive(Clone, Copy, Eq, PartialEq, Hash)] | ||
| pub enum SemverCompatibility { |
There was a problem hiding this comment.
I think this is a pretty clever way to store Activations, nice find!
|
We are not seeing reports of performance problems from Cargos users. So I don't think it is worth committing changes that make the code harder to understand. Now, while it is fresh in my head, is the best time to add comments/docs. What could be made more clear? |
Eh2406
force-pushed
the
resolver-simplification
branch
from
8fddcb2
to
a3c3b18
Compare
|
Oh well so to be clear this is just a super tricky chunk of Cargo inherently, so I don't think this is really the fault of this PR or anything. What would be nice, however, would be a textual long-form description of what's going on somewhere, perhaps maintained in one location. The code could then liberally be commented with respect to that master description, and it'd be much easier to review the word changes (to make sure they make sense) and then cross-reference the code to make sure changes in the code match changes in the words. In that sense there's not really a function or two I think that would benefit from more docs, but more like a "if everything had 10% more documentation it'd probably make it easier". In that sense if you can think of any comments to add that'd be great! |
|
I definitely agree that we need to make progress to getting this (and the rest of cargos internals) better documented. Someday I'd love to have a coherent overall view of the resolver (maybe something as great as pubgrub's). Several people have asked me for a "formal specification". In the meantime we can try to find more places to insert explanatory material and try to make sure we are using consistent terminology. We should try to add more type aliases. For example the inconsistently used terminology of "age" (added in this PR) should be a reference to the new type. That gives us a place to add a comment on how and why it is used. (I'll add a commit when I have a spare cycles.) I think let's try to only merge PR's that leave the resolver better documented. Incremental progress can add up. |
Eh2406
force-pushed
the
resolver-simplification
branch
2 times, most recently
from
c63d2ca
to
122dc60
Compare
|
It is building for me locally.... |
This makes a O(n^2) loop in the hart of the resolver a O(n) loop, but n is small and hashing is not free. So the main reason to do this is to make the code clearer.
Yes, apparently we do. So I can't do optimizations based on that being unique.
Thus, if all the things it can resolve to have already ben determined to be conflicting, then we can just say that we conflict with the parent.
The best generalization of this is to let things bubble up and let `jumpback_critical_id` figure this out.
Eh2406
force-pushed
the
resolver-simplification
branch
from
122dc60
to
e4bc36e
Compare
|
Rebased on master. This time I even remembered to fetch first! |
Eh2406
force-pushed
the
resolver-simplification
branch
from
e4bc36e
to
1dd8e56
Compare
…p`, or `jumpback`
|
I think I've improve comments in several miner ways. Doing an updated overall design document is going to have to happen after pub/priv is working with whatever optimizations it takes to make that work. |
|
I think this is good to go, and yeah holding off a design doc and such sounds good to me! @bors: r+ |
|
📌 Commit 91b5a9d has been approved by |
bors
added
S-waiting-on-bors
Resolver: A dep is equivalent to one of the things it can resolve to. This is a series of small changes to the resolver, each one on its own is not worth the cherne, but somehow all together we can add a new optimization rule. The result is that the test in #6283 is no longer exponencial (still a large polynomial, cubick?) and so N can be bumped from 3 to 20. This also means that we pass with all the cases reported in #6258. Resolution is NP-Hard, so we are moving the slow cases around. To reduce the chance that we will be flooded by new bad cases I run the 4 proptests overnight, and they did not find a new exponencial case. I would recommend reviewing this commit by commit. As each change is pretty simple on its own, but the mixed diff is harder to follow. This is submitted as one big PR as that is @alexcrichton's preference. A special thanks to @nex3, our conversation was important in convincing me that several of these changes would be needed even in an eventual PubGrub based system. And, the question "why would PubGrub not have a problem with #6283" was wat crystallized this optimization opportunity in my mind.
|
☀️ Test successful - checks-travis, status-appveyor |
Update cargo 20 commits in 63231f438a2b5b84ccf319a5de22343ee0316323..6f3e9c367abb497c64f360c3839dab5e74928d5c 2019-03-27 12:26:45 +0000 to 2019-04-04 14:11:33 +0000 - Fix Init for Fossil SCM project (rust-lang/cargo#6792) - Fix member_manifest_version_error accessing the network (rust-lang/cargo#6799) - Don't include email if it is empty (rust-lang/cargo#6802) - Fix unused import warning (rust-lang/cargo#6807) - Add some help and documentation for unstable flags (rust-lang/cargo#6791) - Allow `cargo doc --open` with multiple packages (rust-lang/cargo#6803) - Allow `cargo install --path P` to load config from P (rust-lang/cargo#6804) - Add more suggestions on how to deal with excluding a package from a workspace (rust-lang/cargo#6805) - Warn on version req with metadata (rust-lang/cargo#6806) - cargo install: Be more restrictive about cli flags (rust-lang/cargo#6801) - Support force-pushed repos with git-fetch-with-cli (rust-lang/cargo#6800) - Cargo clippy (rust-lang/cargo#6759) - Don't include metadata in wasm binary examples (rust-lang/cargo#6812) - Update glossary for `feature` (rust-lang/cargo#6809) - Include proc-macros in `build-override` (rust-lang/cargo#6811) - Resolver: A dep is equivalent to one of the things it can resolve to (rust-lang/cargo#6776) - Add some docs for `Downloads` (rust-lang/cargo#6815) - Resolve: Be less strict while offline (rust-lang/cargo#6814) - Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818) - Fix doc link (rust-lang/cargo#6820) <br> I specifically care about "Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)" to unblock rust-lang#59076. Mentioning @ehuss.
Update cargo 20 commits in 63231f438a2b5b84ccf319a5de22343ee0316323..6f3e9c367abb497c64f360c3839dab5e74928d5c 2019-03-27 12:26:45 +0000 to 2019-04-04 14:11:33 +0000 - Fix Init for Fossil SCM project (rust-lang/cargo#6792) - Fix member_manifest_version_error accessing the network (rust-lang/cargo#6799) - Don't include email if it is empty (rust-lang/cargo#6802) - Fix unused import warning (rust-lang/cargo#6807) - Add some help and documentation for unstable flags (rust-lang/cargo#6791) - Allow `cargo doc --open` with multiple packages (rust-lang/cargo#6803) - Allow `cargo install --path P` to load config from P (rust-lang/cargo#6804) - Add more suggestions on how to deal with excluding a package from a workspace (rust-lang/cargo#6805) - Warn on version req with metadata (rust-lang/cargo#6806) - cargo install: Be more restrictive about cli flags (rust-lang/cargo#6801) - Support force-pushed repos with git-fetch-with-cli (rust-lang/cargo#6800) - Cargo clippy (rust-lang/cargo#6759) - Don't include metadata in wasm binary examples (rust-lang/cargo#6812) - Update glossary for `feature` (rust-lang/cargo#6809) - Include proc-macros in `build-override` (rust-lang/cargo#6811) - Resolver: A dep is equivalent to one of the things it can resolve to (rust-lang/cargo#6776) - Add some docs for `Downloads` (rust-lang/cargo#6815) - Resolve: Be less strict while offline (rust-lang/cargo#6814) - Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818) - Fix doc link (rust-lang/cargo#6820) <br> I specifically care about "Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)" to unblock #59076. Mentioning @ehuss.
Small things This has two small changes that are worth saving from my most recent attempt to speedup #6258 (comment): 1. This removes the `ConflictCache::contains` added in #6776. Replacing it with a more general and easier to explain system based on the existing `ConflictCache::find_conflicting`. 2. This adds code to print the used part of the input for failing resolver tests. This is very helpful when 1. The proptest shrinking algorithm is interrupted, at least you have the smallest one found so far. 2. Hand minimizing, remove a dep and it will tell you all the packages that are no longer needed for the test to fail.
This is a series of small changes to the resolver, each one on its own is not worth the cherne, but somehow all together we can add a new optimization rule. The result is that the test in #6283 is no longer exponencial (still a large polynomial, cubick?) and so N can be bumped from 3 to 20. This also means that we pass with all the cases reported in #6258. Resolution is NP-Hard, so we are moving the slow cases around. To reduce the chance that we will be flooded by new bad cases I run the 4 proptests overnight, and they did not find a new exponencial case.
I would recommend reviewing this commit by commit. As each change is pretty simple on its own, but the mixed diff is harder to follow. This is submitted as one big PR as that is @alexcrichton's preference.
A special thanks to @nex3, our conversation was important in convincing me that several of these changes would be needed even in an eventual PubGrub based system. And, the question "why would PubGrub not have a problem with #6283" was wat crystallized this optimization opportunity in my mind.