Reland "Postpone InterfaceProcessor interface modifications"
This reverts commit f7024558c818d88df3facc8528f973c298b42728.
Bug: 183998768
Change-Id: I3a0f126f2220ec8f65559db5adcbfd3917d66b81
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/itf/ClassProcessor.java b/src/main/java/com/android/tools/r8/ir/desugar/itf/ClassProcessor.java
index 9d9a502..956cd2c 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/itf/ClassProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/itf/ClassProcessor.java
@@ -43,7 +43,9 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Consumer;
+import java.util.function.Function;
import org.objectweb.asm.Opcodes;
/**
@@ -340,17 +342,17 @@
private final boolean needsLibraryInfo;
// Mapping from program and classpath classes to their information summary.
- private final Map<DexClass, ClassInfo> classInfo = new IdentityHashMap<>();
+ private final Map<DexClass, ClassInfo> classInfo = new ConcurrentHashMap<>();
// Mapping from library classes to their information summary.
- private final Map<DexLibraryClass, SignaturesInfo> libraryClassInfo = new IdentityHashMap<>();
+ private final Map<DexLibraryClass, SignaturesInfo> libraryClassInfo = new ConcurrentHashMap<>();
// Mapping from arbitrary interfaces to an information summary.
- private final Map<DexClass, SignaturesInfo> interfaceInfo = new IdentityHashMap<>();
+ private final Map<DexClass, SignaturesInfo> interfaceInfo = new ConcurrentHashMap<>();
// Mapping from actual program classes to the synthesized forwarding methods to be created.
private final Map<DexProgramClass, ProgramMethodSet> newSyntheticMethods =
- new IdentityHashMap<>();
+ new ConcurrentHashMap<>();
ClassProcessor(AppView<?> appView, InterfaceMethodRewriter rewriter) {
this.appView = appView;
@@ -375,12 +377,9 @@
@Override
public void process(DexProgramClass clazz, ProgramMethodSet synthesizedMethods) {
- visitClassInfo(clazz, new ReportingContext(clazz, clazz));
- }
-
- @Override
- public boolean shouldProcess(DexProgramClass clazz) {
- return !clazz.isInterface();
+ if (!clazz.isInterface()) {
+ visitClassInfo(clazz, new ReportingContext(clazz, clazz));
+ }
}
// We introduce forwarding methods only once all desugaring has been performed to avoid
@@ -816,6 +815,26 @@
return clazz;
}
+ // We cannot use ConcurrentHashMap computeIfAbsent because the calls are recursive ending in
+ // concurrent modification of the ConcurrentHashMap while performing computeIfAbsent.
+ private <C extends DexClass, I> I reentrantComputeIfAbsent(
+ Map<C, I> infoMap, C clazz, Function<C, I> supplier) {
+ // Try to avoid the synchronization when possible.
+ I computedInfo = infoMap.get(clazz);
+ if (computedInfo != null) {
+ return computedInfo;
+ }
+ synchronized (clazz) {
+ computedInfo = infoMap.get(clazz);
+ if (computedInfo != null) {
+ return computedInfo;
+ }
+ computedInfo = supplier.apply(clazz);
+ infoMap.put(clazz, computedInfo);
+ return computedInfo;
+ }
+ }
+
private ClassInfo visitClassInfo(DexType type, ReportingContext context) {
DexClass clazz = definitionOrNull(type, context);
return clazz == null ? ClassInfo.EMPTY : visitClassInfo(clazz, context);
@@ -826,7 +845,7 @@
if (clazz.isLibraryClass()) {
return ClassInfo.EMPTY;
}
- return classInfo.computeIfAbsent(clazz, key -> visitClassInfoRaw(key, context));
+ return reentrantComputeIfAbsent(classInfo, clazz, key -> visitClassInfoRaw(key, context));
}
private ClassInfo visitClassInfoRaw(DexClass clazz, ReportingContext context) {
@@ -857,7 +876,8 @@
private SignaturesInfo visitLibraryClassInfo(DexClass clazz) {
assert !clazz.isInterface();
return clazz.isLibraryClass()
- ? libraryClassInfo.computeIfAbsent(clazz.asLibraryClass(), this::visitLibraryClassInfoRaw)
+ ? reentrantComputeIfAbsent(
+ libraryClassInfo, clazz.asLibraryClass(), this::visitLibraryClassInfoRaw)
: SignaturesInfo.EMPTY;
}
@@ -879,7 +899,8 @@
if (iface.isLibraryClass() && ignoreLibraryInfo()) {
return SignaturesInfo.EMPTY;
}
- return interfaceInfo.computeIfAbsent(iface, key -> visitInterfaceInfoRaw(key, context));
+ return reentrantComputeIfAbsent(
+ interfaceInfo, iface, key -> visitInterfaceInfoRaw(key, context));
}
private SignaturesInfo visitInterfaceInfoRaw(DexClass iface, ReportingContext context) {
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/itf/EmulatedInterfaceProcessor.java b/src/main/java/com/android/tools/r8/ir/desugar/itf/EmulatedInterfaceProcessor.java
index 0a56ddb..e8f4756 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/itf/EmulatedInterfaceProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/itf/EmulatedInterfaceProcessor.java
@@ -274,15 +274,10 @@
}
@Override
- public boolean shouldProcess(DexProgramClass clazz) {
- return appView.options().isDesugaredLibraryCompilation()
- && rewriter.isEmulatedInterface(clazz.type);
- }
-
- @Override
public void process(DexProgramClass emulatedInterface, ProgramMethodSet synthesizedMethods) {
- assert rewriter.isEmulatedInterface(emulatedInterface.type);
- if (appView.isAlreadyLibraryDesugared(emulatedInterface)) {
+ if (!appView.options().isDesugaredLibraryCompilation()
+ || !rewriter.isEmulatedInterface(emulatedInterface.type)
+ || appView.isAlreadyLibraryDesugared(emulatedInterface)) {
return;
}
generateEmulateInterfaceLibrary(emulatedInterface);
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceDesugaringProcessor.java b/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceDesugaringProcessor.java
index 821ff44..c429fc7 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceDesugaringProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceDesugaringProcessor.java
@@ -10,8 +10,6 @@
public interface InterfaceDesugaringProcessor {
- boolean shouldProcess(DexProgramClass clazz);
-
// The process phase can be performed before, concurrently or after code desugaring. It can be
// executed concurrently on all classes. Each processing may require to read other classes,
// so this phase cannot modify the classes themselves (for example insertion/removal of methods).
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceMethodRewriter.java b/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceMethodRewriter.java
index c45a1e0..0661c1d 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceMethodRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceMethodRewriter.java
@@ -74,9 +74,14 @@
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.IteratorUtils;
import com.android.tools.r8.utils.Pair;
+import com.android.tools.r8.utils.ThreadUtils;
+import com.android.tools.r8.utils.collections.ProgramMethodSet;
import com.android.tools.r8.utils.collections.SortedProgramMethodSet;
import com.android.tools.r8.utils.structural.Ordered;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
+import java.util.List;
import java.util.ListIterator;
import java.util.Map;
import java.util.Set;
@@ -128,9 +133,8 @@
// The emulatedMethod set is there to avoid doing the emulated look-up too often.
private final Set<DexString> emulatedMethods = Sets.newIdentityHashSet();
- // All forwarding methods generated during desugaring. We don't synchronize access
- // to this collection since it is only filled in ClassProcessor running synchronously.
- private final SortedProgramMethodSet synthesizedMethods = SortedProgramMethodSet.create();
+ // All forwarding methods and all throwing methods generated during desugaring.
+ private final ProgramMethodSet synthesizedMethods = ProgramMethodSet.createConcurrent();
// Caches default interface method info for already processed interfaces.
private final Map<DexType, DefaultMethodsHelper.Collection> cache = new ConcurrentHashMap<>();
@@ -977,16 +981,17 @@
// classes if needed.
InterfaceProcessor interfaceProcessor = new InterfaceProcessor(appView, this);
- // TODO(b/183998768): Merge the following loops into a single one.
- process(emulatedInterfaceProcessor, builder, flavour);
- process(classProcessor, builder, flavour);
- process(interfaceProcessor, builder, flavour);
+ // The interface processors must be ordered so that finalization of the processing is performed
+ // in that order. The emulatedInterfaceProcessor has to be last at this point to avoid renaming
+ // emulated interfaces before the other processing.
+ ImmutableList<InterfaceDesugaringProcessor> orderedInterfaceDesugaringProcessors =
+ ImmutableList.of(classProcessor, interfaceProcessor, emulatedInterfaceProcessor);
+ processClassesConcurrently(
+ orderedInterfaceDesugaringProcessors, builder, flavour, executorService);
- classProcessor.finalizeProcessing(builder, synthesizedMethods);
- interfaceProcessor.finalizeProcessing(builder, synthesizedMethods);
- emulatedInterfaceProcessor.finalizeProcessing(builder, synthesizedMethods);
-
- converter.processMethodsConcurrently(synthesizedMethods, executorService);
+ SortedProgramMethodSet sortedSynthesizedMethods = SortedProgramMethodSet.create();
+ sortedSynthesizedMethods.addAll(synthesizedMethods);
+ converter.processMethodsConcurrently(sortedSynthesizedMethods, executorService);
// Cached data is not needed any more.
clear();
@@ -1004,9 +1009,29 @@
return (!clazz.originatesFromDexResource() || flavour == Flavor.IncludeAllResources);
}
+ private void processClassesConcurrently(
+ List<InterfaceDesugaringProcessor> processors,
+ Builder<?> builder,
+ Flavor flavour,
+ ExecutorService executorService)
+ throws ExecutionException {
+ ThreadUtils.processItems(
+ Iterables.filter(
+ builder.getProgramClasses(), (DexProgramClass clazz) -> shouldProcess(clazz, flavour)),
+ clazz -> {
+ for (InterfaceDesugaringProcessor processor : processors) {
+ processor.process(clazz, synthesizedMethods);
+ }
+ },
+ executorService);
+ for (InterfaceDesugaringProcessor processor : processors) {
+ processor.finalizeProcessing(builder, synthesizedMethods);
+ }
+ }
+
private void process(InterfaceDesugaringProcessor processor, Builder<?> builder, Flavor flavour) {
for (DexProgramClass clazz : builder.getProgramClasses()) {
- if (shouldProcess(clazz, flavour) && processor.shouldProcess(clazz)) {
+ if (shouldProcess(clazz, flavour)) {
processor.process(clazz, synthesizedMethods);
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceProcessor.java b/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceProcessor.java
index 0bc86a3..83f051d 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/itf/InterfaceProcessor.java
@@ -60,6 +60,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
import org.objectweb.asm.Opcodes;
// Default and static method interface desugaring processor for interfaces.
@@ -72,36 +73,34 @@
private final AppView<?> appView;
private final InterfaceMethodRewriter rewriter;
- private final InterfaceProcessorNestedGraphLens.Builder graphLensBuilder;
+ private final Map<DexProgramClass, PostProcessingInterfaceInfo> postProcessingInterfaceInfos =
+ new ConcurrentHashMap<>();
// All created companion classes indexed by interface type.
- final Map<DexClass, DexProgramClass> syntheticClasses = new IdentityHashMap<>();
+ final Map<DexClass, DexProgramClass> syntheticClasses = new ConcurrentHashMap<>();
InterfaceProcessor(AppView<?> appView, InterfaceMethodRewriter rewriter) {
this.appView = appView;
this.rewriter = rewriter;
- graphLensBuilder = InterfaceProcessorNestedGraphLens.builder();
- }
-
- @Override
- public boolean shouldProcess(DexProgramClass clazz) {
- return clazz.isInterface();
}
@Override
public void process(DexProgramClass iface, ProgramMethodSet synthesizedMethods) {
- assert iface.isInterface();
+ if (!iface.isInterface()) {
+ return;
+ }
+
// The list of methods to be created in companion class.
List<DexEncodedMethod> companionMethods = new ArrayList<>();
ensureCompanionClassInitializesInterface(iface, companionMethods);
// Process virtual interface methods first.
- processVirtualInterfaceMethods(iface, companionMethods, graphLensBuilder);
+ processVirtualInterfaceMethods(iface, companionMethods);
// Process static and private methods, move them into companion class as well,
// make private instance methods public static.
- processDirectInterfaceMethods(iface, companionMethods, graphLensBuilder);
+ processDirectInterfaceMethods(iface, companionMethods);
if (companionMethods.isEmpty()) {
return; // No methods to create, companion class not needed.
@@ -221,10 +220,7 @@
}
private void processVirtualInterfaceMethods(
- DexProgramClass iface,
- List<DexEncodedMethod> companionMethods,
- InterfaceProcessorNestedGraphLens.Builder graphLensBuilder) {
- List<DexEncodedMethod> newVirtualMethods = new ArrayList<>();
+ DexProgramClass iface, List<DexEncodedMethod> companionMethods) {
for (ProgramMethod method : iface.virtualProgramMethods()) {
DexEncodedMethod virtual = method.getDefinition();
if (rewriter.isDefaultMethod(virtual)) {
@@ -260,28 +256,19 @@
code,
true);
implMethod.copyMetadata(virtual);
- virtual.setDefaultInterfaceMethodImplementation(implMethod);
companionMethods.add(implMethod);
- graphLensBuilder.recordCodeMovedToCompanionClass(
- method.getReference(), implMethod.getReference());
+ getPostProcessingInterfaceInfo(iface)
+ .mapDefaultMethodToCompanionMethod(virtual, implMethod);
}
- // Remove bridge methods.
- if (interfaceMethodRemovalChangesApi(virtual, iface)) {
- newVirtualMethods.add(virtual);
+ if (!interfaceMethodRemovalChangesApi(virtual, iface)) {
+ getPostProcessingInterfaceInfo(iface).setHasBridgesToRemove();
}
}
-
- // If at least one bridge method was removed then update the table.
- if (newVirtualMethods.size() < iface.getMethodCollection().numberOfVirtualMethods()) {
- iface.setVirtualMethods(newVirtualMethods.toArray(DexEncodedMethod.EMPTY_ARRAY));
- }
}
private void processDirectInterfaceMethods(
- DexProgramClass iface,
- List<DexEncodedMethod> companionMethods,
- InterfaceProcessorNestedGraphLens.Builder graphLensBuilder) {
+ DexProgramClass iface, List<DexEncodedMethod> companionMethods) {
DexEncodedMethod clinit = null;
for (ProgramMethod method : iface.directProgramMethods()) {
DexEncodedMethod definition = method.getDefinition();
@@ -322,7 +309,7 @@
true);
implMethod.copyMetadata(definition);
companionMethods.add(implMethod);
- graphLensBuilder.move(oldMethod, companionMethod);
+ getPostProcessingInterfaceInfo(iface).moveMethod(oldMethod, companionMethod);
continue;
}
@@ -354,9 +341,18 @@
true);
implMethod.copyMetadata(definition);
companionMethods.add(implMethod);
- graphLensBuilder.move(oldMethod, companionMethod);
+ getPostProcessingInterfaceInfo(iface).moveMethod(oldMethod, companionMethod);
}
+ boolean hasNonClinitDirectMethods =
+ iface.getMethodCollection().size() != (clinit == null ? 0 : 1);
+ if (hasNonClinitDirectMethods) {
+ getPostProcessingInterfaceInfo(iface).setHasNonClinitDirectMethods();
+ }
+ }
+
+ private void clearDirectMethods(DexProgramClass iface) {
+ DexEncodedMethod clinit = iface.getClassInitializer();
MethodCollection methodCollection = iface.getMethodCollection();
if (clinit != null) {
methodCollection.setSingleDirectMethod(clinit);
@@ -445,9 +441,57 @@
&& !rewriter.factory.isClassConstructor(method.getReference());
}
+ private InterfaceProcessorNestedGraphLens postProcessInterfaces() {
+ InterfaceProcessorNestedGraphLens.Builder graphLensBuilder =
+ InterfaceProcessorNestedGraphLens.builder();
+ postProcessingInterfaceInfos.forEach(
+ (iface, info) -> {
+ if (info.hasNonClinitDirectMethods()) {
+ clearDirectMethods(iface);
+ }
+ if (info.hasDefaultMethodsToImplementationMap()) {
+ info.getDefaultMethodsToImplementation()
+ .forEach(
+ (defaultMethod, companionMethod) -> {
+ defaultMethod.setDefaultInterfaceMethodImplementation(companionMethod);
+ graphLensBuilder.recordCodeMovedToCompanionClass(
+ defaultMethod.getReference(), companionMethod.getReference());
+ });
+ }
+ if (info.hasMethodsToMove()) {
+ info.getMethodsToMove().forEach(graphLensBuilder::move);
+ }
+ if (info.hasBridgesToRemove()) {
+ removeBridges(iface);
+ }
+ });
+ return graphLensBuilder.build(appView);
+ }
+
+ private void removeBridges(DexProgramClass iface) {
+ List<DexEncodedMethod> newVirtualMethods = new ArrayList<>();
+ for (ProgramMethod method : iface.virtualProgramMethods()) {
+ DexEncodedMethod virtual = method.getDefinition();
+ // Remove bridge methods.
+ if (interfaceMethodRemovalChangesApi(virtual, iface)) {
+ newVirtualMethods.add(virtual);
+ }
+ }
+
+ // If at least one bridge method was removed then update the table.
+ if (newVirtualMethods.size() < iface.getMethodCollection().numberOfVirtualMethods()) {
+ iface.setVirtualMethods(newVirtualMethods.toArray(DexEncodedMethod.EMPTY_ARRAY));
+ } else {
+ assert false
+ : "Interface "
+ + iface
+ + " was analysed as having bridges to remove, but no bridges were found.";
+ }
+ }
+
@Override
public void finalizeProcessing(Builder<?> builder, ProgramMethodSet synthesizedMethods) {
- InterfaceProcessorNestedGraphLens graphLens = graphLensBuilder.build(appView);
+ InterfaceProcessorNestedGraphLens graphLens = postProcessInterfaces();
if (appView.enableWholeProgramOptimizations() && graphLens != null) {
appView.setGraphLens(graphLens);
}
@@ -461,6 +505,65 @@
new InterfaceMethodRewriterFixup(appView, graphLens).run();
}
+ private PostProcessingInterfaceInfo getPostProcessingInterfaceInfo(DexProgramClass iface) {
+ return postProcessingInterfaceInfos.computeIfAbsent(
+ iface, ignored -> new PostProcessingInterfaceInfo());
+ }
+
+ static class PostProcessingInterfaceInfo {
+ private Map<DexEncodedMethod, DexEncodedMethod> defaultMethodsToImplementation;
+ private Map<DexMethod, DexMethod> methodsToMove;
+ private boolean hasNonClinitDirectMethods;
+ private boolean hasBridgesToRemove;
+
+ public void mapDefaultMethodToCompanionMethod(
+ DexEncodedMethod defaultMethod, DexEncodedMethod companionMethod) {
+ if (defaultMethodsToImplementation == null) {
+ defaultMethodsToImplementation = new IdentityHashMap<>();
+ }
+ defaultMethodsToImplementation.put(defaultMethod, companionMethod);
+ }
+
+ public Map<DexEncodedMethod, DexEncodedMethod> getDefaultMethodsToImplementation() {
+ return defaultMethodsToImplementation;
+ }
+
+ boolean hasDefaultMethodsToImplementationMap() {
+ return defaultMethodsToImplementation != null;
+ }
+
+ public void moveMethod(DexMethod ifaceMethod, DexMethod companionMethod) {
+ if (methodsToMove == null) {
+ methodsToMove = new IdentityHashMap<>();
+ }
+ methodsToMove.put(ifaceMethod, companionMethod);
+ }
+
+ public Map<DexMethod, DexMethod> getMethodsToMove() {
+ return methodsToMove;
+ }
+
+ public boolean hasMethodsToMove() {
+ return methodsToMove != null;
+ }
+
+ boolean hasNonClinitDirectMethods() {
+ return hasNonClinitDirectMethods;
+ }
+
+ void setHasNonClinitDirectMethods() {
+ hasNonClinitDirectMethods = true;
+ }
+
+ boolean hasBridgesToRemove() {
+ return hasBridgesToRemove;
+ }
+
+ void setHasBridgesToRemove() {
+ hasBridgesToRemove = true;
+ }
+ }
+
// Specific lens which remaps invocation types to static since all rewrites performed here
// are to static companion methods.
public static class InterfaceProcessorNestedGraphLens extends NestedGraphLens {
