Track outdated priorities in a set #313
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pubgrub currently assumes that package priorities can only change when the derivations of a package change. In tracks the decision level range where derivations changed, and only updates decisions in this level. This assumption is incorrect for uv, where the priority doesn't depend on the range, but on specifiers and discovery order per package name, not per `DP::P`, which contains virtual packages, too. This change enables more flexible priority updating for uv now and for pubgrub generally in the future. Instead of updating all package priorities in a decision level range, we directly track the set of packages that need to be updated because we changed their derivations. This enables adding pushing priority updates from uv in our fork. Currently, packages that are removed from prioritization, those that need to be changed and those that are added are all treated the same way, there might be some optimization by telling them apart. This branch is based on dev...Eh2406:pubgrub:stop-prioritize.
|
@Eh2406 can you run the cargo perf numbers on this? on uv it's perf neutral with astral-sh/uv#10935 |
CodSpeed Instrumentation Performance ReportMerging #313 will improve performances by 6.38%Comparing Summary
Benchmarks breakdown
|
When the user's implementation breaks the contract about choose version, we now panic. This contract is trivial to enforce (`vs.contains(v)`), so an error path does not make sense. Fixes #239
Eh2406
reviewed
Jan 28, 2025
src/internal/partial_solution.rs
Outdated
| // Throw away all stored priority levels, And mark that they all need to be recomputed. | ||
| self.prioritized_potential_packages.clear(); | ||
| self.prioritize_decision_level = self.current_decision_level.0.saturating_sub(1) as usize; | ||
| // Throw away all stored priority levels and mark them for recomputing |
There was a problem hiding this comment.
the code this comment refers to has been removed.
|
I just checked, no measurable difference on CPU time or memory usage for crates.io. So the question is is the code going to be easier to maintain this way? |
Eh2406
reviewed
Jan 28, 2025
src/internal/partial_solution.rs
Outdated
| @@ -62,8 +63,6 @@ pub(crate) struct PartialSolution<DP: DependencyProvider> { | |||
| /// range. | |||
There was a problem hiding this comment.
The comet ending on this line is now out of date.
I find the set-based approach significantly easier to understand. |
Eh2406
approved these changes
Jan 29, 2025
Sounds logical to me too. |
|
It also looks faster in uv in a quick test (p = 1.18e-08): |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
pubgrub currently assumes that package priorities can only change when the derivations of a package change. In tracks the decision level range where derivations changed, and only updates decisions in this level.
This assumption is incorrect for uv, where the priority doesn't depend on the range, but on specifiers and discovery order per package name, not per
DP::P, which contains virtual packages, too. This change enables more flexible priority updating for uv now and for pubgrub generally in the future.Instead of updating all package priorities in a decision level range, we directly track the set of packages that need to be updated because we changed their derivations. This enables adding pushing priority updates from uv in our fork.
Currently, packages that are removed from prioritization, those that need to be changed and those that are added are all treated the same way, there might be some optimization by telling them apart.
This branch is based on dev...Eh2406:pubgrub:stop-prioritize.
Half of #239, incidentally