-
Notifications
You must be signed in to change notification settings - Fork 38
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
Old behavior when there is no previous_satisfier #257
base: dev
Are you sure you want to change the base?
Conversation
I've tried this branch with the uv fork: https://github.com/astral-sh/uv/compare/konsti/pubgrub-257 A lot of messages become clearer: We're not starting with some dependencies, but with your project and its direct dependencies. old:
new:
Here the new one feels like it skips less steps. old:
new:
Neutral to slight regression: This one feels less consistent now, instead of either doing it from the end or from the beginning, we're now backtracking from the middle. old:
new:
This one is imo a regression, the "all of" list is strange: old:
new:
Overall, I like the strategy of starting from the project instead of the deepest dep more, but we should do one or the other consistently, right now we sometimes start in the middle. @zanieb what do think |
I'm not really sure if it's better to start with "your project depends on xyz" or with "there are no versions of xyz". On one hand, it's nice to have a logical forward progression, but it's also nice to get to the reason the resolution failed quickly. As Konsti suggested, starting in the middle is definitely not ideal. Generally, the way that incompatibilities are ordered by PubGrub are a mystery to me and I am okay with changes. Let us know if you want us to do more testing. |
CodSpeed Performance ReportMerging #257 will improve performances by ×40Comparing Summary
Benchmarks breakdown
|
With this PR, resolution time for |
In #79 we removed the special case described in the original PubGrub documentation as handling there not being a
previous_satisfier
. This PR brings that special case back. We believe that this special case happens when there is only one package referenced in the conflict, and that package was depended on by only one other package. Specifically under some circumstances the old code would point out that the unitary conflict has been "almost satisfied" from the very beginning of history and so backjump to beginning of the problem. (It is always technically safe to have excessive backjump. Even restarting with no decisions each time there's a conflict would still get the correct answer.) Instead after #79 they would backjump only to where the package was first depended on, triggering resolving conflict by marking the depender is unavailable.On one hand, the post #79 code involves less backjumping and therefore less redundant decision-making and keeps the
state
free of unnecessary facts. On the other hand the old code put a fundamental conflict in the correct place in thestate
so that it will not get repeatedly removed and re-added when backjumping.Which one is faster depends on how much optimizations we've applied to different parts of the code and the exact shape of the problem. Crates.io (without Solana) gets 14% faster. The synthetic slow_135 (based on #135) gets 19% faster. elm, "large_case", and zule have small regressions that are as likely to be noise as real. The Sudoku problem gets ~4x slower. Based on extensive testing, all of these changes are due to the algorithmic change. The second commit which reduces allocations does not have a measurable impact on performance.
By changing the order incompatibilities are discovered and combined this changes error messages. Whether for the better or worse this repo does not have sufficient testing to determine.
Some relevant back links are