Registry functions return Poll to enable parallel fetching of index data #10064
Conversation
rust-highfive
added
the
S-waiting-on-review
|
I've updated the @jonhoo 's http registry prototype based on this change and the initial results look good. Not quite ready for PR yet though: arlosi@dd5eb9e#diff-cb43b9ba868eedb392ea22664d59b4444dd7c4c4f5e641ab045aa861a38ffa4c |
|
Thank you for picking this up!
I assumed we would have an API more like "Block until at least one outstanding request is Poll::Ready". But having thought about it the conditions for my API to be helpful are corner cases of corner cases. And if you're only making one request your API prevents needing a retry loop, |
|
Thanks for picking this up @arlosi! Are you interested in pushing this through to completion? I've got some thoughts about how best to go about that, but if you mostly just wanted to rebase it may not be too interesting to discuss my thoughts at this time. |
|
@alexcrichton yes, I'd like to get this completed! Any suggestions you have to move it forward would be awesome. |
|
Ok so talking with @Eh2406 a bit yesterday one of the concerns is that while this infrastructure makes sense none of it's really being exercised yet (since everything is always "ready"). I think that we will actually want to change how remote sources (git/registry) work today to actually leverage this new functionality, however. Basically I think that the |
|
I'm finally coming back to this in the new year. @alexcrichton I'm hoping for some clarification on what you'd like here for the existing git model. Currently, the cargo/src/cargo/sources/registry/mod.rs Line 695 in 2478331 All the blocking work currently happens in the This change has the potential to become large, depending on how much of the design is changed and I want to make sure we get agreement on how we want it to look before doing the work. |
|
Yes that's a good point, and also an indicator at how the current design is a bit sprawling... Currently there's basically an implicit "protocol" between the resovler and sources that the resolve will appropriately, for some definition of "appropriately" call Instead I think a better model would be to remove If the same loop ends up being written in Cargo then that seems emminently solvable with a helper method or similar, I don't think we need to worry too much about the impact. The purpose of this change is to enable parallel downloads and updates for the resolver, and if everything else doesn't act in parallel that's ok we can update them later over time. |
|
☔ The latest upstream changes (presumably #10269) made this pull request unmergeable. Please resolve the merge conflicts. |
updating, registries now update as needed. To force a registry to ensure the latest copy of crate metadata is available, a new method called `invalidate_cache` is added. This method will cause the registry to update the next time `block_until_ready` is called.
|
@alexcrichton I updated this PR to remove the If the registry has no local cache (hasn't been cloned yet, for instance), then queries will return I also added an "invalidate_cache" method on the registry that makes subsequent queries return |
| } | ||
| }); | ||
| for (_, source) in sources.iter_mut() { | ||
| source.block_until_ready().ok()?; |
There was a problem hiding this comment.
So I think one of the fundamental questions is can block_until_ready spuriously wake?
I.E. If block_until_ready returns, can the next call to query return Poll::Pending?
If Yes, then this loop is correct. But we should document it.
If No, then this loop seems like overkill. As it has to pass on the second iteration. (The same pattern is also true in other loops but I'm only gonna comment here.)
Another fundamental question is what happens if you call block_until_ready on a source that has not yet returned Poll::Pending?
To be specific, do we iterate over sources.iter_mut() or only the ones that are still in package_ids?
There was a problem hiding this comment.
For a given source and query parameters, a query call that previously returned Poll::Pending must return Poll::Ready after calling block_until_ready. If the query parameters are different (such as querying for a package that hasn't been queried before), then query may return Poll::Pending, even after a block_until_ready call.
Calling block_until_ready if a source hasn't yet returned a Poll::Pending is an interesting question. If you call invalidate_cache block_until_ready, the git-based sources will do a fetch.
I agree we should be more precise here and only call block_until_ready for the sources remaining in package_ids
There was a problem hiding this comment.
For a given source and query parameters, a
querycall that previously returnedPoll::Pendingmust returnPoll::Readyafter callingblock_until_ready. If the query parameters are different (such as querying for a package that hasn't been queried before), thenquerymay returnPoll::Pending, even after ablock_until_readycall.
If we trust that must then we don't need a loop. So instead of "loops" like:
let config = loop {
match self.config()? {
Poll::Ready(cfg) => break cfg.unwrap(),
Poll::Pending => self.block_until_ready()?,
}
};We can have:
let config = match self.config()? {
Poll::Ready(cfg) => cfg.unwrap(),
Poll::Pending => {
self.block_until_ready()?;
self.config()?.expect("must get Pending after block_until_ready").unwrap()
}
};of course will probably want some kind of helper to DRY the redundant self.config()?. And possibly a helper to deal with (the two) places where we have a vec of things to query.
Overnight the possibility of weakening it to should started to grow on me. So someone implementing the trait should make sure that the second query always returns a Ready. But, someone using the trait has to loop calling block_until_ready until the query returns Ready.
Whatever we decide we should document it on the trait.
Calling
block_until_readyif a source hasn't yet returned aPoll::Pendingis an interesting question. If you callinvalidate_cacheblock_until_ready, the git-based sources will do a fetch.
I think we should define block_until_ready before Poll::Pending/invalidate_cache as a NOP. For example this loop calls block_until_ready on all sources even if only some of them need it:
cargo/src/cargo/core/registry.rs
Lines 730 to 734 in f12f025
Keeping track of witch ones need it, seams like a lot of extra work.
Can we document on the trait when it is an NOP, and point out that it is a recommended optimization to avoid unneeded calls to
block_until_ready?
There was a problem hiding this comment.
Ok, I've changed it to should return Poll::Ready after block_until_ready and updated the doc comments. Let me know if it makes more sense now!
| ) | ||
| })?; | ||
|
|
||
| let summaries = match source.query_vec(dep)? { |
There was a problem hiding this comment.
I was surprised at what ? did on a Poll. But https://github.com/rust-lang/rust/blob/582b6964a8868c9714881d9821d08415a8f4f13b/library/core/src/task/poll.rs#L272-L274
is exactly what we need here.
There was a problem hiding this comment.
This is nightly only as try_trait_v2, so we can't use it, right?
There was a problem hiding this comment.
On stable you can not imple the trait in try_trait_v2. You can use ?, even though it de-sugars to them.
There was a problem hiding this comment.
Ok, so I am already using the try_trait_v2 here already. That's what's enabling ? to cause Poll::Ready(Err(...)) to return up.
Maybe I'm missing something, but I don't see any way to use it further here.
There was a problem hiding this comment.
Yes, sorry for the miss communication. Given how ? works on Poll your code is correct. I was leaving a note that ? did not do what I expected.
| Poll::Pending => match registry.block_until_ready() { | ||
| Ok(()) => continue, | ||
| Err(e) => return to_resolve_err(e), | ||
| }, |
There was a problem hiding this comment.
@alexcrichton you asked for this loop to be removed in #8985 (comment) on the previous PR, what do you think now?
| @@ -17,6 18,7 @@ pub struct DirectorySource<'cfg> { | |||
| root: PathBuf, | |||
| packages: HashMap<PackageId, (Package, Checksum)>, | |||
| config: &'cfg Config, | |||
| updated: bool, | |||
| } | |||
There was a problem hiding this comment.
Would it be clearer to use packages.is_empty() as the unupdated state?
There was a problem hiding this comment.
I think that could cause an infinite updating loop if the directory source had no packages in it.
|
I'm going to be stepping down from the Cargo team so feel free to merge this without me, I unfortunately won't be able to get around to reviewing this. |
|
Without alex watching over us we will have to make our mistakes the hard way. @bors r |
|
📌 Commit 0c07056 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
This PR is causing errors when integrating upstream. Can you take a look at it? You can test in the rust-lang/rust repo by doing:
I am seeing the following errors: |
|
These errors are from running |
|
@ehuss looking into it now. |
…tself. This enables registry implementations to signal if the cache is valid on a per-request basis. Fixes a bug introduced by rust-lang#10064 that caused Cargo not to update for several cases in a release build because it believed the index cache to be valid when it was not.
Refactor RegistryData::load to handle management of the index cache Enables registry implementations to signal if the cache is valid on a per-request basis. Fixes a bug introduced by #10064 that caused Cargo not to update for several cases in a release build because it believed the index cache to be valid when it was not. The issue only occurred in release builds because debug builds verify that the cache contents is correct (by refreshing it). Previously `current_version` was called by the index to determine whether the cache was valid. In the new model, `RegistryData::load` is passed the current version of the cache and returns an enum to indicate the status of the cached data. r? `@eh2406` cc `@ehuss`
Update cargo 9 commits in 65c82664263feddc5fe2d424be0993c28d46377a..109bfbd055325ef87a6e7f63d67da7e838f8300b 2022-03-09 02:32:56 0000 to 2022-03-17 21:43:09 0000 - Refactor RegistryData::load to handle management of the index cache (rust-lang/cargo#10482) - Separate VCS command paths with "--" (rust-lang/cargo#10483) - Fix panic when artifact target is used for `[target.'cfg(<target>)'.dependencies` (rust-lang/cargo#10433) - Bump [email protected] and [email protected] (rust-lang/cargo#10479) - vendor: Don't allow multiple values for --sync (rust-lang/cargo#10448) - Use types to make clere (credential process || token) (rust-lang/cargo#10471) - Warning on conflicting keys (rust-lang/cargo#10316) - Registry functions return Poll to enable parallel fetching of index data (rust-lang/cargo#10064) - Refine the contributor guide (rust-lang/cargo#10468)
Upgrade Cargo to 0.62.0 Address the changes on rust-lang/cargo#10064
…tself. This enables registry implementations to signal if the cache is valid on a per-request basis. Fixes a bug introduced by rust-lang#10064 that caused Cargo not to update for several cases in a release build because it believed the index cache to be valid when it was not.
…tself. This enables registry implementations to signal if the cache is valid on a per-request basis. Fixes a bug introduced by rust-lang#10064 that caused Cargo not to update for several cases in a release build because it believed the index cache to be valid when it was not.
the main change seems to be that registries no longer have an explicit update() call, but instead return `Poll` on queries with a helper to block until the Poll is ready. upstream change for reference: rust-lang/cargo#10064 Signed-off-by: Fabian Grünbichler <[email protected]>
Adds
Pollas a return type for several registry functions to enable parallel fetching of crate metadata with a future http-based registry.Work is scheduled by calling the
queryand related functions, then waited on withblock_until_ready.This PR is based on the draft PR started by eh2406 here #8985.
r? @Eh2406
cc @alexcrichton
cc @jonhoo