Caching the dependencies#6853
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
be40c3a to
2246b8b
Compare
|
☔ The latest upstream changes (presumably #6840) made this pull request unmergeable. Please resolve the merge conflicts. |
2246b8b to
2bd11f0
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Looks pretty reasonable to me, thanks for separating the commits! I think the organization between DepCache and RegistryQueryer may want to be tweaked slightly, but it's not the end of the world either way.
The main thing I think this needs is to expand the documentation as to why this caching strategy is valid, aka just some documentation on DepsCache as to what it's caching and why it's possible.
There was a problem hiding this comment.
This seems like it's somewhat of a loss, but is there a reason we couldn't use &'a BTreeSet here for the Required list?
There was a problem hiding this comment.
I don't see how. (love to be proven wrong.) This change was to get rid of the lifetime, so that it could be used as the key in the DepsCache above. Then I switched from a Vec to a BTreeSet to reduce the number of equivalent configurations that can end up in the cache. Then added the Rc as that was the kind of reference I usually had.
This explanation should be a comment. I will add after #6860 lands.
There was a problem hiding this comment.
Similar to below, could this document the return value and what each value of the tuple is?
There was a problem hiding this comment.
(also while you're at it if you can add general documentation for what this method is doing that'd be great!
There was a problem hiding this comment.
Could a comment be added here as to why we can cache the results? It'd be good to explain why this cache works the way it does and what enables it (e.g. the things that don't change and how this is a "pure" query which can be memoized.
|
☔ The latest upstream changes (presumably #6860) made this pull request unmergeable. Please resolve the merge conflicts. |
2bd11f0 to
0c24471
Compare
0c24471 to
5ae13e3
Compare
|
How does this look now? |
|
@bors: r+ Looks great to me, thanks for the updates! |
|
📌 Commit 79714ba has been approved by |
bors
added
S-waiting-on-bors
Caching the dependencies There are 2 sources of facts for the resolver: 1. The `Registry` tells us for a Dependency what versions are available to fulfil it. 2. The `Summary` tells us for a version (and features) what dependencies need to be fulfilled for it to be activated. The `Registry` was cached with a `RegistryQueryer` back in #5112. This adds a `DepsCache` to cache the calculation of which dependencies are activated by features. In the happy path `flag_activated` means that we don't get to reuse `build_deps`, but the more we backtrack the more time we save. In pathological cases like #6258 (comment), I have measured this as a 10% improvement with release. This also means that `build_deps` can be run in a context free way, which may be useful in a follow up PR to solve #6258 (comment).
|
☀️ Test successful - checks-travis, status-appveyor |
6 participants
There are 2 sources of facts for the resolver:
Registrytells us for a Dependency what versions are available to fulfil it.Summarytells us for a version (and features) what dependencies need to be fulfilled for it to be activated.The
Registrywas cached with aRegistryQueryerback in #5112. This adds aDepsCacheto cache the calculation of which dependencies are activated by features.In the happy path
flag_activatedmeans that we don't get to reusebuild_deps, but the more we backtrack the more time we save. In pathological cases like #6258 (comment), I have measured this as a 10% improvement with release.This also means that
build_depscan be run in a context free way, which may be useful in a follow up PR to solve #6258 (comment).