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

pkg: version comparison fastpath #11273

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Jan 6, 2025

Solving dependencies requires performing many package version comparisons. Opam's logic for comparing versions is complicated in the worst case, however in practice most package versions follow an approximation of semantic versioning. In such cases the components of the version can be packed into a single integer value which can be efficiently compared.

This change introduces a pre-processing step for package versions that packs them into an int of possible and uses int comparison when comparing package versions when possible.

This is based on an optimization in python's uv package manager.

I benchmarked this change by solving https://github.com/gridbugs/climate where the time to solve went from ~2.6s to ~2.3s.

With #11264 applied the solve time goes from ~0.65s to ~0.55s.

For larger projects (e.g. bonsai) the improvement is surprisingly less pronounced. I don't yet understand why this is.

Solving dependencies requires performing many package version
comparisons. Opam's logic for comparing versions is complicated in the
worst case, however in practice most package versions follow an
approximation of semantic versioning. In such cases the components of
the version can be packed into a single integer value which can be
efficiently compared.

This change introduces a pre-processing step for package versions that
packs them into an int of possible and uses int comparison when
comparing package versions when possible.

This is based on an optimization in python's uv package manager.

Signed-off-by: Stephen Sherratt <[email protected]>
@Leonidas-from-XIV
Copy link
Collaborator

I think this is a cool idea! For such optimizations I think it would be good to have a simple microbenchmark thing to be able to tell whether some change makes it faster or slower - I have some ideas but I would like to be able to estimate the performance impact of the suggestion.

@rgrinberg
Copy link
Member

How about using the version comparison in this blog post? https://roscidus.com/blog/blog/2024/07/22/performance-2/ Does it not yield us enough of a benefit?

Also, since this is a change in the vendored library, it needs to have a patch in our "fork" repo

@gridbugs
Copy link
Collaborator Author

gridbugs commented Jan 9, 2025

I'll try out the idea from that post and compare it to my change. For benchmarks I think a macrobenchmark would be more useful so our optimizations are guided by real-life workloads. Perhaps we could use the solve-times of a few different packages of different sizes?

@Leonidas-from-XIV
Copy link
Collaborator

The problem with real-life package testing is, while also useful, that they introduce a lot of noise due to I/O and are potentially slow. If I want to compare 1000 runs of the optimized code to 1000 runs to the older code, the difference is harder to see if each runs also e.g. loads 1000 OPAM files via calls to git and I have to wait an hour for them to finish.

@rgrinberg
Copy link
Member

I'm inclined to agree with Marek. It should be possible to write a version comparison function that is faster on all inputs. So a micro benchmark on say 100 common version comparison pairs should hopefully demonstrate that one algorithm is strictly superior to the other one.

At a first glance, it seems like your approach is the more promising one. Though I would like to confirm this first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants