Skip to content

Commit f170c65

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 b44e6b5 commit f170c65

File tree

5 files changed

+402
-21
lines changed

5 files changed

+402
-21
lines changed

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

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ private List<IClasspathEntry> computePluginEntriesByModel() throws CoreException
220220
return List.of();
221221
}
222222

223-
Map<BundleDescription, List<Rule>> map = retrieveVisiblePackagesFromState(desc);
223+
Map<BundleDescription, Set<Rule>> map = retrieveVisiblePackagesFromState(desc);
224224

225225
// Add any library entries contributed via classpath contributor
226226
// extension (Bug 363733)
@@ -333,8 +333,8 @@ private static synchronized Stream<IClasspathContributor> getClasspathContributo
333333
return Stream.concat(fClasspathContributors.stream(), PDECore.getDefault().getClasspathContributors());
334334
}
335335

336-
private Map<BundleDescription, List<Rule>> retrieveVisiblePackagesFromState(BundleDescription desc) {
337-
Map<BundleDescription, List<Rule>> visiblePackages = new HashMap<>();
336+
private Map<BundleDescription, Set<Rule>> retrieveVisiblePackagesFromState(BundleDescription desc) {
337+
Map<BundleDescription, Set<Rule>> visiblePackages = new HashMap<>();
338338
StateHelper helper = BundleHelper.getPlatformAdmin().getStateHelper();
339339
addVisiblePackagesFromState(helper, desc, visiblePackages);
340340
if (desc.getHost() != null) {
@@ -344,7 +344,7 @@ private Map<BundleDescription, List<Rule>> retrieveVisiblePackagesFromState(Bund
344344
}
345345

346346
private void addVisiblePackagesFromState(StateHelper helper, BundleDescription desc,
347-
Map<BundleDescription, List<Rule>> visiblePackages) {
347+
Map<BundleDescription, Set<Rule>> visiblePackages) {
348348
if (desc == null) {
349349
return;
350350
}
@@ -354,11 +354,9 @@ private void addVisiblePackagesFromState(StateHelper helper, BundleDescription d
354354
if (exporter == null) {
355355
continue;
356356
}
357-
List<Rule> list = visiblePackages.computeIfAbsent(exporter, e -> new ArrayList<>());
357+
Set<Rule> list = visiblePackages.computeIfAbsent(exporter, e -> new LinkedHashSet<>());
358358
Rule rule = getRule(helper, desc, export);
359-
if (!list.contains(rule)) {
360-
list.add(rule);
361-
}
359+
list.add(rule);
362360
}
363361
}
364362

@@ -370,7 +368,7 @@ private Rule getRule(StateHelper helper, BundleDescription desc, ExportPackageDe
370368
}
371369

372370
protected void addDependencyViaImportPackage(BundleDescription desc, Set<BundleDescription> added,
373-
Map<BundleDescription, List<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
371+
Map<BundleDescription, Set<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
374372
if (desc == null || !added.add(desc)) {
375373
return;
376374
}
@@ -388,12 +386,12 @@ protected void addDependencyViaImportPackage(BundleDescription desc, Set<BundleD
388386
}
389387

390388
private void addDependency(BundleDescription desc, Set<BundleDescription> added,
391-
Map<BundleDescription, List<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
389+
Map<BundleDescription, Set<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
392390
addDependency(desc, added, map, entries, true);
393391
}
394392

395393
private void addDependency(BundleDescription desc, Set<BundleDescription> added,
396-
Map<BundleDescription, List<Rule>> map, List<IClasspathEntry> entries, boolean useInclusion)
394+
Map<BundleDescription, Set<Rule>> map, List<IClasspathEntry> entries, boolean useInclusion)
397395
throws CoreException {
398396
if (desc == null || !added.add(desc)) {
399397
return;
@@ -436,7 +434,7 @@ private void addDependency(BundleDescription desc, Set<BundleDescription> added,
436434
}
437435
}
438436

439-
private boolean addPlugin(BundleDescription desc, boolean useInclusions, Map<BundleDescription, List<Rule>> map,
437+
private boolean addPlugin(BundleDescription desc, boolean useInclusions, Map<BundleDescription, Set<Rule>> map,
440438
List<IClasspathEntry> entries) throws CoreException {
441439
IPluginModelBase model = PluginRegistry.findModel((Resource) desc);
442440
if (model == null || !model.isEnabled()) {
@@ -464,7 +462,7 @@ private boolean addPlugin(BundleDescription desc, boolean useInclusions, Map<Bun
464462
return true;
465463
}
466464

467-
private List<Rule> getInclusions(Map<BundleDescription, List<Rule>> map, IPluginModelBase model) {
465+
private List<Rule> getInclusions(Map<BundleDescription, Set<Rule>> map, IPluginModelBase model) {
468466
BundleDescription desc = model.getBundleDescription();
469467
if (desc == null || "false".equals(System.getProperty("pde.restriction")) //$NON-NLS-1$ //$NON-NLS-2$
470468
|| !(fModel instanceof IBundlePluginModelBase) || TargetPlatformHelper.getTargetVersion() < 3.1) {
@@ -474,12 +472,12 @@ private List<Rule> getInclusions(Map<BundleDescription, List<Rule>> map, IPlugin
474472
if (desc.getHost() != null) {
475473
desc = (BundleDescription) desc.getHost().getSupplier();
476474
}
477-
List<Rule> rules = map.getOrDefault(desc, List.of());
478-
return (rules.isEmpty() && !ClasspathUtilCore.hasBundleStructure(model)) ? null : rules;
475+
Set<Rule> rules = map.getOrDefault(desc, Set.of());
476+
return (rules.isEmpty() && !ClasspathUtilCore.hasBundleStructure(model)) ? null : new ArrayList<>(rules);
479477
}
480478

481479
private void addHostPlugin(HostSpecification hostSpec, Set<BundleDescription> added,
482-
Map<BundleDescription, List<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
480+
Map<BundleDescription, Set<Rule>> map, List<IClasspathEntry> entries) throws CoreException {
483481
BaseDescription desc = hostSpec.getSupplier();
484482

485483
if (desc instanceof BundleDescription host) {
@@ -623,7 +621,7 @@ private void addJunit5RuntimeDependencies(Set<BundleDescription> added, List<ICl
623621
}
624622

625623
// add dependency with exclude all rule
626-
Map<BundleDescription, List<Rule>> rules = Map.of(desc, List.of());
624+
Map<BundleDescription, Set<Rule>> rules = Map.of(desc, Set.of());
627625
addPlugin(desc, true, rules, entries);
628626
}
629627
}
@@ -687,7 +685,7 @@ private void addTransitiveDependenciesWithForbiddenAccess(Set<BundleDescription>
687685
while (transitiveDeps.hasNext()) {
688686
BundleDescription desc = transitiveDeps.next();
689687
if (added.add(desc)) {
690-
Map<BundleDescription, List<Rule>> rules = Map.of(desc, List.of());
688+
Map<BundleDescription, Set<Rule>> rules = Map.of(desc, Set.of());
691689
addPlugin(desc, true, rules, entries);
692690
}
693691
}
@@ -701,21 +699,24 @@ private void addExtraModel(BundleDescription desc, Set<BundleDescription> added,
701699
if (added.contains(bundleDesc)) {
702700
return;
703701
}
704-
Map<BundleDescription, List<Rule>> rules = new HashMap<>();
702+
Map<BundleDescription, Set<Rule>> rules = new HashMap<>();
705703
findExportedPackages(bundleDesc, desc, rules);
706704
addDependency(bundleDesc, added, rules, entries, true);
707705
}
708706
}
709707

710708
protected final void findExportedPackages(BundleDescription desc, BundleDescription projectDesc,
711-
Map<BundleDescription, List<Rule>> map) {
709+
Map<BundleDescription, Set<Rule>> map) {
712710
if (desc != null) {
713711
Queue<BundleDescription> queue = new ArrayDeque<>();
714712
queue.add(desc);
715713
while (!queue.isEmpty()) {
716714
BundleDescription bdesc = queue.remove();
715+
if (map.containsKey(bdesc)) {
716+
continue;
717+
}
717718
ExportPackageDescription[] expkgs = bdesc.getExportPackages();
718-
List<Rule> rules = new ArrayList<>();
719+
Set<Rule> rules = new LinkedHashSet<>();
719720
for (ExportPackageDescription expkg : expkgs) {
720721
boolean discouraged = restrictPackage(projectDesc, expkg);
721722
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)