-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Caching the dependencies #6853
Caching the dependencies #6853
Conversation
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
be40c3a
to
2246b8b
Compare
☔ The latest upstream changes (presumably #6840) made this pull request unmergeable. Please resolve the merge conflicts. |
2246b8b
to
2bd11f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -89,26 +89,26 @@ impl ResolverProgress { | |||
} | |||
} | |||
|
|||
#[derive(Clone, Copy)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/cargo/core/resolver/dep_cache.rs
Outdated
@@ -167,6 +167,36 @@ impl<'a> RegistryQueryer<'a> { | |||
} | |||
} | |||
|
|||
pub fn build_deps( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to below, could this document the return value and what each value of the tuple is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also while you're at it if you can add general documentation for what this method is doing that'd be great!
candidate: &Summary, | ||
method: &Method, | ||
) -> ActivateResult<Rc<(Vec<String>, HashSet<InternedString>, Rc<Vec<DepInfo>>)>> { | ||
if let Some(out) = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 |
There are 2 sources of facts for the resolver:
Registry
tells us for a Dependency what versions are available to fulfil it.Summary
tells us for a version (and features) what dependencies need to be fulfilled for it to be activated.The
Registry
was cached with aRegistryQueryer
back in #5112. This adds aDepsCache
to cache the calculation of which dependencies are activated by features.In the happy path
flag_activated
means 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_deps
can be run in a context free way, which may be useful in a follow up PR to solve #6258 (comment).