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

Add pre-processing to coinstallability checks #5024

Merged
merged 6 commits into from
Feb 22, 2022
Merged

Conversation

AltGr
Copy link
Member

@AltGr AltGr commented Jan 25, 2022

Addresses some remaining costly cases in #4311

The patch includes a small reorganisation of OpamSolver, but the general
idea is to fix the performance regression compared to 2.0:

  • with the introduction of solver invariants, the pre-processing that trimmed
    packages conflicting with the base in OpamState was removed
  • it was replaced by something much more general (and reliable) at the
    OpamCudf level
  • but only for calls to the external solver, until now

NOTE: this enforces the invariant even for opam install --coinstallable-with, which is consistent with 2.0 but had changed in 2.1.
Without it we can't really expect reasonable performance in general anyway.

@AltGr
Copy link
Member Author

AltGr commented Jan 25, 2022

Test case:

opam list -s --depends-on minios-xen.0.7 --coinstallable-with minios-xen.0.7 --rec

from 5+ minutes on master to 12s here

@AltGr
Copy link
Member Author

AltGr commented Jan 25, 2022

You can start reviewing, but hold on a little more before merging, I found a bug. Shouldn't be anything big.

@AltGr
Copy link
Member Author

AltGr commented Jan 26, 2022

Fixed!

@AltGr
Copy link
Member Author

AltGr commented Jan 26, 2022

Squeezed in a one-line fix for #4135

@@ -230,6 +230,47 @@ rankers.1.0.0
rankers.2.0.1
rankers.2.0.7
### opam list --depends-on lacaml -s --all-versions --coinstallable-with phylogenetics.x
Copy link
Member Author

Choose a reason for hiding this comment

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

coinstallability with a non-existent version used to return nothing. Now it returns what would be compatible with that version, which seems more useful

@@ -261,11 +302,6 @@ Done.
gpr.1.5.0
phylogenetics.0.0.0
### opam list --depends-on lacaml -s --all-versions --coinstallable-with lbfgs.0.8.8 gpr phylogenetics
Copy link
Member Author

Choose a reason for hiding this comment

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

coinstallable now implies installable (like in 2.0). The intersection was empty, so empty results here.

@AltGr
Copy link
Member Author

AltGr commented Feb 2, 2022

Squeezed in a one-line fix for #4135

Actually dropped that, it wasn't needed ; I will comment further on #4135

See the actual diff on opam list outputs here

@rjbou rjbou added this to PR in progress in Opam 2.2.0 via automation Feb 2, 2022
@rjbou rjbou added this to the 2.2.0~alpha milestone Feb 2, 2022
@rjbou rjbou linked an issue Feb 2, 2022 that may be closed by this pull request
@rjbou rjbou removed a link to an issue Feb 2, 2022
@rjbou rjbou linked an issue Feb 2, 2022 that may be closed by this pull request
@gasche
Copy link
Member

gasche commented Feb 3, 2022

@Armael and myself hit performances issues in OpamSolver.installable when re-running Marracheck after rebasing on the opam master (last time was a while ago, we were working with something 2.0-ish). We call it to get the set of installable packages in a switch with just a compiler installed. The call would previously take seconds (we don't know how much exactly, but it was fast enough that we didn't even bother to log it) and it now takes around seven minutes on my machine.

We warmly welcome progress on this :-)

@Armael
Copy link
Member

Armael commented Feb 15, 2022

FWIW, in marracheck we run OpamSolver.installable on a universe corresponding to a freshly installed switch (containing only the base compiler).
On opam/master this takes 14 minutes (!) on my machine; with this PR it does not seem to be much faster and takes 13 minutes.

@kit-ty-kate
Copy link
Member

@Armael what is your switch invariant in your use-case?

@Armael
Copy link
Member

Armael commented Feb 15, 2022

I have invariant: [ "ocaml-base-compiler" {= "4.13.1"} ] (in .opam/theswitch/.opam-switch/switch-config)

@rjbou rjbou moved this from PR in progress to PR to review in Opam 2.2.0 Feb 18, 2022
@rjbou
Copy link
Collaborator

rjbou commented Feb 18, 2022

note from dev meeting: installable is not directly impacted by this PR (coinstallable is) . Better merge and see how it's going for this one.

@AltGr
Copy link
Member Author

AltGr commented Feb 18, 2022

Indeed, this reimplements OpamSolver.installable_subset but doesn't change OpamSolver.installable; I assume the latter could be implemented from the former, but I didn't change it right away to go incrementally (they are bound to different Dose functions underneath, but these should be semantically equivalent).

So it's normal that you don't get a speedup yet; as a benchmark you could try installable_subset with all packages as argument, where the switch invariant will be used for trimming and you should benefit from the optimisation already.

@rjbou rjbou added PR: QUEUED Pending pull request, waiting for other work to be merged or closed and removed PR: QUEUED Pending pull request, waiting for other work to be merged or closed labels Feb 22, 2022
AltGr and others added 5 commits February 22, 2022 11:15
Fix list test on 32bit archs

Fix 'list' test for Windows
Addresses some remaining costly cases in ocaml#4311

The patch includes a small reorganisation of `OpamSolver`, but the general idea
is to fix the performance regression compared to 2.0:

- with the introduction of solver invariants, the pre-processing that trimmed
  packages conflicting with the base in `OpamState` was removed
- it was replaced by something much more general (and reliable) at the
  `OpamCudf` level
- but only for calls to the external solver, until now

NOTE: this enforces the invariant even for `opam install --coinstallable-with`,
which is consistent with 2.0 but had changed in 2.1. Without it we can't really
expect reasonable performance in general anyway.
Just run `opam list --installable` without further argument. Not adding a test
because it would be too big in time and space.
@rjbou rjbou moved this from PR to review to PR finalised (merge with CI) in Opam 2.2.0 Feb 22, 2022
@rjbou rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Feb 22, 2022
@rjbou rjbou merged commit 6555a0c into ocaml:master Feb 22, 2022
Opam 2.2.0 automation moved this from PR finalised (merge with CI) to Done Feb 22, 2022
rjbou added a commit to rjbou/opam that referenced this pull request Apr 27, 2022
Add pre-processing to coinstallability checks
rjbou added a commit to rjbou/opam that referenced this pull request Jul 12, 2022
Add pre-processing to coinstallability checks
@rjbou rjbou added this to PR in Progress in Opam 2.1.x via automation Jul 4, 2023
@rjbou rjbou modified the milestones: 2.2.0~alpha, 2.1.3 Jul 4, 2023
@rjbou rjbou moved this from PR in Progress to Done in Opam 2.1.x Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Opam 2.1.x
  
Done
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

[opam 2.1~alpha3] opam list --installable is really slow
5 participants