Don't include more bundles if the requirement is already fulfilled#1618
Don't include more bundles if the requirement is already fulfilled#1618laeubi wants to merge 2 commits intoeclipse-pde:masterfrom
Conversation
Test Results 273 files - 12 273 suites - 12 31m 22s ⏱️ - 14m 41s For more details on these failures, see this check. Results for commit 197ac64. ± Comparison against base commit 98a5134. This pull request removes 2650 tests.♻️ This comment has been updated with latest results. |
HannesWell
left a comment
HannesWell
left a comment
There was a problem hiding this comment.
The problem that a product only contains a sub-set of all bundles in the TP and that therefore the wiring can be different than in the TP is something I thought about a while ago.
While filtering out already provided capabilities as you suggested should have the desired effect (since it's performing a breath-first-search through the complete wiring to compute the closure), I'm not sure if there are cases where just skipping capabilities and thus bundles could break something (plus we have the potential performance problem described below).
When I thought about this general problem, the solution alternative solution I found was to create a new state/wiring resolution for the launch where the bundles explicitly listed in the product (directly or through feature containments) are the 'roots' and are therefore preferred by the resolver. But I havn't checked if it's possible to define such 'roots' or maybe adding them first is already sufficient?
@HannesWell something for RC1?
Better not, as this area has been shown to contain surprises in the past that one doesn't really expect.
| for (BundleCapability bundleCapability : list) { | ||
| if (requirement.matches(bundleCapability)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This will increase the runtime characteristics of the collection of dependencies by one or two orders (depending on the reference) as for each single requirement the entire namespace is searched again. And namespaces like osgi.wiring.package or osgi.wiring.bundle the namespace is huge, basically all packages or bundles available reside their and it would be checked with a linear search for each single-requirement.
To mitigate that this check should either exclude certain known large namespaces like package and bundle that we are usually not interested anyways (ok, well excluding duplicates could also be interesting for bundles/packages).
Or by introducing a second look-up level that includes the bundle/package name, but I think that's not trivial just on general requirements/capabilities as one has to decompose the filters.
There was a problem hiding this comment.
I don't think that skipping anything is possible here and yes the problem is "hard" but doing a full resolve could even be harder.
We do similar thing on Tycho and even for very large sets of metadata this has never shown any performance problem as this are very simple comparisons.
I can't think about it right now, but the users always has the chance to influence "the roots" (but can't exclude things what leads to the problem described here). Basically this is how Tycho/P2 works when building a product, first include the roots and then everything what is required from there (just that it uses deep first if I rember correctly).
This is what actually is the resolver service of OSGi for, but don't expect it to be more efficient, I use such think in the PDE>BND integration in Tycho already so it is possible. |
I think that's what we should do in PDE as well to get a clean new state for a launch. |
|
I have major challenges with EMF's development workspace because the current helpful-but-aggressive algorithms drags in RAP dependencies because package imports are resolved/wired to both the real SWT and RAP's RWT. Not only is this not needed, but in fact this makes the self-hosted launch 100% unusable. Working around this is super difficult to specify and maintain (long lists of bundles) as opposed to the current approach of listing a smaller number of features and automatically what's needed is included. With this PR it works well again for EMF. As for performance I see it indeed doing a great many linear matches through some very long lists: The overall time for I don't see a good way to make this perform much better via the API which hide important details about the requirement, e.g., we can't know anything about org.eclipse.osgi.internal.resolver.GenericSpecificationImpl.matchingFilter or the name attribute that will be matched:
If we knew the name being matched we could build an index on the name... Without this logic, I get performance
|
|
I made changes to this version to build an index using information extracted via the APIs: The performance is then |
|
Thanks Ed for having a look, too. I also had a look at this, respectively my suggestion to create a new state were the explicit content is preferred, and noticed that the launch validation already creates a new state.So I think that wouldn't take too long, at least I never noticed that. Furthermore I noticed that the |
|
M2 is next week so I'm concerned that if this doesn't move forward that we will miss this release cycle due to riskiness. Whatever we change, we should ensure it's tested out in the wild sooner rather than later. I don't really have a clue of what you think would be a better alternative implementation so I cannot help with that. |
|
Thanks for the heads-up.
I have just create a PR with my alternative solution: #1725 |
Currently PDE includes everything wired in the target platform even though a requirement might already be fulfilled and only has single cardinality. This now tracks all capabilities provided and check if a single cardinality is already fulfilled by some of the bundles chosen.
|
I'm now reusing a class from Equinox for the index. I'll prepare a PR with the adjustments as well. Given that using the resolver will not likely be faster and is currently having issues, I would propose we merge this one for M2 and maybe improve / enhance / replace it with something else later on. |
|
Testing this with my EMF workspace I have this one problem:
Which leads to real problems in the log: This problem does not happen with the *.txt version I attached above:
So something is not quite greedy enough... |
|
If I read this correct, it appears that the requirement is ignored because something believes that it's coming from the system bundle:
|
|
The system bundle exports the union of all EEs:
So once the system bundle is added to the capabilities, then many javax things that were removed from the JDK to jakarta will become problematic. Avoiding to add the system bundle to the capabilities avoids this problem. Yes, that might lead to bundles being included that are not strictly needed because the JDK provides the required package. If we properly populated the state with the actual EE, we'd not need this, but that's a much bigger design change than simply being a bit more pessimistic here. |
What issues do you mean #1725 has? You have mentioned some points that are not ideal, but I think that has to be accepted and in my testing it worked as desired. But it would be nice if @merks would confirm that as well. The fundamental problems I see with this approach still exists:
|
|
I tested #1725 and it works well for my EMF scenario... |
You previously wrote
so I assumed it is/was not ready yet.
All callers of
If we ignore the copied code from Equinox, it is 30 simple lines versus 70 lines of highly complex lamda and streaming so I doubt 'less complex' is really true here anyways my main goal is that we get some solution to the problem. It was initially meant to be merged/fixed as soon as master opens, now we have already missed M1 and maybe even will miss M2 as well. |
|
Let's not miss M2! |
That was referring to the very first state of that PR two weeks ago and was fixed soon afterwards.
That statement is not true. They get the correct result according to their current wiring. But as the wiring state is not necessarily unambiguous different solutions are possible. In the general case of the TP-state there is simply not a preference for a specific solution(yet, theoretically we could apply a similar schema when creating the TP-state based on their definition). But in case of a launch the already included bundles are (kind of) preferred, as other providers are (maybe) not necessary and could potentially lead to problems.
From my POV #1725 (comment) is ready, maybe you want to give it a try :) |
|
Then lets close this and merge the other PR asap! |
3 participants
Currently PDE includes everything wired in the target platform even though a requirement might already be fulfilled and only has single cardinality.
This now tracks all capabilities provided and check if a single cardinality is already fulfilled by some of the bundles chosen.
Fix #1604
@HannesWell something for RC1?