search

pubgrub-rs / pubgrub

PubGrub version solving algorithm implemented in Rust
https://pubgrub-rs.github.io/pubgrub/pubgrub/
Mozilla Public License 2.0
365 stars 34 forks source link

refactor: satisfier search in a helper method #97

Closed Eh2406 closed 3 years ago

Eh2406 commented 3 years ago

This builds on #96 and is intended to be merged after it.

This pulls out the shared code between find_satisfier and find_previous_satisfier into a new method called satisfier. Yes, this is reviving the find_satisfier_helper that I removed in #79. But thanks to @mpizenberg work in #92 we have a PackageAssignments where it fits much more naturally.

I don't know for certain if it finds the same previous_satisfier in all the corner cases, but test pass. 🤷 I can try to be more careful about that if you want.

I did not do perf runs as my desktop needs some maintenance and I don't know when the replacement part will be available.

aleksator commented 3 years ago

Looks cleaner! I run NumberVersion benchmark and saw no changes in performance with the latest commit and without:

Benchmarking large_cases/large_case_u16_NumberVersion.ron: Analyzing
large_cases/large_case_u16_NumberVersion.ron
                        time:   [10.390 ms 10.417 ms 10.445 ms]
                        change: [-0.4907% -0.1013% +0.2715%] (p = 0.62 > 0.05)
                        No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild
  6 (6.00%) high severe

I don't have other benchmarks mentioned on Zulip though (zuse?), where can I get them?

Eh2406 commented 3 years ago

elm (some renaming requered) and zuse

Eh2406 commented 3 years ago

I merged, when we find a case where we want different behavior we can revert then.

Eh2406 commented 3 weeks ago

I don't know for certain if it finds the same previous_satisfier in all the corner cases, but test pass. 🤷 I can try to be more careful about that if you want.

I finally did the due diligence in the first commit of https://github.com/Eh2406/pubgrub/pull/new/check_97_find_previous_satisfier yes. It does always retrieve the same result. the second commit pulls in the small change discussed https://github.com/pubgrub-rs/pubgrub/pull/79#issuecomment-2329782596 to maintain the behavior from earlier versions, the change to the new code is equally small.