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

Improve performance #274

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

Improve performance #274

wants to merge 1 commit into from

Conversation

x-hgg-x
Copy link
Contributor

@x-hgg-x x-hgg-x commented Nov 7, 2024

This PR is an experiment to improve performance of Pubgrub.
There are many API breaking changes which should be reviewed before this can be merged.

The following benchmarks are done using the new bench file bench_backtracking.ron:

Commit Duration
dev branch (57971c4) 6.5s (estimated)
Use an arena for packages 7.58s
Use u64 for version set type 1.21s
Optimize incompatibility contradiction 661ms
Reduce AssignmentsIntersection size 660ms
Use smallvec crate in pubgrub 604ms
Use a Vec for incompatibilities and package assignments 434ms

Results for the large_case bench:

Commit Duration
dev branch (57971c4) 5.0616ms
This PR 3.5688ms

Results for the sudoku bench:

Commit Duration (easy/hard)
dev branch (57971c4) 1.62ms/1.86ms
This PR 1.32ms/1.43ms

Implementation example for cargo: https://github.com/x-hgg-x/pubgrub-bench

@Eh2406
Copy link
Member

Eh2406 commented Nov 7, 2024

Those are very impressive numbers. Thank you for looking into this!

I appreciate getting to see the unified vision of where everything is going. Obviously, with this much going on we are not going to be able to review it all in one PR. It's up to you how you want to divide things up into reviewable pieces.

@konstin just added some really cool automation for running our benchmarks in #271, which apparently I forgot to click merge on. So if we get the benchmark merged first we can easily and automatically see the improvements from the other pieces.

The non breaking changes do not seem all that controversial. So they may be easy to land. Of course, they may not pull their weight until the other (API-breaking) optimizations have come in.

The breaking changes will need to be looked at very carefully. Certainly each as their own PR. An order of magnitude performance improvement is nothing to sneeze at, but performance is not our only priority. Our existing API is careful to be "misuse resistant" in that it is hard to get an incorrect answer because you were "holding it wrong".

I look forward to reviewing this work, one piece at a time, over the next weeks/months.

@charliermarsh
Copy link
Contributor

I wonder if any of these changes can be broken off into smaller, non-breaking PRs?

@x-hgg-x x-hgg-x force-pushed the perf branch 2 times, most recently from b4ae053 to e559617 Compare November 8, 2024 09:51
@konstin
Copy link
Member

konstin commented Nov 8, 2024

That's an impressive speedup, and some deep changes!

Long and ramble-y comment ahead, here are some unstructured thoughts about this:

From spending a lot of time optimizing uv, i found that often making pubgrub do less work is the most effective strategy, for example by package prioritization (https://github.com/astral-sh/uv/blob/14507a1793f12bf884786be49d716ca8f8322340/crates/uv-resolver/src/pubgrub/priority.rs#L25) or astral-sh/uv#8157. If we're checking a large (exponential) combination of packages of versions, trying to get to a package selection that decides things that are either easy/clear or that reduce the remaining space a lot avoids doing work in pubgrub.

How did you create bench_backtracking.ron? Is there a way to bench the changes with cargo?

By splitting out the non-breaking changes, we can bench them in uv.

We need to decide which optimizations are in pubgrub and which are outside. For example, in uv we Arc our version and package types, so cloning them in pubgrub is cheap. When #242 or the arena-for-packages commit in this PR lands, we may stop Arcing our package type, and instead have pubgrub do that, since Arc inside an arena would be duplicated.

I'm not following along with the new Version type, how do i pass in a pep440_rs version with the new type? How do i represent large versions?

What version type is cargo currently using, can we do an optimization for small versions? One thing a had pitched to @Eh2406 earlier: semver::Version is 40 bytes, while most versions fit in a (u8, u8, u16) for (major, minor, patch), which can be in an enum with Box<semver::Version> for a total u64 type. Version comparisons are then a u32 comparison for most versions. We do similar optimizations in https://github.com/astral-sh/uv/blob/a80b6f51b07e0706800a0a9e8e0f5fe42651aa1b/crates/uv-pep440/src/version.rs#L263-L282.

Remove Eq bound for M: We want to retain that bound for #232.

@x-hgg-x
Copy link
Contributor Author

x-hgg-x commented Nov 8, 2024

How did you create bench_backtracking.ron?

This file was created from cargo, using https://github.com/x-hgg-x/pubgrub-bench, by dumping the output of get_dependencies() for solana-archiver-lib v1.1.12.

Is there a way to bench the changes with cargo?

See Eh2406/pubgrub-crates-benchmark#6 (comment).

By splitting out the non-breaking changes, we can bench them in uv.

I will make separate PRs for each commit, this PR is only used to show the combined performance impact.

I'm not following along with the new Version type, how do i pass in a pep440_rs version with the new type? How do i represent large versions?

Version is an index of the true version (semver::Version for cargo), and is limited to 63 versions so that VersionSet and Term can be represented as a u64 bitset, making intersection and union essentially free (this is the main improvement of this PR).

To use it, a dependency provider will need to store a list of all versions for a given package, which should be sorted by priority so that choose_version() can always choose the last Version of a VersionSet.

To allow for more than 63 versions of a package, virtual packages need to be defined to ensure package unicity and to allow more than 63 possible versions of a dependency:

Note that this complexifies version choosing, but we can avoid difficulties by sorting versions by priority in the dependency provider.

@x-hgg-x x-hgg-x force-pushed the perf branch 5 times, most recently from 0efe2d9 to ec45c54 Compare November 8, 2024 20:51
@mpizenberg
Copy link
Member

This is an impressive speedup. I don’t have the bandwidth personally to review this unfortunately so I’ll just trust the other pairs of eyes already on this. Few remarks, I agree with the others about the possibilities to split this between non-controversial changes and more controversial changes. But it seems you already agree with that too so all good.

Another broad remark, I feel like some of the speedups there rely on specific representations for versions if I understand correctly what you did with the u64 bitset. I’d just want to point out the fact that it’s important to keep the overall API generic so that version sets details of implementation can stay user-specific and not embedded directly into pubgrub assumptions.

Nothing else to add except thank you for all that work! and I’m excited to (sporadically) follow along.

@x-hgg-x
Copy link
Contributor Author

x-hgg-x commented Nov 8, 2024

Another broad remark, I feel like some of the speedups there rely on specific representations for versions if I understand correctly what you did with the u64 bitset.

Dependency providers can still internally use whatever version format the want, but with this PR Pubgrub only accepts discrete versions for a dependency in the get_dependencies() method, which are representing the indices of provider versions.

This means we need to list all versions of a dependency matching the requirement in the get_dependencies() method, then give Pubgrub the corresponding bitset.

The VersionSet trait is therefore not needed anymore since Pubgrub will do the intersections/unions on the bitset (instead of on the range corresponding to the provider version). This is why the API doesn't need to be generic anymore.

@x-hgg-x
Copy link
Contributor Author

x-hgg-x commented Nov 9, 2024

Version is an index of the true version

I will rename Version to VersionIndex to clarify this.

@x-hgg-x x-hgg-x force-pushed the perf branch 2 times, most recently from 064d143 to f2c6859 Compare November 12, 2024 12:58
Copy link

codspeed-hq bot commented Nov 12, 2024

CodSpeed Performance Report

Merging #274 will improve performances by 35.2%

Comparing x-hgg-x:perf (080a443) with dev (e9170cb)

Summary

⚡ 6 improvements

Benchmarks breakdown

Benchmark dev x-hgg-x:perf Change
backtracking_disjoint_versions 2,337.2 ms 370.4 ms ×6.3
backtracking_ranges 1,972.6 ms 63.4 ms ×31
backtracking_singletons 4,264.6 ms 774.5 ms ×5.5
large_case_u16_NumberVersion.ron 21.4 ms 15.8 ms +35.2%
sudoku-easy 3.8 ms 1.4 ms ×2.7
sudoku-hard 3.9 ms 1.3 ms ×3.1

@Eh2406
Copy link
Member

Eh2406 commented Nov 19, 2024

Those are very exciting numbers! What piece do you want to work on next? The first of your commits is 5834083 Move offline provider into a separate module which I've long wanted to do. So that is an ez +1. The next is eeb525b Use an arena for packages, which has a lot going on but I'm very supportive of the effort. #242 is my attempt to do something similar without breaking changes. If you could look that over I'd appreciate it. Specifically any optimizations from your work that should be included in that PR? Generally, I want to continue working for the non-breaking changes first.

@x-hgg-x
Copy link
Contributor Author

x-hgg-x commented Nov 20, 2024

Generally, I want to continue working for the non-breaking changes first.

The other non-breaking changes don't give any perf gain without the Use u64 for version set type commit since VersionSet and Term types are too big, so I will follow the commit order.

@x-hgg-x
Copy link
Contributor Author

x-hgg-x commented Nov 27, 2024

I added a method in the dependency provider to register a conflict when backtracking for better package prioritization, and it shows good results for solana-archiver-lib v1.1.12:

Before:

should cancel count: 7429904
backtracks: 8592
incompatibilities: 424401
packages: 6802
time for unit propagation: 1.138949313s
time for pick highest priority pkg: 574.226395ms

After:

should cancel count: 274934
backtracks: 8597
incompatibilities: 407651
packages: 6441
time for unit propagation: 143.769512ms
time for pick highest priority pkg: 62.437494ms

Conflict map:

(7212, "solana-sdk")
(1212, "solana-metrics")
(966, "solana-clap-utils")
(545, "ed25519-dalek")
(501, "solana-client")
(482, "solana-vote-program")
(404, "solana-version")
(398, "solana-perf")
(395, "solana-net-utils")
(384, "solana-runtime")
(369, "solana-config-program")
(359, "solana-account-decoder")
(353, "solana-measure")
(333, "solana-faucet")
(317, "solana-ledger")
(296, "solana-core")
(282, "solana-transaction-status")
(278, "cc")
(269, "solana-stake-program")
(267, "solana-remote-wallet")
(266, "solana-bpf-loader-program")
(247, "solana-sys-tuner")
(150, "solana-crate-features")
(68, "solana-program")
(64, "solana-exchange-program")
(63, "curve25519-dalek")
(52, "solana-vote-signer")
(50, "openssl-sys")
(32, "solana-budget-program")
(30, "solana-vest-program")
(21, "cxx")
(16, "spl-token")
(9, "ring")
(8, "regex-automata")
(4, "aho-corasick")
(3, "openssl")
(2, "solana-sdk-macro")
(1, "solana-genesis-programs")

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.

5 participants