Skip to content
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

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

Eh2406
Copy link
Member

@Eh2406 Eh2406 commented Aug 27, 2024

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.

@Eh2406 Eh2406 requested a review from mpizenberg August 27, 2024 21:56
@mpizenberg
Copy link
Member

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.

@Eh2406
Copy link
Member Author

Eh2406 commented Aug 28, 2024

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 store[new_incompatibilities].iter().all(not_satisfied) line. Which seems unlikely.

Copy link
Member

@mpizenberg mpizenberg left a 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!

@zanieb
Copy link
Member

zanieb commented Aug 28, 2024

Interesting! I like it.

@Eh2406 Eh2406 added this pull request to the merge queue Aug 28, 2024
Merged via the queue into pubgrub-rs:dev with commit 4b4b444 Aug 28, 2024
4 checks passed
@Eh2406 Eh2406 deleted the precheck_only_after_backtrack branch August 28, 2024 17:50
konstin added a commit to astral-sh/uv that referenced this pull request Oct 16, 2024
There are some changes in snapshots due to pubgrub-rs/pubgrub#252
@konstin
Copy link
Member

konstin commented Oct 16, 2024

This PR caused some minor change uv conflict reporting: https://github.com/astral-sh/uv/pull/8245/files

@mpizenberg
Copy link
Member

It should be expected that error reporting can change, because the resolution path is changed with this PR.

konstin added a commit to astral-sh/uv that referenced this pull request Oct 16, 2024
There are some changes in snapshots due to pubgrub-rs/pubgrub#252
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants