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: do not recompute the intersection when backtracking #171

Merged
merged 3 commits into from
Dec 21, 2023

Conversation

Eh2406
Copy link
Member

@Eh2406 Eh2406 commented Dec 20, 2023

This eliminates the intersection calls made when backtracking. Unfortunately, it is within the noise on all of my standard benchmarks. However it does cause dramatic speedups for #135 (comment) (I really need to make a ron file of this, so that I can add it to my normal benchmarking suite.)

Based on the flame graphs we reviewed in Office Hours today I suspect this will make a solid difference for @konstin workloads. It also unlocks more impactful simplifications in satisfier search.

@mpizenberg
Copy link
Member

Let me know if my understanding is correct.

This PR is a memory/compute trade off.

Instead of only keeping the latest assignments intersection, we keep a record of intersections at each stage by storing it in dated_derivations.

This change prevents the need to recompute intersections when backtracking since dated derivations also store the intersection at the corresponding time.

I suppose AssignmentsIntersection::Derivations could now use a reference "&" to the latest dated derivation intersection instead of its own copy?

@konstin
Copy link
Member

konstin commented Dec 21, 2023

Great work, that makes the two bad cases i test a good bit faster:

main: f64c499
branch: 8d5cfbb

tf-models-nightly main
real    0m5,509s
user    0m5,228s
sys     0m0,274s

tf-models-nightly branch
real    0m2,905s
user    0m2,630s
sys     0m0,272s

bio_embeddings main
real    2m7,274s
user    2m5,410s
sys     0m1,396s

bio_embeddings branch
real    1m42,388s
user    1m40,671s
sys     0m1,533s

@Eh2406
Copy link
Member Author

Eh2406 commented Dec 21, 2023

@mpizenberg, yes your understanding is correct.

I suppose AssignmentsIntersection::Derivations could now use a reference "&" to the latest dated derivation intersection instead of its own copy?

Theoretically yes. But then PackageAssignments become self-referential. I experimented not storing anything in AssignmentsIntersection::Derivations and retrieving the intersection with pa.dated_derivations.last().unwrap() but the code was too convoluted for me to find the bug I had introduced. We could share the memory with an RC but given its only can be used in two places it's probably the same cost as a clone. So I decided any larger change should be done in a follow-up.

@Eh2406
Copy link
Member Author

Eh2406 commented Dec 21, 2023

I fixed the comment and improved some redundancy. This should be ready for a final review.

I also generated a Ron file inspired by the example I've been using for testing. slow_135_0_u16_NumberVersion.txt (change from txt to ron to use). It's runtime gets about 50% faster after this PR. How realistic is it 🤷 .

@Eh2406 Eh2406 added this pull request to the merge queue Dec 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 21, 2023
@Eh2406 Eh2406 changed the title perf: do not recompute the accumulated_intersection when backtracking perf: do not recompute the intersection when backtracking Dec 21, 2023
@Eh2406 Eh2406 added this pull request to the merge queue Dec 21, 2023
Merged via the queue into dev with commit 8e11a81 Dec 21, 2023
5 checks passed
@Eh2406 Eh2406 deleted the accumulated_intersection branch December 21, 2023 17:54
konstin pushed a commit to astral-sh/pubgrub that referenced this pull request Dec 26, 2023
…#171)

* perf: do not recompute the accumulated_intersection when backtracking

* more correct comment message

* make things dry again
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
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