Fix API tools slow down with multiple source folders sharing output #2202
Conversation
The fix in commit 215a618 for issue eclipse-pde#2096 introduced an O(n^2) time complexity regression when projects have multiple source folders sharing the same output location (like SWT with 26 source roots). The problem was that each source folder caused a new CompositeApiTypeContainer to be created, wrapping all previous containers plus a new one. This resulted in exponential duplication of container visits during API analysis. The fix modifies ProjectTypeContainer to support multiple IPackageFragmentRoots, allowing a single container to discover packages from all source folders that share the same output location. This avoids creating composite containers and maintains O(n) complexity. Fixes eclipse-pde#2197 Signed-off-by: moaead <moaead@users.noreply.github.com>
|
I checked out the code @moaead originally wrote before the PR disappeared and it looks good to me - the SWT build time has returned to normal (i.e. quite fast) with it. The test that @laeubi originally wrote for 215a618 has been updated and it passes. However I am hoping that @laeubi can review this to make sure that the test itself still addresses the issue raised in #2096 |
There was a problem hiding this comment.
Pull request overview
This pull request fixes an O(n²) performance regression introduced in a previous fix for issue #2096. The regression occurred when projects had multiple source folders sharing the same output location (e.g., SWT with 26 source roots), where each source folder caused creation of a new CompositeApiTypeContainer wrapping all previous containers, leading to exponential duplication.
Changes:
- Modified
ProjectTypeContainerto support multipleIPackageFragmentRootobjects using a list instead of a single field - Added
addPackageFragmentRoot()method to accumulate package fragment roots for shared output locations - Replaced composite container creation logic in
ProjectComponentwith calls toaddPackageFragmentRoot()
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ProjectTypeContainer.java | Converted from single IPackageFragmentRoot field to CopyOnWriteArrayList, added addPackageFragmentRoot method, updated getPackageNames to iterate all roots |
| ProjectComponent.java | Replaced CompositeApiTypeContainer creation logic with calls to addPackageFragmentRoot on existing ProjectTypeContainer instances |
| ProjectTypeContainerTests.java | Updated test expectations from COMPOSITE to FOLDER container type with explanatory comments |
Comments suppressed due to low confidence (1)
apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/model/ProjectTypeContainer.java:188
- The lazy initialization pattern for fPackageNames lacks proper synchronization, which could lead to race conditions when combined with the cache invalidation in addPackageFragmentRoot. If getPackageNames is called concurrently with addPackageFragmentRoot, or if multiple threads call getPackageNames simultaneously, the cache could be computed multiple times or a stale cache could be used. Consider adding synchronized blocks similar to the pattern used in AbstractApiTypeContainer.getApiTypeContainers() (lines 180-186 of that class), or making fPackageNames volatile and using double-checked locking.
public String[] getPackageNames() throws CoreException {
if (fPackageNames == null) {
SortedSet<String> names = new TreeSet<>();
for (IPackageFragmentRoot root : fPackageFragmentRoots) {
if (root.exists()) {
collectPackageNames(names, root);
}
}
fPackageNames = names.toArray(String[]::new);
}
return fPackageNames;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (root != null && !fPackageFragmentRoots.contains(root)) { | ||
| fPackageFragmentRoots.add(root); | ||
| // Clear cached package names so they will be recomputed | ||
| fPackageNames = null; |
There was a problem hiding this comment.
Setting fPackageNames to null without synchronization could cause visibility issues across threads and race conditions with concurrent reads in getPackageNames(). Consider using synchronized block or making fPackageNames volatile to ensure thread-safe cache invalidation.
The bad thing with API tools is that it is quite hard to test things without real examples. So my suggestion would be to try it out and maybe re-iterate on this as we are quite early in the release there is enough time to fix things as they are discovered so better sooner than later... |
Test Results 650 files + 361 650 suites +361 56m 38s ⏱️ + 33m 41s For more details on these failures, see this check. Results for commit 0967a1b. ± Comparison against base commit 324fb67. |
| super(parent, IApiElement.API_TYPE_CONTAINER, container.getName()); | ||
| this.fRoot = container; | ||
| this.fPackageFragmentRoot = Objects.requireNonNull(packageFragmentRoot); | ||
| if (packageFragmentRoot != null) { |
There was a problem hiding this comment.
I have fixed this in #2203 - but FWIW @laeubi added the javadoc saying "may be null" so it looks like @moaead simply implemented the code as it was javadoc. The PR removes the may be null from the javadoc too.
@laeubi if you can review to make sure your original intentions are still intact that will be appreciated.
There was a problem hiding this comment.
The code has undergone some refactoring so the javadoc was just wrong.
|
macOS test failed with: Caused by: java.io.IOException: Server returned HTTP code: 400 for URL https://download.eclipse.org/eclipse/updates/4.38/R-4.38-202512010920/content.xml.xz I raised https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/7067 as I have seen this error in a few other builds in the last couple of days. |
See https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/6783 :-\ |
|
Is it OK to merge now or not? Would be good to have it in M2 build. |
|
I have added a comment regarding degraded |
Did you pushed your change? |
|
@iloveeclipse I'm ok to merge now, I can provide a update to this pr with null check later today or tomorrow. |
|
OK, let have it in. |
…iners In all current use cases of this internal class the value passed in was non-null anyway, so there is no functional change with this commit, it simply prevents future changes from trying to pass in a null here An extra complication is that in 02fa168 the javadoc was updated to allow null IPackageFragmentRoot but the code did not match that. Fixes eclipse-pde#2202 (comment)
…iners In all current use cases of this internal class the value passed in was non-null anyway, so there is no functional change with this commit, it simply prevents future changes from trying to pass in a null here An extra complication is that in 02fa168 the javadoc was updated to allow null IPackageFragmentRoot but the code did not match that. Fixes eclipse-pde#2202 (comment)
3 participants
This PR was originally #2198 but it seems to have disappeared (404) - the original PR had passed ECA
The fix in commit 215a618 for issue #2096 introduced an O(n^2) time complexity regression when projects have multiple source folders sharing the same output location (like SWT with 26 source roots).
The problem was that each source folder caused a new CompositeApiTypeContainer to be created, wrapping all previous containers plus a new one. This resulted in exponential duplication of container visits during API analysis.
The fix modifies ProjectTypeContainer to support multiple IPackageFragmentRoots, allowing a single container to discover packages from all source folders that share the same output location. This avoids creating composite containers and maintains O(n) complexity.
Fixes #2197