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

Use namespace, directives and attributes to compare equal of Requirement #538

Closed
wants to merge 1 commit into from

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Feb 22, 2024

Currently the resolver is ultra slow in some scenarios and even never finds a valid solution, just adding more and more permutations to check.

The reason is that one check that should actually reduce the number of problematic permutations fails because it uses Requirement#equals what is incapable of compare requirements across Resource boundaries, that means even if the requirement describes the same restrictions it is not considered equal and therefore selected as a possible permutation to remove this, then the validation check detects this is unresolvable adding possible new permutations and so on.

This now performs an equals check instead on namespace, directives and attributes (what can be disabled by a system property) what makes the resolver in such case from running forever to just an blink of an eye.

@tjwatson this dramatically reduces resolving time and leads to a resolved state where previously confusing use constraint violation and missing packages are reported that are actually resolvable. For my example described here:

it goes from minutes (until it hits the timeout) to milliseconds, I therefore suggest to include this in RC1, I added a system property as last resort if this would break something badly, so please review and give +1 as PL, for the record I would also give +1 as PL but would want your vote and review as binding here!

FYI @tivervac

Currently the resolver is ultra slow in some scenarios and even never
finds a valid solution, just adding more and more permutations to check.

The reason is that one check that should actually *reduce* the number of
problematic permutations fails because it uses Requirement#equals what
is incapable of compare requirements across Resource boundaries, that
means even if the requirement describes the same restrictions it is not
considered equal and therefore selected as a possible permutation to
remove this, then the validation check detects this is unresolvable
adding possible new permutations and so on.

This now performs an equals check instead on namespace, directives and
attributes (what can be disabled by a system property) what makes the
resolver in such case from running forever to just an blink of an eye.
@laeubi
Copy link
Member Author

laeubi commented Feb 22, 2024

In my example there is the following requirements from two bundles that are using substitution packages:

osgi.wiring.package; filter:="(osgi.wiring.package=org.eclipse.lsp4j.jsonrpc)"
osgi.wiring.package; filter:="(osgi.wiring.package=org.eclipse.lsp4j.jsonrpc)"

their toString looks equals but if you look into the object you will see that one requirement is from

osgi.identity; osgi.identity="org.eclipse.lsp4j.jsonrpc"; type="osgi.bundle"; version:Version="0.19.0.v20221118-0359"

and the other from

osgi.identity; osgi.identity="org.eclipse.lsp4j.jsonrpc"; type="osgi.bundle"; version:Version="0.21.1.v20230829-0012"

so using equals compare them as false resulting in (the drop of lines is where the resolver gives up after one minute):

grafik

after apply they fix this results in the following:

grafik

Copy link

github-actions bot commented Feb 22, 2024

Test Results

   27 files  + 1     27 suites  +1   10m 55s ⏱️ -34s
2 170 tests ± 0  2 124 ✅ ± 0  46 💤 ±0  0 ❌ ±0 
2 228 runs  +14  2 182 ✅ +14  46 💤 ±0  0 ❌ ±0 

Results for commit 413aaa0. ± Comparison against base commit 906f6d6.

♻️ This comment has been updated with latest results.

@laeubi laeubi requested a review from tjwatson February 22, 2024 13:36
@stbischof
Copy link
Contributor

@timothyjward fyi

@@ -1260,6 +1287,25 @@ public boolean canRemoveCandidate(Requirement req)
return false;
}

private boolean isMatchingRequirement(Requirement requirement, Requirement other) {
if (USE_REQUIREMENTS_EQUALS) {
Copy link
Contributor

@stbischof stbischof Feb 22, 2024

Choose a reason for hiding this comment

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

you may want to document this fallback somewhere or name it *_FALLBACK

@tjwatson
Copy link
Contributor

so using equals compare them as false resulting in (the drop of lines is where the resolver gives up after one minute):

But they are two different requirements because they are from two different bundles.

@laeubi laeubi marked this pull request as draft February 22, 2024 15:43
@laeubi
Copy link
Member Author

laeubi commented Feb 22, 2024

Convert to draft as it does not seem to solve the issue in all cases...

@laeubi
Copy link
Member Author

laeubi commented Feb 23, 2024

@tjwatson I agree that this seems not the correct way to address the issue, I'll closing this for now and try to came up with an improved solution. This has most probably be detected in the inital collection of candidates so the resolver can't run into a conflict.

@laeubi laeubi closed this Feb 23, 2024
@laeubi
Copy link
Member Author

laeubi commented Feb 24, 2024

I have now pushed some experiments to the PR that contains the testcase(s) that show the explosion of permutations here:

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