Revert "Postpone InterfaceProcessor interface modifications"
This reverts commit 9df87f433f3f9de04f6b8090d07d16c0b2e00449.
Revert "Interface processor support concurrency"
This reverts commit 073013114b264f9f7f593115f1fbd17ccd381859.
Revert "Interface Method desugaring class processing concurrent"
This reverts commit 542a41b4ccac4370db4887fed251c4617ac4f217.
Reason for revert: Bot failures
Change-Id: I6c5a51c5abc1d510ba5749798c137293021f6345
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 956cd2c..9d9a502 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,9 +43,7 @@
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;
/**
@@ -342,17 +340,17 @@
private final boolean needsLibraryInfo;
// Mapping from program and classpath classes to their information summary.
- private final Map<DexClass, ClassInfo> classInfo = new ConcurrentHashMap<>();
+ private final Map<DexClass, ClassInfo> classInfo = new IdentityHashMap<>();
// Mapping from library classes to their information summary.
- private final Map<DexLibraryClass, SignaturesInfo> libraryClassInfo = new ConcurrentHashMap<>();
+ private final Map<DexLibraryClass, SignaturesInfo> libraryClassInfo = new IdentityHashMap<>();
// Mapping from arbitrary interfaces to an information summary.
- private final Map<DexClass, SignaturesInfo> interfaceInfo = new ConcurrentHashMap<>();
+ private final Map<DexClass, SignaturesInfo> interfaceInfo = new IdentityHashMap<>();
// Mapping from actual program classes to the synthesized forwarding methods to be created.
private final Map<DexProgramClass, ProgramMethodSet> newSyntheticMethods =
- new ConcurrentHashMap<>();
+ new IdentityHashMap<>();
ClassProcessor(AppView<?> appView, InterfaceMethodRewriter rewriter) {
this.appView = appView;
@@ -377,9 +375,12 @@
@Override
public void process(DexProgramClass clazz, ProgramMethodSet synthesizedMethods) {
- if (!clazz.isInterface()) {
- visitClassInfo(clazz, new ReportingContext(clazz, clazz));
- }
+ visitClassInfo(clazz, new ReportingContext(clazz, clazz));
+ }
+
+ @Override
+ public boolean shouldProcess(DexProgramClass clazz) {
+ return !clazz.isInterface();
}
// We introduce forwarding methods only once all desugaring has been performed to avoid
@@ -815,26 +816,6 @@
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);
@@ -845,7 +826,7 @@
if (clazz.isLibraryClass()) {
return ClassInfo.EMPTY;
}
- return reentrantComputeIfAbsent(classInfo, clazz, key -> visitClassInfoRaw(key, context));
+ return classInfo.computeIfAbsent(clazz, key -> visitClassInfoRaw(key, context));
}
private ClassInfo visitClassInfoRaw(DexClass clazz, ReportingContext context) {
@@ -876,8 +857,7 @@
private SignaturesInfo visitLibraryClassInfo(DexClass clazz) {
assert !clazz.isInterface();
return clazz.isLibraryClass()
- ? reentrantComputeIfAbsent(
- libraryClassInfo, clazz.asLibraryClass(), this::visitLibraryClassInfoRaw)
+ ? libraryClassInfo.computeIfAbsent(clazz.asLibraryClass(), this::visitLibraryClassInfoRaw)
: SignaturesInfo.EMPTY;
}
@@ -899,8 +879,7 @@
if (iface.isLibraryClass() && ignoreLibraryInfo()) {
return SignaturesInfo.EMPTY;
}
- return reentrantComputeIfAbsent(
- interfaceInfo, iface, key -> visitInterfaceInfoRaw(key, context));
+ return interfaceInfo.computeIfAbsent(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 e8f4756..0a56ddb 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,10 +274,15 @@
}
@Override
+ public boolean shouldProcess(DexProgramClass clazz) {
+ return appView.options().isDesugaredLibraryCompilation()
+ && rewriter.isEmulatedInterface(clazz.type);
+ }
+
+ @Override
public void process(DexProgramClass emulatedInterface, ProgramMethodSet synthesizedMethods) {
- if (!appView.options().isDesugaredLibraryCompilation()
- || !rewriter.isEmulatedInterface(emulatedInterface.type)
- || appView.isAlreadyLibraryDesugared(emulatedInterface)) {
+ assert rewriter.isEmulatedInterface(emulatedInterface.type);
+ if (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 c429fc7..821ff44 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,6 +10,8 @@
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 0661c1d..c45a1e0 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,14 +74,9 @@
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;
@@ -133,8 +128,9 @@
// The emulatedMethod set is there to avoid doing the emulated look-up too often.
private final Set<DexString> emulatedMethods = Sets.newIdentityHashSet();
- // All forwarding methods and all throwing methods generated during desugaring.
- private final ProgramMethodSet synthesizedMethods = ProgramMethodSet.createConcurrent();
+ // 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();
// Caches default interface method info for already processed interfaces.
private final Map<DexType, DefaultMethodsHelper.Collection> cache = new ConcurrentHashMap<>();
@@ -981,17 +977,16 @@
// classes if needed.
InterfaceProcessor interfaceProcessor = new InterfaceProcessor(appView, this);
- // 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);
+ // TODO(b/183998768): Merge the following loops into a single one.
+ process(emulatedInterfaceProcessor, builder, flavour);
+ process(classProcessor, builder, flavour);
+ process(interfaceProcessor, builder, flavour);
- SortedProgramMethodSet sortedSynthesizedMethods = SortedProgramMethodSet.create();
- sortedSynthesizedMethods.addAll(synthesizedMethods);
- converter.processMethodsConcurrently(sortedSynthesizedMethods, executorService);
+ classProcessor.finalizeProcessing(builder, synthesizedMethods);
+ interfaceProcessor.finalizeProcessing(builder, synthesizedMethods);
+ emulatedInterfaceProcessor.finalizeProcessing(builder, synthesizedMethods);
+
+ converter.processMethodsConcurrently(synthesizedMethods, executorService);
// Cached data is not needed any more.
clear();
@@ -1009,29 +1004,9 @@
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)) {
+ if (shouldProcess(clazz, flavour) && processor.shouldProcess(clazz)) {
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 2fd96c2..0bc86a3 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,7 +60,6 @@
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.
@@ -73,34 +72,36 @@
private final AppView<?> appView;
private final InterfaceMethodRewriter rewriter;
- private final Map<DexProgramClass, PostProcessingInterfaceInfo> postProcessingInterfaceInfos =
- new ConcurrentHashMap<>();
+ private final InterfaceProcessorNestedGraphLens.Builder graphLensBuilder;
// All created companion classes indexed by interface type.
- final Map<DexClass, DexProgramClass> syntheticClasses = new ConcurrentHashMap<>();
+ final Map<DexClass, DexProgramClass> syntheticClasses = new IdentityHashMap<>();
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) {
- if (!iface.isInterface()) {
- return;
- }
-
+ assert iface.isInterface();
// 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);
+ processVirtualInterfaceMethods(iface, companionMethods, graphLensBuilder);
// Process static and private methods, move them into companion class as well,
// make private instance methods public static.
- processDirectInterfaceMethods(iface, companionMethods);
+ processDirectInterfaceMethods(iface, companionMethods, graphLensBuilder);
if (companionMethods.isEmpty()) {
return; // No methods to create, companion class not needed.
@@ -220,7 +221,10 @@
}
private void processVirtualInterfaceMethods(
- DexProgramClass iface, List<DexEncodedMethod> companionMethods) {
+ DexProgramClass iface,
+ List<DexEncodedMethod> companionMethods,
+ InterfaceProcessorNestedGraphLens.Builder graphLensBuilder) {
+ List<DexEncodedMethod> newVirtualMethods = new ArrayList<>();
for (ProgramMethod method : iface.virtualProgramMethods()) {
DexEncodedMethod virtual = method.getDefinition();
if (rewriter.isDefaultMethod(virtual)) {
@@ -256,21 +260,28 @@
code,
true);
implMethod.copyMetadata(virtual);
- getPostProcessingInterfaceInfo(iface)
- .mapDefaultMethodToCompanionMethod(virtual, implMethod);
+ virtual.setDefaultInterfaceMethodImplementation(implMethod);
companionMethods.add(implMethod);
- getPostProcessingInterfaceInfo(iface)
- .moveMethod(method.getReference(), implMethod.getReference());
+ graphLensBuilder.recordCodeMovedToCompanionClass(
+ method.getReference(), implMethod.getReference());
}
- if (!interfaceMethodRemovalChangesApi(virtual, iface)) {
- getPostProcessingInterfaceInfo(iface).setHasBridgesToRemove();
+ // 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));
+ }
}
private void processDirectInterfaceMethods(
- DexProgramClass iface, List<DexEncodedMethod> companionMethods) {
+ DexProgramClass iface,
+ List<DexEncodedMethod> companionMethods,
+ InterfaceProcessorNestedGraphLens.Builder graphLensBuilder) {
DexEncodedMethod clinit = null;
for (ProgramMethod method : iface.directProgramMethods()) {
DexEncodedMethod definition = method.getDefinition();
@@ -311,7 +322,7 @@
true);
implMethod.copyMetadata(definition);
companionMethods.add(implMethod);
- getPostProcessingInterfaceInfo(iface).moveMethod(oldMethod, companionMethod);
+ graphLensBuilder.move(oldMethod, companionMethod);
continue;
}
@@ -343,18 +354,9 @@
true);
implMethod.copyMetadata(definition);
companionMethods.add(implMethod);
- getPostProcessingInterfaceInfo(iface).moveMethod(oldMethod, companionMethod);
+ graphLensBuilder.move(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);
@@ -443,57 +445,9 @@
&& !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 = postProcessInterfaces();
+ InterfaceProcessorNestedGraphLens graphLens = graphLensBuilder.build(appView);
if (appView.enableWholeProgramOptimizations() && graphLens != null) {
appView.setGraphLens(graphLens);
}
@@ -507,65 +461,6 @@
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 {
