Skip to content

Fix API tools slow down with multiple source folders sharing output #2202

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.

Sign up for GitHub

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

Merged
iloveeclipse merged 1 commit into from
Jan 21, 2026
Merged

Fix API tools slow down with multiple source folders sharing output #2202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .../src/org/eclipse/pde/api/tools/builder/tests/compatibility/ProjectTypeContainerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,9 @@ public void testMultipleSourceFoldersWithSharedOutput() throws CoreException {
IApiTypeContainer[] containers = component.getApiTypeContainers();
assertEquals("Wrong number of API type containers", 1, containers.length); //$NON-NLS-1$
IApiTypeContainer container = containers[0];
assertEquals("Should be a composite type container", IApiTypeContainer.COMPOSITE, //$NON-NLS-1$
// A single ProjectTypeContainer handles multiple source roots sharing
// the same output location by tracking all package fragment roots
assertEquals("Should be a folder type container", IApiTypeContainer.FOLDER, //$NON-NLS-1$
container.getContainerType());

String[] packageNames = container.getPackageNames();
Expand Down
28 changes: 4 additions & 24 deletions ....eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/model/ProjectComponent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package org.eclipse.pde.api.tools.internal.model;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -419,30 +418,11 @@ private static IApiTypeContainer getApiTypeContainer(String location, ProjectCom
}
cfc = new ProjectTypeContainer(component, container, root);
outputLocationToContainer.put(outputLocation, cfc);
} else {
} else if (cfc instanceof ProjectTypeContainer) {
// Multiple source folders share the same output location
// Create a composite container to include packages from all source roots
List<IApiTypeContainer> containers = new ArrayList<>();
if (cfc instanceof CompositeApiTypeContainer) {
// Already a composite, add to it
IApiTypeContainer[] typeContainers = ((CompositeApiTypeContainer) cfc)
.getApiTypeContainers();
containers.addAll(Arrays.asList(typeContainers));
} else {
// Convert single container to composite
containers.add(cfc);
}
// Add new container for this source root
IPath projectFullPath = project.getProject().getFullPath();
IContainer container = null;
if (projectFullPath.equals(outputLocation)) {
container = project.getProject();
} else {
container = project.getProject().getWorkspace().getRoot().getFolder(outputLocation);
}
containers.add(new ProjectTypeContainer(component, container, root));
cfc = new CompositeApiTypeContainer(component, containers);
outputLocationToContainer.put(outputLocation, cfc);
// Add the new package fragment root to the existing container
// so it can discover packages from all source roots
((ProjectTypeContainer) cfc).addPackageFragmentRoot(root);
}
return cfc;
}
Expand Down
32 changes: 26 additions & 6 deletions ...ipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/model/ProjectTypeContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
import java.util.concurrent.CopyOnWriteArrayList;

import org.eclipse.core.resources.IContainer;
import org.eclipse.core.resources.IFile;
Expand Down Expand Up @@ -50,9 +50,10 @@ public class ProjectTypeContainer extends ApiElement implements IApiTypeContaine
private String[] fPackageNames;

/**
* Optional package fragment root for JDT-based package discovery
* Package fragment roots for JDT-based package discovery. Multiple roots
* may exist when several source folders share the same output location.
*/
private final IPackageFragmentRoot fPackageFragmentRoot;
private final List<IPackageFragmentRoot> fPackageFragmentRoots = new CopyOnWriteArrayList<>();

/**
* Constructs an {@link IApiTypeContainer} rooted at the location with an
Expand All @@ -67,7 +68,24 @@ public class ProjectTypeContainer extends ApiElement implements IApiTypeContaine
public ProjectTypeContainer(IApiElement parent, IContainer container, IPackageFragmentRoot packageFragmentRoot) {
super(parent, IApiElement.API_TYPE_CONTAINER, container.getName());
this.fRoot = container;
this.fPackageFragmentRoot = Objects.requireNonNull(packageFragmentRoot);
if (packageFragmentRoot != null) {
Copy link
Contributor

@laeubi laeubi Jan 21, 2026

Choose a reason for hiding this comment

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

null should not be allowed

Copy link
Contributor Author

@jonahgraham jonahgraham Jan 21, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@laeubi laeubi Jan 22, 2026

Choose a reason for hiding this comment

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

The code has undergone some refactoring so the javadoc was just wrong.

this.fPackageFragmentRoots.add(packageFragmentRoot);
}
}

/**
* Adds an additional package fragment root to this container. This is used
* when multiple source folders share the same output location.
*
* @param root the package fragment root to add
* @since 1.3.400
*/
public void addPackageFragmentRoot(IPackageFragmentRoot root) {
if (root != null && !fPackageFragmentRoots.contains(root)) {
fPackageFragmentRoots.add(root);
// Clear cached package names so they will be recomputed
fPackageNames = null;
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
}

@Override
Expand Down Expand Up @@ -159,8 +177,10 @@ public IApiTypeRoot findTypeRoot(String qualifiedName) throws CoreException {
public String[] getPackageNames() throws CoreException {
if (fPackageNames == null) {
SortedSet<String> names = new TreeSet<>();
if (fPackageFragmentRoot.exists()) {
collectPackageNames(names, fPackageFragmentRoot);
for (IPackageFragmentRoot root : fPackageFragmentRoots) {
if (root.exists()) {
collectPackageNames(names, root);
}
}
fPackageNames = names.toArray(String[]::new);
}
Expand Down
Loading