pkg: don't expand deps of restricting deps #11264
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gridbugs
requested review from
Leonidas-from-XIV,
rgrinberg,
maiste and
ElectreAAS
rgrinberg
reviewed
Jan 6, 2025
|
@talex5 do you mind doing a sanity check on this? |
|
Is it possible to add a test to demonstrate a change in behavior? Since this change is loading opam files, I imagine that it should avoid raising an error when one of these opam files has an error. |
This change sounds reasonable to me. |
|
Upstreaming would be good. It would likely make the solver service a fair bit faster. |
Previously dune would expand dependencies of packages that are only ever marked as conflicting with packages in the solution. This led to the expansion of far more packages that necessary. Expanding a package requires reading an opam file and so is a relatively expensive operation. For packages that only depend on the compiler (ocaml-base-compiler), this reduces the number of expanded packages from 8376 to 1841 at the time of writing. Signed-off-by: Stephen Sherratt <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
rgrinberg
force-pushed
the
do-not-expand-conflict-only-packages
branch
from
1a64582 to
8a12dfc
Compare
rgrinberg
approved these changes
Jan 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously dune would expand dependencies of packages that are only ever marked as conflicting with packages in the solution. This led to the expansion of far more packages that necessary. Expanding a package requires reading an opam file and so is a relatively expensive operation.
For packages that only depend on the compiler (ocaml-base-compiler), this reduces the number of expanded packages from 8376 to 1841 at the time of writing.
Consider this to be a proof of concept demonstrating the problem and a potential solution. "Better" solutions might avoid the additional mutable state I've added here but would probably be more invasive. Since we've discussed eventually rewriting the solver anyway, I'm happy for this to be merged if my approach is deemed acceptable.
Here are the performance stats (using this feature) before and after this change on my machine (also including #11254 as it removes a large constant factor (technically
O(num-pkgs-in-repo)) from the solver runtime). The project being locked has a single dependency onocaml-base-compiler.