-
Notifications
You must be signed in to change notification settings - Fork 37
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
perf: skip preemptively looking for conflicts befor a backtrack #252
Conversation
Really nice find! I’m less sold regarding the usage of lazy cell. If I understood correctly, it is to avoid the expensive precomputation of the the value used in the iterator function. It sounds like something that control flow would solve. Doing instead something like: if !self.has_ever_backtracked {
// log and add decision
} else {
exact = ...
not_satisfied = ...
if store[new_incompatibilities].iter().all(not_satisfied) {
// log and add decision
} else {
// else branch
}
} Also I’m wondering if the lazy_cell version would add an extra annoyance for threaded/parallel versions of the code. |
Good suggestion. A control flow based solution ends up with much clearer code, if a slightly bigger diff. I think the lazy cell would only cause a threaded/parallel problem if someone tried to parallelize the |
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.
Thanks for the doc comments!
Interesting! I like it. |
There are some changes in snapshots due to pubgrub-rs/pubgrub#252
This PR caused some minor change uv conflict reporting: https://github.com/astral-sh/uv/pull/8245/files |
It should be expected that error reporting can change, because the resolution path is changed with this PR. |
There are some changes in snapshots due to pubgrub-rs/pubgrub#252
…rub-rs#252) * perf: skip preemptively looking for conflicts befor a backtrack * document the new optimization
add_version
does a preemptive quick check the first time we had a selected version, to see if one of the just added dependencies would be incompatible with activating this version. This check is theoretically unnecessary. Without it, we would add the new dependencies and the version then proceed to unit propagation where we would backtrack to try a different version. However backtracking is quite expensive. So this preemptive check is on the whole worth it. However it's unfortunate overhead when checking the lock file, where the probability of a conflict is extremely low. Even without a lock file, most resolutions succeed with the first thing they tried.This PR adds a Boolean to keep track of whether we have ever backtrack. Before the first backtrack, we assume that conflicts are unlikely and so skip the preemptive check. Once we've seen a backtrack, we returned to the current behavior.
The
elm
benchmark improves by 12%.zuse
improved by 10%. All of Crates (excluding Solana) improves by 5% without a lock file and by 10% checking lock files.