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

Consider already selected packages during solve #1406

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonjohnsonjr
Copy link
Contributor

@jonjohnsonjr jonjohnsonjr commented Nov 15, 2024

We ran into a very very slow solve. I can't tell if it was actually slow
or infinite, but it didn't seem to be convering very quickly. I realized
that we essentially recurse the entire graph for every constraint, even
for things we've already solved. This was surprising, because I thought
we handled that already, but it seems like we didn't.

I've added an example "abseil-regression.yaml" that took over a second
to solve (as of this writing) before this change and takes about 1ms
after this change. There's an open PR in wolfi that timed out because
the abseil dep graph got even more gnarly and took several hours, so
there was something (at least) quadratic going on here, oops.

I've deleted a test that was exercising a scenario where packages were
disallowed from providing themselves because there is no code path where
that was actually possible (all invocations had "true" hard-coded).

I also ran this against a several thousand image configs and they all
produced the same results before and after this change, so I am fairly
confident in it.

@jonjohnsonjr
Copy link
Contributor Author

e2e tests are failing because a lot of our examples use alpine repos, and currently musl on x86_64 is at 1.2.5-r6 whereas musl on aarch64 is at 1.2.5-r5

We ran into a very very slow solve. I can't tell if it was actually slow
or infinite, but it didn't seem to be convering very quickly. I realized
that we essentially recurse the entire graph for every constraint, even
for things we've already solved. This was surprising, because I thought
we handled that already, but it seems like we didn't.

I've added an example "abseil-regression.yaml" that took over a second
to solve (as of this writing) before this change and takes about 1ms
after this change. There's an open PR in wolfi that timed out because
the abseil dep graph got even more gnarly and took several hours, so
there was something (at least) quadratic going on here, oops.

I've deleted a test that was exercising a scenario where packages were
disallowed from providing themselves because there is no code path where
that was actually possible (all invocations had "true" hard-coded).

I also ran this against a several thousand image configs and they all
produced the same results before and after this change, so I am fairly
confident in it.

Signed-off-by: Jon Johnson <[email protected]>
@jonjohnsonjr jonjohnsonjr marked this pull request as ready for review November 16, 2024 21:32
@jonjohnsonjr jonjohnsonjr force-pushed the split branch 2 times, most recently from d63dbd3 to b787510 Compare November 16, 2024 21:55
Alpine doesn't keep their archs in sync and only keeps around the latest
revision in edge, which breaks a bunch of our test, so I've cut down the
alpine tests to a single arch.

Signed-off-by: Jon Johnson <[email protected]>
@dannf
Copy link

dannf commented Nov 18, 2024

I've tested this branch and it fixes the issue for me. Thanks! Do you mind referencing chainguard-dev/melange#1651 in the commit log?

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