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

refactor: satisfier search in a helper method #97

Merged
merged 1 commit into from
Jun 14, 2021
Merged

refactor: satisfier search in a helper method #97

merged 1 commit into from
Jun 14, 2021

Conversation

Eh2406
Copy link
Member

@Eh2406 Eh2406 commented Jun 11, 2021

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
Copy link
Member

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
Copy link
Member Author

Eh2406 commented Jun 14, 2021

Copy link
Member

@mpizenberg mpizenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So by extracting the code looking for a given package satisfier and reusing it when looking for a previous statisfier (though with a different starter accumulated intersection), we fully embrace the behavior consisting of returning the same decision level for previous satisfier than for satisfier when there is no previous satisfier. As discussed in zulip, this case corresponds to a situation when an incompatibility is satisfied by the first time that package appears in a dependency. And by returning the same decision level as the one of the satisfier, we branch in the code that looks for the decision leading to the package with that dependency, use the rule of resolution to prevent that from happening again and continue on with conflict resolution.

To be clear, this is the same behavior as we already chose, just with this change it makes it slightly more complex to behave differently if we want.

Regarding performances, I think it should be very similar since the resolution path taken is the same. It might just compute one more term inclusion in a corner case (when previous derivations slice would have been empty in the previous code) but I think it's fine.

@Eh2406 Eh2406 merged commit 2ca8446 into dev Jun 14, 2021
@Eh2406 Eh2406 deleted the satisfier branch June 14, 2021 15:40
@Eh2406
Copy link
Member Author

Eh2406 commented Jun 14, 2021

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

@Eh2406
Copy link
Member Author

Eh2406 commented Sep 4, 2024

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 #79 (comment) to maintain the behavior from earlier versions, the change to the new code is equally small.

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