Skip to content

Commit 1cac8bc

Browse files
vogellaclaude
vogella
and
claude
committed
Fix performance issue and infinite loop in classpath computation
Optimize access rule accumulation in RequiredPluginsClasspathContainer by using Set<Rule> instead of List<Rule> to avoid O(N) containment checks during insertion, which was causing O(N^2) complexity for plug-ins with many re-exported packages. Add a recursion guard in findExportedPackages to prevent infinite loops when processing cyclic re-exports in implicit or secondary dependencies. Add two tests: - RequiredPluginsClasspathContainerPerformanceTest: verifies that classpath computation finishes quickly even when secondary dependencies form a cyclic re-export graph. - ChainedReexportPerformanceTest: verifies performance in a chained re-export scenario to ensure no O(N^2) scaling. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 776f934 commit 1cac8bc

File tree

5 files changed

+403
-22
lines changed

5 files changed

+403
-22
lines changed

‎ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/RequiredPluginsClasspathContainer.java‎

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ class RequiredPluginsClasspathContainer {
8585
private static final String JUNIT4_PLUGIN = "org.junit";
8686
private static final VersionRange JUNIT_4_VERSION = new VersionRange("[4.0,5)"); //$NON-NLS-1$
8787
@SuppressWarnings("nls")
88-
private static final Set<String> JUNIT5_RUNTIME_PLUGINS = Set.of("org.junit", //
88+
private static final List<String> JUNIT5_RUNTIME_PLUGINS = List.of("org.junit", //
8989
"junit-platform-launcher",
9090
"org.junit.platform.launcher",
9191
"junit-jupiter-engine", // BSN of the bundle from Maven-Central
@@ -223,7 +223,7 @@ private List<IClasspathEntry> computePluginEntriesByModel() throws CoreException
223223
return List.of();
224224
}
225225

226-
Map<BundleDescription, List<Rule>> map = retrieveVisiblePackagesFromState(desc);
226+
Map<BundleDescription, LinkedHashSet<Rule>> map = retrieveVisiblePackagesFromState(desc);
227227

228228
// Add any library entries contributed via classpath contributor
229229
// extension (Bug 363733)
@@ -338,8 +338,8 @@ private static synchronized Stream<IClasspathContributor> getClasspathContributo
338338
return Stream.concat(fClasspathContributors.stream(), PDECore.getDefault().getClasspathContributors());
339339
}
340340

341-
private Map<BundleDescription, List<Rule>> retrieveVisiblePackagesFromState(BundleDescription desc) {
342-
Map<BundleDescription, List<Rule>> visiblePackages = new HashMap<>();
341+
private Map<BundleDescription, LinkedHashSet<Rule>> retrieveVisiblePackagesFromState(BundleDescription desc) {
342+
Map<BundleDescription, LinkedHashSet<Rule>> visiblePackages = new HashMap<>();
343343
StateHelper helper = BundleHelper.getPlatformAdmin().getStateHelper();
344344
addVisiblePackagesFromState(helper, desc, visiblePackages);
345345
if (desc.getHost() != null) {
@@ -349,7 +349,7 @@ private Map<BundleDescription, List<Rule>> retrieveVisiblePackagesFromState(Bund
349349
}
350350

351351
private void addVisiblePackagesFromState(StateHelper helper, BundleDescription desc,
352-
Map<BundleDescription, List<Rule>> visiblePackages) {
352+
Map<BundleDescription, LinkedHashSet<Rule>> visiblePackages) {
353353
if (desc == null) {
354354
return;
355355
}
@@ -359,11 +359,9 @@ private void addVisiblePackagesFromState(StateHelper helper, BundleDescription d
359359
if (exporter == null) {
360360
continue;
361361
}
362-
List<Rule> list = visiblePackages.computeIfAbsent(exporter, e -> new ArrayList<>());
362+
LinkedHashSet<Rule> list = visiblePackages.computeIfAbsent(exporter, e -> new LinkedHashSet<>());
363363
Rule rule = getRule(helper, desc, export);
364-
if (!list.contains(rule)) {
365-
list.add(rule);
366-
}
364+
list.add(rule);
367365
}
368366
}
369367

@@ -375,7 +373,7 @@ private Rule getRule(StateHelper helper, BundleDescription desc, ExportPackageDe
375373
}
376374

377375
protected void addDependencyViaImportPackage(BundleDescription desc, Set<BundleDescription> added,
378-
Map<BundleDescription, List<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
376+
Map<BundleDescription, LinkedHashSet<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
379377
if (desc == null || !added.add(desc)) {
380378
return;
381379
}
@@ -393,12 +391,12 @@ protected void addDependencyViaImportPackage(BundleDescription desc, Set<BundleD
393391
}
394392

395393
private void addDependency(BundleDescription desc, Set<BundleDescription> added,
396-
Map<BundleDescription, List<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
394+
Map<BundleDescription, LinkedHashSet<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
397395
addDependency(desc, added, map, entries, true);
398396
}
399397

400398
private void addDependency(BundleDescription desc, Set<BundleDescription> added,
401-
Map<BundleDescription, List<Rule>> map, List<IClasspathEntry> entries, boolean useInclusion)
399+
Map<BundleDescription, LinkedHashSet<Rule>> map, List<IClasspathEntry> entries, boolean useInclusion)
402400
throws CoreException {
403401
if (desc == null || !added.add(desc)) {
404402
return;
@@ -441,7 +439,7 @@ private void addDependency(BundleDescription desc, Set<BundleDescription> added,
441439
}
442440
}
443441

444-
private boolean addPlugin(BundleDescription desc, boolean useInclusions, Map<BundleDescription, List<Rule>> map,
442+
private boolean addPlugin(BundleDescription desc, boolean useInclusions, Map<BundleDescription, LinkedHashSet<Rule>> map,
445443
List<IClasspathEntry> entries) throws CoreException {
446444
IPluginModelBase model = PluginRegistry.findModel((Resource) desc);
447445
if (model == null || !model.isEnabled()) {
@@ -469,7 +467,7 @@ private boolean addPlugin(BundleDescription desc, boolean useInclusions, Map<Bun
469467
return true;
470468
}
471469

472-
private List<Rule> getInclusions(Map<BundleDescription, List<Rule>> map, IPluginModelBase model) {
470+
private List<Rule> getInclusions(Map<BundleDescription, LinkedHashSet<Rule>> map, IPluginModelBase model) {
473471
BundleDescription desc = model.getBundleDescription();
474472
if (desc == null || "false".equals(System.getProperty("pde.restriction")) //$NON-NLS-1$ //$NON-NLS-2$
475473
|| !(fModel instanceof IBundlePluginModelBase) || TargetPlatformHelper.getTargetVersion() < 3.1) {
@@ -479,12 +477,12 @@ private List<Rule> getInclusions(Map<BundleDescription, List<Rule>> map, IPlugin
479477
if (desc.getHost() != null) {
480478
desc = (BundleDescription) desc.getHost().getSupplier();
481479
}
482-
List<Rule> rules = map.getOrDefault(desc, List.of());
483-
return (rules.isEmpty() && !ClasspathUtilCore.hasBundleStructure(model)) ? null : rules;
480+
LinkedHashSet<Rule> rules = map.getOrDefault(desc, new LinkedHashSet<>());
481+
return (rules.isEmpty() && !ClasspathUtilCore.hasBundleStructure(model)) ? null : new ArrayList<>(rules);
484482
}
485483

486484
private void addHostPlugin(HostSpecification hostSpec, Set<BundleDescription> added,
487-
Map<BundleDescription, List<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
485+
Map<BundleDescription, LinkedHashSet<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
488486
BaseDescription desc = hostSpec.getSupplier();
489487

490488
if (desc instanceof BundleDescription host) {
@@ -628,7 +626,7 @@ private void addJunit5RuntimeDependencies(Set<BundleDescription> added, List<ICl
628626
}
629627

630628
// add dependency with exclude all rule
631-
Map<BundleDescription, List<Rule>> rules = Map.of(desc, List.of());
629+
Map<BundleDescription, LinkedHashSet<Rule>> rules = Map.of(desc, new LinkedHashSet<>());
632630
addPlugin(desc, true, rules, entries);
633631
}
634632
}
@@ -691,7 +689,7 @@ private void addTransitiveDependenciesWithForbiddenAccess(Set<BundleDescription>
691689
while (transitiveDeps.hasNext()) {
692690
BundleDescription desc = transitiveDeps.next();
693691
if (added.add(desc)) {
694-
Map<BundleDescription, List<Rule>> rules = Map.of(desc, List.of());
692+
Map<BundleDescription, LinkedHashSet<Rule>> rules = Map.of(desc, new LinkedHashSet<>());
695693
addPlugin(desc, true, rules, entries);
696694
}
697695
}
@@ -705,21 +703,24 @@ private void addExtraModel(BundleDescription desc, Set<BundleDescription> added,
705703
if (added.contains(bundleDesc)) {
706704
return;
707705
}
708-
Map<BundleDescription, List<Rule>> rules = new HashMap<>();
706+
Map<BundleDescription, LinkedHashSet<Rule>> rules = new HashMap<>();
709707
findExportedPackages(bundleDesc, desc, rules);
710708
addDependency(bundleDesc, added, rules, entries, true);
711709
}
712710
}
713711

714712
protected final void findExportedPackages(BundleDescription desc, BundleDescription projectDesc,
715-
Map<BundleDescription, List<Rule>> map) {
713+
Map<BundleDescription, LinkedHashSet<Rule>> map) {
716714
if (desc != null) {
717715
Queue<BundleDescription> queue = new ArrayDeque<>();
718716
queue.add(desc);
719717
while (!queue.isEmpty()) {
720718
BundleDescription bdesc = queue.remove();
719+
if (map.containsKey(bdesc)) {
720+
continue;
721+
}
721722
ExportPackageDescription[] expkgs = bdesc.getExportPackages();
722-
List<Rule> rules = new ArrayList<>();
723+
LinkedHashSet<Rule> rules = new LinkedHashSet<>();
723724
for (ExportPackageDescription expkg : expkgs) {
724725
boolean discouraged = restrictPackage(projectDesc, expkg);
725726
IPath path = IPath.fromOSString(expkg.getName().replace('.', '/') + "/*"); //$NON-NLS-1$

‎ui/org.eclipse.pde.ui.tests/META-INF/MANIFEST.MF‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ Import-Package: jakarta.annotation;version="[2.1.0,3.0.0)",
5959
org.eclipse.pde.internal.build,
6060
org.hamcrest,
6161
org.junit,
62+
org.junit.jupiter.api;version="[5.8.1,6.0.0)",
6263
org.junit.jupiter.api.function;version="[5.8.1,6.0.0)",
6364
org.junit.jupiter.migrationsupport;version="[5.13.0,6.0.0)",
6465
org.junit.platform.suite.api;version="[1.13.0,2.0.0)",

‎ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/classpath/ChainedReexportPerformanceTest.java‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
/*******************************************************************************
2+
* Copyright (c) 2025 vogella GmbH and others.
3+
*
4+
* This program and the accompanying materials
5+
* are made available under the terms of the Eclipse Public License 2.0
6+
* which accompanies this distribution, and is available at
7+
* https://www.eclipse.org/legal/epl-2.0/
8+
*
9+
* SPDX-License-Identifier: EPL-2.0
10+
*
11+
* Contributors:
12+
* Lars Vogel - initial API and implementation
13+
*******************************************************************************/
14+
package org.eclipse.pde.core.tests.internal.classpath;
15+
16+
import static org.junit.jupiter.api.Assertions.assertFalse;
17+
import static org.junit.jupiter.api.Assertions.assertTrue;
18+
19+
import java.io.File;
20+
import java.io.FileOutputStream;
21+
import java.io.IOException;
22+
import java.nio.file.Files;
23+
import java.nio.file.Path;
24+
import java.util.Comparator;
25+
import java.util.List;
26+
import java.util.jar.Attributes;
27+
import java.util.jar.JarOutputStream;
28+
import java.util.jar.Manifest;
29+
import java.util.zip.ZipEntry;
30+
31+
import org.eclipse.core.resources.IProject;
32+
import org.eclipse.core.resources.IWorkspaceDescription;
33+
import org.eclipse.core.resources.IncrementalProjectBuilder;
34+
import org.eclipse.core.resources.ResourcesPlugin;
35+
import org.eclipse.core.runtime.NullProgressMonitor;
36+
import org.eclipse.jdt.core.IClasspathEntry;
37+
import org.eclipse.jdt.core.JavaCore;
38+
import org.eclipse.pde.core.plugin.IPluginModelBase;
39+
import org.eclipse.pde.core.plugin.PluginRegistry;
40+
import org.eclipse.pde.core.project.IBundleProjectDescription;
41+
import org.eclipse.pde.core.project.IBundleProjectService;
42+
import org.eclipse.pde.core.project.IRequiredBundleDescription;
43+
import org.eclipse.pde.core.target.ITargetDefinition;
44+
import org.eclipse.pde.core.target.ITargetLocation;
45+
import org.eclipse.pde.core.target.ITargetPlatformService;
46+
import org.eclipse.pde.internal.core.PDECore;
47+
import org.eclipse.pde.internal.ui.wizards.tools.UpdateClasspathJob;
48+
import org.eclipse.pde.ui.tests.runtime.TestUtils;
49+
import org.eclipse.pde.ui.tests.util.ProjectUtils;
50+
import org.eclipse.pde.ui.tests.util.TargetPlatformUtil;
51+
import org.junit.jupiter.api.AfterAll;
52+
import org.junit.jupiter.api.AfterEach;
53+
import org.junit.jupiter.api.BeforeAll;
54+
import org.junit.jupiter.api.BeforeEach;
55+
import org.junit.jupiter.api.Test;
56+
import org.osgi.framework.Version;
57+
58+
public class ChainedReexportPerformanceTest {
59+
60+
private static final String CHAIN_PREFIX = "Chain_";
61+
private static final int PACKAGE_COUNT = 1000;
62+
private static final int BUNDLE_CHAIN_DEPTH = 5;
63+
private File targetDir;
64+
private ITargetDefinition savedTarget;
65+
66+
@BeforeAll
67+
public static void beforeAll() throws Exception {
68+
ProjectUtils.deleteAllWorkspaceProjects();
69+
}
70+
71+
@AfterAll
72+
public static void afterAll() throws Exception {
73+
ProjectUtils.deleteAllWorkspaceProjects();
74+
}
75+
76+
@BeforeEach
77+
public void setUp() throws Exception {
78+
savedTarget = TargetPlatformUtil.TPS.getWorkspaceTargetDefinition();
79+
80+
IWorkspaceDescription desc = ResourcesPlugin.getWorkspace().getDescription();
81+
desc.setAutoBuilding(false);
82+
ResourcesPlugin.getWorkspace().setDescription(desc);
83+
84+
targetDir = Files.createTempDirectory("pde_chain_perf_target").toFile();
85+
createChainedTargetPlatform();
86+
}
87+
88+
@AfterEach
89+
public void tearDown() throws Exception {
90+
IWorkspaceDescription desc = ResourcesPlugin.getWorkspace().getDescription();
91+
desc.setAutoBuilding(true);
92+
ResourcesPlugin.getWorkspace().setDescription(desc);
93+
94+
if (targetDir != null && targetDir.exists()) {
95+
Files.walk(targetDir.toPath()).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
96+
}
97+
98+
if (savedTarget != null && savedTarget != TargetPlatformUtil.TPS.getWorkspaceTargetDefinition()) {
99+
TargetPlatformUtil.loadAndSetTarget(savedTarget);
100+
}
101+
}
102+
103+
private void createChainedTargetPlatform() throws Exception {
104+
// Create a chain of bundles: B_0 -> B_1 -> ... -> B_N (all re-exporting)
105+
for (int i = 0; i < BUNDLE_CHAIN_DEPTH; i++) {
106+
String name = CHAIN_PREFIX + i;
107+
String exports = createPackageExports(name);
108+
String requires = (i > 0) ? (CHAIN_PREFIX + (i - 1) + ";visibility:=reexport") : null;
109+
createBundle(targetDir, name, exports, requires);
110+
}
111+
112+
ITargetPlatformService tps = PDECore.getDefault().acquireService(ITargetPlatformService.class);
113+
ITargetDefinition target = tps.newTarget();
114+
target.setTargetLocations(new ITargetLocation[] { tps.newDirectoryLocation(targetDir.getAbsolutePath()) });
115+
TargetPlatformUtil.loadAndSetTarget(target);
116+
}
117+
118+
private String createPackageExports(String bundleName) {
119+
StringBuilder sb = new StringBuilder();
120+
for (int i = 0; i < PACKAGE_COUNT; i++) {
121+
if (sb.length() > 0) {
122+
sb.append(",");
123+
}
124+
sb.append(bundleName).append(".pkg.").append(i);
125+
}
126+
return sb.toString();
127+
}
128+
129+
private void createBundle(File dir, String name, String exports, String requires) throws IOException {
130+
File jarFile = new File(dir, name + ".jar");
131+
try (JarOutputStream jos = new JarOutputStream(new FileOutputStream(jarFile))) {
132+
Manifest manifest = new Manifest();
133+
Attributes main = manifest.getMainAttributes();
134+
main.put(Attributes.Name.MANIFEST_VERSION, "1.0");
135+
main.put(new Attributes.Name("Bundle-ManifestVersion"), "2");
136+
main.put(new Attributes.Name("Bundle-SymbolicName"), name);
137+
main.put(new Attributes.Name("Bundle-Version"), "1.0.0");
138+
if (exports != null) {
139+
main.put(new Attributes.Name("Export-Package"), exports);
140+
}
141+
if (requires != null) {
142+
main.put(new Attributes.Name("Require-Bundle"), requires);
143+
}
144+
145+
ZipEntry entry = new ZipEntry("META-INF/MANIFEST.MF");
146+
jos.putNextEntry(entry);
147+
manifest.write(jos);
148+
jos.closeEntry();
149+
}
150+
}
151+
152+
@Test
153+
public void testChainedReexportPerformance() throws Exception {
154+
IBundleProjectService service = PDECore.getDefault().acquireService(IBundleProjectService.class);
155+
156+
String consumerName = "ConsumerBundle";
157+
IProject consumerProj = ResourcesPlugin.getWorkspace().getRoot().getProject(consumerName);
158+
consumerProj.create(null);
159+
consumerProj.open(null);
160+
161+
IBundleProjectDescription consumerDesc = service.getDescription(consumerProj);
162+
consumerDesc.setSymbolicName(consumerName);
163+
consumerDesc.setBundleVersion(new Version("1.0.0"));
164+
165+
// Require the last bundle in the chain with re-export
166+
IRequiredBundleDescription mainReq = service.newRequiredBundle(CHAIN_PREFIX + (BUNDLE_CHAIN_DEPTH - 1), null,
167+
false, true);
168+
consumerDesc.setRequiredBundles(new IRequiredBundleDescription[] { mainReq });
169+
170+
consumerDesc.setNatureIds(new String[] { JavaCore.NATURE_ID, IBundleProjectDescription.PLUGIN_NATURE });
171+
consumerDesc.apply(null);
172+
173+
ResourcesPlugin.getWorkspace().build(IncrementalProjectBuilder.FULL_BUILD, new NullProgressMonitor());
174+
TestUtils.waitForJobs("Init", 500, 5000);
175+
176+
IPluginModelBase consumerModel = PluginRegistry.findModel(consumerProj);
177+
if (consumerModel == null) {
178+
throw new IllegalStateException("Consumer model not found");
179+
}
180+
181+
UpdateClasspathJob.scheduleFor(List.of(consumerModel), false);
182+
boolean timedOut = TestUtils.waitForJobs("classpath", 100, 10000);
183+
assertFalse(timedOut, "Performance regression: classpath computation timed out for chained re-exports");
184+
185+
IClasspathEntry[] resolvedClasspath = JavaCore.create(consumerProj).getRawClasspath();
186+
assertTrue(resolvedClasspath.length > 0, "Classpath should not be empty");
187+
}
188+
}

0 commit comments

Comments
 (0)