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

when priorities are equal do breadth first search #299

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

Eh2406
Copy link
Member

@Eh2406 Eh2406 commented Dec 18, 2024

Now that #298 has landed, the next steps are evaluate additional heuristics on a case-by-case basis. The obvious next most important from #291 is discovery_order. Thinking about it, almost all users could use this as a tiebreaker, and it costs next to nothing to calculate and the storage cost in priorities seems minimal. Overall, the cost to adding it to the documentation and telling all of our users to add it to their boilerplate seems higher than just always including it in the priority.

Given that we would now have a reasonable default I thought about giving prioritize a default implementation. Unfortunately, it would need to return a Self::Priority and associated types cannot have a default value. :-(

cargo run -r -- -m pub --with-solana --filter solana-archiver-lib -t 16 is now giving 2.52min, a massive improvement from 7.56min with dev.

Copy link

codspeed-hq bot commented Dec 18, 2024

CodSpeed Performance Report

Merging #299 will degrade performances by 6.94%

Comparing Eh2406:discovery_order (bda2b81) with dev (d49bf5f)

Summary

❌ 1 regressions
✅ 5 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark dev Eh2406:discovery_order Change
sudoku-hard 4 ms 4.2 ms -6.94%

@konstin konstin added this pull request to the merge queue Dec 19, 2024
Merged via the queue into pubgrub-rs:dev with commit 3bef331 Dec 19, 2024
5 of 6 checks passed
konstin added a commit to astral-sh/uv that referenced this pull request Dec 19, 2024
uv gives priorities to packages by package name, not by virtual package (`PubGrubPackage`). pubgrub otoh when prioritizing order the virtual packages. When the order of virtual packages changes, uv changes its resolutions and error messages. This means uv was depending on implementation details of pubgrub's prioritization caching.

This broke with pubgrub-rs/pubgrub#299, which added a tiebreaker term that made pubgrub's sorting deterministic given a deterministic ordering of allocating the packages (which happens the first time pubgrub sees a package).

The new custom tiebreaker decreases the difference to upstream pubgrub.
konstin added a commit to astral-sh/uv that referenced this pull request Dec 19, 2024
uv gives priorities to packages by package name, not by virtual package (`PubGrubPackage`). pubgrub otoh when prioritizing order the virtual packages. When the order of virtual packages changes, uv changes its resolutions and error messages. This means uv was depending on implementation details of pubgrub's prioritization caching.

This broke with pubgrub-rs/pubgrub#299, which added a tiebreaker term that made pubgrub's sorting deterministic given a deterministic ordering of allocating the packages (which happens the first time pubgrub sees a package).

The new custom tiebreaker decreases the difference to upstream pubgrub.
@Eh2406 Eh2406 deleted the discovery_order branch December 19, 2024 15:10
konstin added a commit to astral-sh/uv that referenced this pull request Dec 19, 2024
uv gives priorities to packages by package name, not by virtual package (`PubGrubPackage`). pubgrub otoh when prioritizing order the virtual packages. When the order of virtual packages changes, uv changes its resolutions and error messages. This means uv was depending on implementation details of pubgrub's prioritization caching.

This broke with pubgrub-rs/pubgrub#299, which added a tiebreaker term that made pubgrub's sorting deterministic given a deterministic ordering of allocating the packages (which happens the first time pubgrub sees a package).

The new custom tiebreaker decreases the difference to upstream pubgrub.
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.

2 participants