Add system property to switch adding transitive dependencies on/off#2253
Conversation
laeubi
left a comment
laeubi
left a comment
There was a problem hiding this comment.
In general I think it won't harm to have such property - even though I would strongly discourage to use it or see it as a last resort and suggest to add a strong comment that this property might be removed, renamed, ... any time without notice (even though maybe unlikely) if we want to change something in the future.
There was a problem hiding this comment.
In general I think it won't harm to have such property - even though I would strongly discourage to use it or see it as a last resort and suggest to add a strong comment that this property might be removed, renamed, ... any time without notice (even though maybe unlikely) if we want to change something in the future.
This is fine for me, with that extension/context.
...rg.eclipse.pde.core/src/org/eclipse/pde/internal/core/RequiredPluginsClasspathContainer.java
Outdated
Show resolved
Hide resolved
This change allows users to control whether the transitive dependencies should be recursively added to the classpath by providing boolean system property `pde.addTransitiveDependenciesWithForbiddenAccess`. This allows to mitigate possible functional or performance regressions of the 89b00be / eclipse-pde#2218 which changed how PDE resolves classpath for bundle projects. By default the property is considered to be "true". See eclipse-pde#2244
iloveeclipse
force-pushed
the
issue_2244_switch
branch
from
99f3f7f to
3d43f9c
Compare
...rg.eclipse.pde.core/src/org/eclipse/pde/internal/core/RequiredPluginsClasspathContainer.java
Show resolved
Hide resolved
Test Results 801 files + 662 801 suites +662 59m 16s ⏱️ + 22m 59s Results for commit 3d43f9c. ± Comparison against base commit 3b4269f. This pull request removes 2202 and adds 2507 tests. Note that renamed tests count towards both.This pull request removes 24 skipped tests and adds 24 skipped tests. Note that renamed tests count towards both. |
|
Build & tests are fine, there are 42 maven console warnings that something can't be validated, like this one: These maven warnings & failed license check caused by currently unstable infra and can be ignored. |
|
Thank you @iloveeclipse for proposing this PR and thank you all for addressing this issue. FWIW, I know that performance kind of took the back seat in this conversation (#2244) but for us it is not a secondary issue. Regardless of whether or not #2218 is the right fix, our DEV team will have to face the consequences (performance impact) in the upcoming release and for us this means a 44%-65% degradation in the full build time. I will update the IDE in the coming days and measure again deactivating #2218. I'll report back. |
If you look at the conversation you will see that actually a JDT bug has caused that (previously undiscovered) - and it was even tried by multiple users and they have not seen any (noticeable) performance impact. So if anyone can show a setup with a reproducible case one can look into this otherwise it is simply impossible to judge what would causing this. Especially I can't buy the argument in case of full build... that makes no sense because:
So whatever causes this is likely already present but not visible and might be trigger by any arbitrary dependency added to your bundles and now become more prominent. Apart from that I try to optimize performance without degrade functionality is of course always welcome... and as you seem to have a case easily reproducible it would be good to see where/why actually the performance comes from. |
|
I tested again today and used yesterday's I-BUILD: I20260313-0340. Here are the results
To disable the transitive dependencies I used @iloveeclipse 's flag: VisualVM Samples |
|
Comparing today's results to my results from Wednesday using I20260310-1800 (where neither eclipse-jdt/eclipse.jdt.core#4922 nor the flag from this PR are present)... Using I20260310-1800
Having eclipse-jdt/eclipse.jdt.core#4922 doesn't seem to make a difference. |
It is a today's build :-)
Let move this discussion to #2244. |
Thanks for sharing these measures, I'll take a look. |
Thank you very much! I added this information and more (the measurements for 2026-03 which I use as "basis") in #2244 (comment) as per requested by Andrey :-) |
|
@iloveeclipse please add to the N&N document, so that clients which are affected, can use this new property |
|
FYI - #2251 should also speed up the classpath computation ( but it doesn't address the core issue, that JDT's incremental builder is slower because it sees far more classpath entries) |
This property is a temporary workaround to allow debugging/performance testing and is not intended as a permanent features. Users should not choose between correctness and performance. Instead everybody interested in performance should help to work of the list of identified issues from: |
For clients who already experience long compilation times, it would be beneficial to provide an option to run the system as it has worked for the past 20 years. As I understand it, the bug fix primarily addressed the “Cleanup MANIFEST” option, which, if I recall correctly, never worked reliably. Therefore, most clients are unlikely to miss it. It is good that PDE is improving, but the runtime increases described in the tickets from @laeubi are concerning. If these performance regressions persist, many of my clients will likely hesitate to migrate to newer Eclipse IDE versions. For example, one report mentions a 6.1× slowdown: |
Even though it was already discussed at length you are exchange cause and effect. Because of the previous wrong behavior (that only worked by chance because JDT is lenient when resolving types, but javac is not and so would have failed "since 20 years") different JDT functions where broken or degraded (e.g. code navigation, autocompletion) or produced hard to understand and fix errors (e.g. incomplete type hierarchy or missing type error) that even depend on what methods one use or if you extend a class or implement an interface with a hierarchy spanning more than one bundle. Because of this people where forced to add new imports/requires that are strictly not necessary to their bundles manifest to make compiler happy. The “Cleanup MANIFEST” then correctly identifies these as unused but if you are removing them the compiler jerks out so it feel like the function is broken while it was not. So as of today I'm not aware of any further cases where it fails on a broader range (there might still be some quite corner cases we not have discovered yet). Regarding the performance, it seems that in some circumstance, where no one yet could provide a public reproducer, it can give varying results. We don't see this with the platform workspace (what is already quite big and convoluted) and neither with a even larger workspace from @merks that contains even more. The referenced measures here where taken by @fedejeanne so a very special setup that already was very slow before (12 min full clean/build time) but e.g. we have no measures about incremental build performance (what is often more important as we usually not doing full builds all day). In any case it just amplifies generic performance issues we should tackle and that would improve performance generally for all user (not only PDE). I already created one PR and plan to do more especially on the smaller ones, but until now it does not get much attention from any JDT developer yet. So if you are specifically concerned about eclipse-jdt/eclipse.jdt.core#4927 (fix is now here eclipse-jdt/eclipse.jdt.core#4939) I can only suggest to work on such things and propose a fix (as already mentioned by @HannesWell), each ticket already contains some possible mitigation strategies so one only need to apply and verify it. What then would maybe lead to even faster builds than before.
Of course not the whole build is 6x slower but there is a 6x slowdown in this particular compared example for this specific code call site and likely also biased by the profiling overhead - so no need for drama. |
5 participants
This change allows users to control whether the transitive dependencies should be recursively added to the classpath by providing boolean system property
pde.addTransitiveDependenciesWithForbiddenAccess.This allows to mitigate possible functional or performance regressions of the 89b00be /
#2218 which changed how PDE resolves classpath for bundle projects.
By default the property is considered to be "true".
See #2244