Use a ad hoc trie to avoid slow cases #6283
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
I forgot that I used NLL to test this. I am not seeing how to get it to work without NLL. Thoughts? |
|
Figured out how to remove the reliance on NLL! |
Eh2406
added
A-dependency-resolution
|
Instead of responding in thread, I pushed a commit that added comments to address each of your excellent questions. On a more general note this is not a heavily optimized and carefully chosen algorithm, it is much closer to the first thing I could think of. I spent some time looking through the data structures I had heard of to see if there is an existing literature, but did not find any good search terms. You menchan "set of sets", do you have links about data structures for that? If anyone has references I am happy to look into them! |
| // will be conflicting. | ||
| if cx.is_active(pid) { | ||
| if let Some(o) = store.find_conflicting(cx, filter) { | ||
| debug_assert!(cx.is_conflicting(None, o)); |
There was a problem hiding this comment.
Would this debug assert bring back the N^2 behavior? In any case it's probably fine to move this debug assert above to if filter(&c), right?
There was a problem hiding this comment.
oops, yes moved it. It should not be too expensive when it is in the correct place.
|
|
||
| use super::types::ConflictReason; | ||
| use core::resolver::Context; | ||
| use core::{Dependency, PackageId}; | ||
|
|
||
| /// This is a data structure for storing a large number of Sets designed to | ||
| /// efficiently see if any of the stored Sets are a subset of a search Set. | ||
| enum ConflictStore { |
There was a problem hiding this comment.
Thinking more about this, I actually think this is basically a trie
|
I again followed up with a commit. |
Eh2406
changed the title
|
@bors: r+ 💯 |
|
📌 Commit 178ee0b has been approved by |
Use a ad hoc trie to avoid slow cases This adds a test for the pure case described [here](#6258 (comment)). When added, this test will time out in debug (~20 sec release) with an `N` of 3. Looking with a profiler, it is spending most of its time in [`Vec.contains`](https://github.com/rust-lang/cargo/blob/d08fd375db0014f097ee3fbdc1c595b632669f6a/src/cargo/core/resolver/conflict_cache.rs#L84). Keeping a deduplicated list with contains is `O(n^2)`, so let's change things to be `Ord` so we can use a btree to deduplicate. Now the profiler claimed that we are spending our time [searching that `Vec`](https://github.com/rust-lang/cargo/blob/d08fd375db0014f097ee3fbdc1c595b632669f6a/src/cargo/core/resolver/conflict_cache.rs#L66). So time to work on that "vaporware", the simplest thing I could think of that lets us check for is_active in better then `O(n)` is nested hashmaps. Each hashmap has keys of a shared Id, and values of hashmaps representing all the conflicts that use that Id. When searching if the key is not active then we need not look at any of its descendants. There are lots of ways to make this better but even this much gets the test to pass with `N` of 3. So maybe we can justify the improvements with `N` of 4? No, eavan in debug `N` of 4 hits the [50k limit](https://github.com/rust-lang/cargo/blob/d08fd375db0014f097ee3fbdc1c595b632669f6a/src/cargo/core/resolver/types.rs#L55), so the that is as far as we need to go on the conflict_cache.
|
☀️ Test successful - status-appveyor, status-travis |
|
I was seeing some non-determinism on the run time of some tests, and trying to figure out where it was coming from. I think it is from here. I do not have proof that this can lead to different lock files, but strongly suspect it. |
|
On discord @alexcrichton clarified that the resolver does not have to be deterministic. In most use cases the index will change between any two invocations anyway. |
ConflictStoreTrie: Faster filtered search This is an optimization that I was thinking of doing in #6283. I did not then as this is optimizing a not particularly hot path. I wish to do it now as it is stuck in my head, and I need that head space for more important things. This also "accidentally" fixes the [indeterminacy](#6283 (comment)) introduced in #6283, by replacing the `HashMap.iter().find()` like code with `BTreeMap.iter().find()` like code. This is not strictly needed, as @alexcrichton pointed out (In most use cases the index will change between any two invocations anyway), but does make it much easier to deal with (fuzz) test cases.
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.
This adds a test for the pure case described here. When added, this test will time out in debug (~20 sec release) with an
Nof 3.Looking with a profiler, it is spending most of its time in
Vec.contains. Keeping a deduplicated list with contains isO(n^2), so let's change things to beOrdso we can use a btree to deduplicate.Now the profiler claimed that we are spending our time searching that
Vec. So time to work on that "vaporware", the simplest thing I could think of that lets us check for is_active in better thenO(n)is nested hashmaps. Each hashmap has keys of a shared Id, and values of hashmaps representing all the conflicts that use that Id. When searching if the key is not active then we need not look at any of its descendants.There are lots of ways to make this better but even this much gets the test to pass with
Nof 3. So maybe we can justify the improvements withNof 4? No, eavan in debugNof 4 hits the 50k limit, so the that is as far as we need to go on the conflict_cache.