-
Notifications
You must be signed in to change notification settings - Fork 805
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
Speed up cold cache urllib3/boto3/botocore with batched prefetching #2452
Conversation
601bcc8
to
775b2a0
Compare
thanks for working on this @konstin , looking forward for this massive speed improvement :) . We're currently waiting for |
You might want to consider pinning or bounding boto3 or urllib3, regardless of what you use to resolve / install. |
775b2a0
to
b7695c4
Compare
Batch prefetching needs more information from the candidate selector, so i've split `select` into methods. Split out from #2452. No functional changes.
e83f868
to
6c6dbd3
Compare
|
||
// Run the fetcher. | ||
let requests_fut = self.fetch(request_stream).fuse(); | ||
|
||
// Run the solver. | ||
let resolve_fut = self.solve(request_sink).fuse(); | ||
let resolve_fut = self.solve(request_sink).boxed().fuse(); |
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.
Once again making windows happy
4d9ce8a
to
116a028
Compare
Batch prefetching needs more information from the candidate selector, so i've split `select` into methods. Split out from #2452. No functional changes.
116a028
to
52cf8ce
Compare
compatible, | ||
previous: candidate.version().clone(), | ||
}; | ||
candidate |
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 think we should probably only do this prefetching when the candidate has a wheel. Otherwise, we risk building tons and tons of source distributions.
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.
Good catch
Range::strictly_higher_than(previous) | ||
}; | ||
if let Some(candidate) = | ||
selector.select_no_preference(package_name, &range, version_map) |
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 feel like this risks being quadratic, because we iterate over total_prefetch
, and then select_no_preference
is also linear, right? Is there any way to improve that?
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 did an attempt turning select_candidate
into an iterator but then looked at the profiles for urllib3/boto3/botocore and i think it's not worth it.
With pubgrub being fast for complex ranges, we can now compute the next n candidates without taking a performance hit. This speeds up cold cache
urllib3<1.25.4
boto3
from maybe 40s - 50s to ~2s. See docstrings for details on the heuristics.Before
After
We need two parts of the prefetching, first looking for compatible version and then falling back to flat next versions. After we selected a boto3 version, there is only one compatible botocore version remaining, so when won't find other compatible candidates for prefetching. We see this as a pattern where we only prefetch boto3 (stack bars), but not botocore (sequential requests between the stacked bars).
The risk is that we're completely wrong with the guess and cause a lot of useless network requests. I think this is acceptable since this mechanism only triggers when we're already on the bad path and we should simply have fetched all versions after some seconds (assuming a fast index like pypi).
It would be even better if the pubgrub state was copy-on-write so we could simulate more progress than we actually have; currently we're guessing what the next version is which could be completely wrong, but i think this is still a valuable heuristic.
Fixes #170.