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: invalidate less contradicted_incompatibilities #170

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

Eh2406
Copy link
Member

@Eh2406 Eh2406 commented Dec 18, 2023

This extends the work from #88. To quote:

It is possible to figure out witch IncompIds are still used after a backtrack, but so far nothing that is pulls its weight.

Looking at our normal benchmarks this is still within the noise. Looking at hobo it is a 5% improvement. However it does reduce the number of times intersection is called, and our users are telling us that intersections in their code are more expensive than in our benchmarks.

There are more efficient alternatives to calling retain(|_, dl| *dl <= decision_level). (The insertion order is also sorted by dl, so binary search and truncate is an option. Or ...) But I have not seen it in profiles so I don't think it is worth more complexity at this time.

If this is a big win for our users then we should merge soon. Otherwise, this can stick around until other optimizations make the background noise smaller.

@konstin
Copy link
Member

konstin commented Dec 19, 2023

Just cherry picking 976d03e onto our tagged branch speeds my test up from a good 4s to 1.7s-1.8s! We'll have to rebase onto the main to measure with the others speedups, once that is done i'll make a proper hyperfine benchmark and report back.

@Eh2406
Copy link
Member Author

Eh2406 commented Dec 19, 2023

That sounds like a big win! Hopefully this can get a review and get merged soon.

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.

So what this PR does if I've understood it correctly, is record the decision level for each contradicted incompatibility. Then when backtracking, instead of resetting all contradicted incompats, we keep those from decisions previous to the backtrack point.

Sounds logical and it's great that this improves real world use cases.

src/internal/core.rs Outdated Show resolved Hide resolved
@konstin
Copy link
Member

konstin commented Dec 20, 2023

When i cherry-pick 976d03e onto f64c499 the speedup is lower but still there. For the tf-models-nightly benchmark, which includes running through a lot of incompatible versions:

Benchmark 1: main
  Time (mean ± σ):      5.235 s ±  0.084 s    [User: 4.932 s, System: 0.295 s]
  Range (min … max):    5.157 s …  5.417 s    10 runs

Benchmark 2: branch
  Time (mean ± σ):      4.875 s ±  0.066 s    [User: 4.578 s, System: 0.291 s]
  Range (min … max):    4.795 s …  4.970 s    10 runs

Summary
  branch ran
    1.07 ± 0.02 times faster than main

@Eh2406 Eh2406 force-pushed the contradicted_incompatibilities branch from 976d03e to 7c98334 Compare December 20, 2023 17:15
@Eh2406
Copy link
Member Author

Eh2406 commented Dec 20, 2023

That makes sense. When #163 applies, as it does in your case, it entirely removes the old incompatibilities from consideration. This PR keeps track of the fact that the old incompatibilities can be skipped, which is much less useful if they're not being considered at all. I'm glad to see that despite the redundancy this PR pulls its weight.

It is easy to imagine cases where #163 does not apply, where this PR could have a much larger impact. Closer to the one you originally measured.

@Eh2406 Eh2406 added this pull request to the merge queue Dec 20, 2023
Merged via the queue into dev with commit 526775f Dec 20, 2023
5 checks passed
@Eh2406 Eh2406 deleted the contradicted_incompatibilities branch December 20, 2023 17:25
konstin pushed a commit to astral-sh/pubgrub that referenced this pull request Dec 21, 2023
* Invalidate less contradicted_incompatibilities

* update comments
konstin pushed a commit to astral-sh/pubgrub that referenced this pull request Dec 26, 2023
* Invalidate less contradicted_incompatibilities

* update comments
konstin added a commit to astral-sh/uv that referenced this pull request Dec 28, 2023
konstin added a commit to astral-sh/uv that referenced this pull request Dec 28, 2023
konstin pushed a commit to astral-sh/pubgrub that referenced this pull request Jan 3, 2024
* Invalidate less contradicted_incompatibilities

* update comments
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.

3 participants