Skip to content

Commit

Permalink
Prevent excessive creation of intermediate SplitPackageBinding instances
Browse files Browse the repository at this point in the history 
* don't call SPB.combine() in a tight loop, but use new combineAll()

fixes eclipse-jdt#2646
  • Loading branch information
@stephan-herrmann
stephan-herrmann committed Jul 18, 2024
1 parent 25606f2 commit 685981d
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 16 deletions.
10 changes: 8 additions & 2 deletions ...t.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/LookupEnvironment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -769,11 +769,14 @@ private PackageBinding computePackageFrom(char[][] constantPoolName, boolean isM
if (this.module.isUnnamed()) {
char[][] declaringModules = ((IModuleAwareNameEnvironment) this.nameEnvironment).getUniqueModulesDeclaringPackage(new char[][] {constantPoolName[0]}, ModuleBinding.ANY);
if (declaringModules != null) {
List<PackageBinding> bindings = new ArrayList<>();
for (char[] mod : declaringModules) {
ModuleBinding declaringModule = this.root.getModule(mod);
if (declaringModule != null)
packageBinding = SplitPackageBinding.combine(declaringModule.getTopLevelPackage(constantPoolName[0]), packageBinding, this.module);
bindings.add(declaringModule.getTopLevelPackage(constantPoolName[0]));
}
if (!bindings.isEmpty())
packageBinding = SplitPackageBinding.combineAll(bindings, this.module);
}
} else {
packageBinding = this.module.getTopLevelPackage(constantPoolName[0]);
Expand All @@ -794,12 +797,15 @@ private PackageBinding computePackageFrom(char[][] constantPoolName, boolean isM
char[][] currentCompoundName = CharOperation.arrayConcat(parent.compoundName, constantPoolName[i]);
char[][] declaringModules = ((IModuleAwareNameEnvironment) this.nameEnvironment).getModulesDeclaringPackage(
currentCompoundName, ModuleBinding.ANY);
List<PackageBinding> bindings = new ArrayList<>();
if (declaringModules != null) {
for (char[] mod : declaringModules) {
ModuleBinding declaringModule = this.root.getModule(mod);
if (declaringModule != null)
packageBinding = SplitPackageBinding.combine(declaringModule.getVisiblePackage(currentCompoundName), packageBinding, this.module);
bindings.add(declaringModule.getVisiblePackage(currentCompoundName));
}
if (!bindings.isEmpty())
packageBinding = SplitPackageBinding.combineAll(bindings, this.module);
}
} else {
packageBinding = this.module.getVisiblePackage(parent, constantPoolName[i]);
Expand Down
20 changes: 14 additions & 6 deletions ...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ModuleBinding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*******************************************************************************/
package org.eclipse.jdt.internal.compiler.lookup;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
Expand Down Expand Up @@ -595,13 +596,17 @@ PackageBinding getVisiblePackage(PackageBinding parent, char[] name) {
}
} else {
// visible but foreign (when current is unnamed or auto):
List<PackageBinding> bindings = new ArrayList<>();
for (char[] declaringModuleName : declaringModuleNames) {
ModuleBinding declaringModule = this.environment.root.getModule(declaringModuleName);
if (declaringModule != null) {
PlainPackageBinding declaredPackage = declaringModule.getDeclaredPackage(fullFlatName);
binding = SplitPackageBinding.combine(declaredPackage, binding, this);
if (declaredPackage != null)
bindings.add(declaredPackage);
}
}
if (!bindings.isEmpty())
binding = SplitPackageBinding.combineAll(bindings, this);
}
}
}
Expand Down Expand Up @@ -664,11 +669,14 @@ public PackageBinding getVisiblePackage(char[][] qualifiedPackageName) {
}

PackageBinding combineWithPackagesFromOtherRelevantModules(PackageBinding currentBinding, char[][] compoundName, char[][] declaringModuleNames) {
for (ModuleBinding moduleBinding : otherRelevantModules(declaringModuleNames)) {
PlainPackageBinding nextBinding = moduleBinding.getDeclaredPackage(CharOperation.concatWith(compoundName, '.'));
currentBinding = SplitPackageBinding.combine(nextBinding, currentBinding, this);
}
return currentBinding;
char[] packageName = CharOperation.concatWith(compoundName, '.');
List<PackageBinding> bindings = otherRelevantModules(declaringModuleNames).stream()
.map(m -> m.getDeclaredPackage(packageName))
.collect(Collectors.toList());
if (bindings.isEmpty())
return currentBinding;
bindings.add(currentBinding);
return SplitPackageBinding.combineAll(bindings, this);
}

List<ModuleBinding> otherRelevantModules(char[][] declaringModuleNames) {
Expand Down
65 changes: 57 additions & 8 deletions ...core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/SplitPackageBinding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,22 @@
*******************************************************************************/
package org.eclipse.jdt.internal.compiler.lookup;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Consumer;

import org.eclipse.jdt.core.compiler.CharOperation;

public class SplitPackageBinding extends PackageBinding {
Set<ModuleBinding> declaringModules;
public Set<PlainPackageBinding> incarnations;

/** TEST ONLY */
public static Consumer<SplitPackageBinding> instanceListener;

/**
* Combine two potential package bindings, answering either the better of those if the other has a problem,
* or combine both into a split package.
Expand Down Expand Up @@ -51,21 +57,58 @@ public static PackageBinding combine(PackageBinding binding, PackageBinding prev
split.add(binding);
return split;
}
public static PackageBinding combineAll(List<PackageBinding> bindings, ModuleBinding primaryModule) {
// collect statistics per rank:
int[] numRanked = new int[RANK_VALID+1];
for (PackageBinding packageBinding : bindings) {
int rank = rank(packageBinding);
numRanked[rank]++;
}
SplitPackageBinding split = null;
for (int rank = 3; rank >= 0; rank--) {
int num = numRanked[rank];
if (num > 0) {
// rank is the best we have, so take all bindings at this rank:
for (PackageBinding packageBinding : bindings) {
if (rank(packageBinding) == rank) {
if (num == 1) {
return packageBinding; // singleton doesn't need SplitPackageBinding
}
if (rank == 0) {
return null; // only null, nothing to combine
}
// finally collect all relevant:
if (split == null)
split = new SplitPackageBinding(packageBinding, primaryModule);
else
split.add(packageBinding);
}
}
return split;
}
}
return null;
}
private static int RANK_VALID = 3;
private static int rank(PackageBinding candidate) {
if (candidate == null)
return 0;
if (candidate == LookupEnvironment.TheNotFoundPackage)
return 1;
if (!candidate.isValidBinding())
return 2;
return 3;
return RANK_VALID;
}

public SplitPackageBinding(PackageBinding initialBinding, ModuleBinding primaryModule) {
super(initialBinding.compoundName, initialBinding.parent, primaryModule.environment, primaryModule);
this.declaringModules = new LinkedHashSet<>();
this.incarnations = new LinkedHashSet<>();
add(initialBinding);
// TEST hook:
if (instanceListener != null) {
instanceListener.accept(this);
}
}
public void add(PackageBinding packageBinding) {
if (packageBinding instanceof SplitPackageBinding) {
Expand Down Expand Up @@ -108,16 +151,21 @@ PackageBinding combineWithSiblings(PackageBinding childPackage, char[] name, Mod
ModuleBinding primaryModule = childPackage.enclosingModule;
// see if other incarnations contribute to the child package, too:
char[] flatName = CharOperation.concatWith(childPackage.compoundName, '.');
List<PackageBinding> bindings = new ArrayList<>();
for (PackageBinding incarnation : this.incarnations) {
ModuleBinding moduleBinding = incarnation.enclosingModule;
if (moduleBinding == module)
continue;
if (childPackage.isDeclaredIn(moduleBinding))
continue;
PlainPackageBinding next = moduleBinding.getDeclaredPackage(flatName);
childPackage = combine(next, childPackage, primaryModule);
if (next != null)
bindings.add(next);
}
return childPackage;
if (bindings.isEmpty())
return childPackage;
bindings.add(childPackage);
return combineAll(bindings, primaryModule);
}

@Override
Expand All @@ -131,13 +179,14 @@ PackageBinding getPackage0(char[] name) {
if (knownPackage != null)
return knownPackage;

PackageBinding candidate = null;
List<PackageBinding> bindings = new ArrayList<>();
for (PackageBinding incarnation : this.incarnations) {
PackageBinding package0 = incarnation.getPackage0(name);
if (package0 == null)
return null; // if any incarnation lacks cached info, a full findPackage will be necessary
candidate = combine(package0, candidate, this.enclosingModule);
bindings.add(package0);
}
PackageBinding candidate = combineAll(bindings, this.enclosingModule);
if (candidate != null)
this.knownPackages.put(name, candidate);

Expand All @@ -150,15 +199,15 @@ PackageBinding getPackage0Any(char[] name) {
if (knownPackage != null)
return knownPackage;

PackageBinding candidate = null;
List<PackageBinding> bindings = new ArrayList<>();
for (PackageBinding incarnation : this.incarnations) {
PackageBinding package0 = incarnation.getPackage0(name);
if (package0 == null)
continue;
candidate = combine(package0, candidate, this.enclosingModule);
bindings.add(package0);
}
// don't cache the result, maybe incomplete
return candidate;
return combineAll(bindings, this.enclosingModule);
}

@Override
Expand Down
42 changes: 42 additions & 0 deletions ...s.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ModuleCompilationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Consumer;

import org.eclipse.jdt.core.JavaCore;
import org.eclipse.jdt.core.ToolFactory;
Expand All @@ -38,6 +39,7 @@
import org.eclipse.jdt.core.util.IModuleAttribute;
import org.eclipse.jdt.internal.compiler.classfmt.ClassFileConstants;
import org.eclipse.jdt.internal.compiler.codegen.AttributeNamesConstants;
import org.eclipse.jdt.internal.compiler.lookup.SplitPackageBinding;

import junit.framework.AssertionFailedError;
import junit.framework.Test;
Expand Down Expand Up @@ -6314,4 +6316,44 @@ class E extends B {}
"",
false);
}
public void testGH2646() {
try {
class Counter implements Consumer<SplitPackageBinding> {
int count;
@Override
public void accept(SplitPackageBinding t) {
this.count++;
}
}
Counter counter = new Counter();
SplitPackageBinding.instanceListener = counter;
runConformModuleTest(
new String[] {
"p/X.java",
"package p;\n" +
"public class X {\n" +
" java.sql.Connection con;\n" +
"}",
"module-info.java",
"module mod.one { \n" +
" requires java.se;\n" +
"}",
"q/Y.java",
"package q;\n" +
"public class Y {\n" +
" java.awt.Image image;\n" +
"}"
},
" -9 \"" + OUTPUT_DIR + File.separator + "module-info.java\" "
+ "\"" + OUTPUT_DIR + File.separator + "q/Y.java\" "
+ "\"" + OUTPUT_DIR + File.separator + "p/X.java\" "
+ "-d " + OUTPUT_DIR ,
"",
"",
true);
assertTrue("Number of SplitPackageBinding created: "+counter.count, counter.count <= 20);
} finally {
SplitPackageBinding.instanceListener = null;
}
}
}
13 changes: 13 additions & 0 deletions ...eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/ModuleBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.HashMap;
import java.util.Hashtable;
import java.util.Map;
import java.util.function.Consumer;

import org.eclipse.core.resources.IFile;
import org.eclipse.core.resources.IFolder;
Expand Down Expand Up @@ -49,6 +50,7 @@
import org.eclipse.jdt.core.dom.CompilationUnit;
import org.eclipse.jdt.core.tests.util.Util;
import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
import org.eclipse.jdt.internal.compiler.lookup.SplitPackageBinding;
import org.eclipse.jdt.internal.compiler.lookup.TypeConstants;
import org.eclipse.jdt.internal.core.ClasspathAttribute;
import org.eclipse.jdt.internal.core.ClasspathEntry;
Expand Down Expand Up @@ -7983,6 +7985,15 @@ public void testBug543195() throws CoreException {
public void testBug543701() throws Exception {
IJavaProject p = createJava9Project("p");
String outputDirectory = Util.getOutputDirectory();
class Counter implements Consumer<SplitPackageBinding> {
int count;
@Override
public void accept(SplitPackageBinding t) {
this.count++;
}
}
Counter counter = new Counter();
SplitPackageBinding.instanceListener = counter;
try {
String jar1Path = outputDirectory + File.separator + "lib1.jar";
Util.createJar(new String[] {
Expand Down Expand Up @@ -8026,7 +8037,9 @@ public void testBug543701() throws Exception {
"----------\n" +
"----------\n",
this.problemRequestor);
assertTrue("Number of SplitPackageBinding created: "+counter.count, counter.count <= 80);
} finally {
SplitPackageBinding.instanceListener = null;
deleteProject(p);
// clean up output dir
File outputDir = new File(outputDirectory);
Expand Down

0 comments on commit 685981d

Please sign in to comment.