Use TopDownTraversal in MethodMinification
Bug: 213415674
Change-Id: Ib39cd680ddbdaf089d6633788ac96153ca742eaa
diff --git a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
index cd6c271..8a8d804 100644
--- a/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/MethodNameMinifier.java
@@ -11,7 +11,7 @@
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.graph.MethodAccessInfoCollection;
import com.android.tools.r8.graph.MethodResolutionResult;
-import com.android.tools.r8.graph.SubtypingInfo;
+import com.android.tools.r8.graph.TopDownClassHierarchyTraversal;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.ThreadUtils;
@@ -123,7 +123,6 @@
}
private final AppView<AppInfoWithLiveness> appView;
- private final SubtypingInfo subtypingInfo;
private final MemberNamingStrategy strategy;
private final Map<DexMethod, DexString> renaming = new IdentityHashMap<>();
@@ -141,12 +140,11 @@
MethodNameMinifier(
AppView<AppInfoWithLiveness> appView,
- SubtypingInfo subtypingInfo,
MemberNamingStrategy strategy) {
this.appView = appView;
- this.subtypingInfo = subtypingInfo;
this.strategy = strategy;
rootReservationState = MethodReservationState.createRoot(getReservationKeyTransform());
+ reservationStates.put(null, rootReservationState);
rootNamingState =
MethodNamingState.createRoot(getNamingKeyTransform(), strategy, rootReservationState);
namingStates.put(null, rootNamingState);
@@ -202,7 +200,8 @@
timing.end();
// Phase 4: Assign names top-down by traversing the subtype hierarchy.
timing.begin("Phase 4");
- assignNamesToClassesMethods(appView.dexItemFactory().objectType, rootNamingState);
+ assignNamesToClassesMethods();
+ renameMethodsInUnrelatedClasspathClasses();
timing.end();
timing.begin("Phase 5: non-rebound references");
renameNonReboundReferences(executorService);
@@ -211,21 +210,46 @@
return new MethodRenaming(renaming);
}
- private void assignNamesToClassesMethods(DexType type, MethodNamingState<?> parentNamingState) {
- MethodReservationState<?> reservationState =
- reservationStates.get(frontiers.getOrDefault(type, type));
- assert reservationState != null : "Could not find reservation state for " + type.toString();
- MethodNamingState<?> namingState =
- namingStates.computeIfAbsent(
- type, ignore -> parentNamingState.createChild(reservationState));
- DexClass holder = appView.definitionFor(type);
- if (holder != null && strategy.allowMemberRenaming(holder)) {
- for (DexEncodedMethod method : holder.allMethodsSorted()) {
- assignNameToMethod(holder, method, namingState);
- }
- }
- for (DexType subType : subtypingInfo.allImmediateExtendsSubtypes(type)) {
- assignNamesToClassesMethods(subType, namingState);
+ private void assignNamesToClassesMethods() {
+ TopDownClassHierarchyTraversal.forAllClasses(appView)
+ .excludeInterfaces()
+ .visit(
+ appView.appInfo().classes(),
+ clazz -> {
+ DexType type = clazz.type;
+ MethodReservationState<?> reservationState =
+ reservationStates.get(frontiers.getOrDefault(type, type));
+ assert reservationState != null
+ : "Could not find reservation state for " + type.toString();
+ MethodNamingState<?> namingState =
+ namingStates.computeIfAbsent(
+ type,
+ ignore ->
+ namingStates
+ .getOrDefault(clazz.superType, rootNamingState)
+ .createChild(reservationState));
+ DexClass holder = appView.definitionFor(type);
+ if (holder != null && strategy.allowMemberRenaming(holder)) {
+ for (DexEncodedMethod method : holder.allMethodsSorted()) {
+ assignNameToMethod(holder, method, namingState);
+ }
+ }
+ });
+ }
+
+ private void renameMethodsInUnrelatedClasspathClasses() {
+ if (appView.options().getProguardConfiguration().hasApplyMappingFile()) {
+ appView
+ .appInfo()
+ .forEachReferencedClasspathClass(
+ clazz -> {
+ for (DexEncodedMethod method : clazz.methods()) {
+ DexString reservedName = strategy.getReservedName(method, clazz);
+ if (reservedName != null && reservedName != method.getReference().name) {
+ renaming.put(method.getReference(), reservedName);
+ }
+ }
+ });
}
}
@@ -248,42 +272,30 @@
}
private void reserveNamesInClasses() {
- reserveNamesInClasses(
- appView.dexItemFactory().objectType,
- appView.dexItemFactory().objectType,
- rootReservationState);
+ TopDownClassHierarchyTraversal.forAllClasses(appView)
+ .visit(
+ appView.appInfo().classes(),
+ clazz -> {
+ DexType type = clazz.type;
+ DexType frontier = frontiers.getOrDefault(clazz.superType, type);
+ if (frontier != type || clazz.isProgramClass()) {
+ DexType existingValue = frontiers.put(clazz.type, frontier);
+ assert existingValue == null;
+ }
+ // If this is not a program class (or effectively a library class as it is missing)
+ // move the frontier forward. This will ensure all reservations are put on the library
+ // or classpath frontier for the program path.
+ allocateReservationStateAndReserve(
+ type,
+ frontier,
+ reservationStates.getOrDefault(clazz.superType, rootReservationState));
+ });
}
- private void reserveNamesInClasses(
- DexType type, DexType libraryFrontier, MethodReservationState<?> parentReservationState) {
- assert !appView.isInterface(type).isTrue();
-
- MethodReservationState<?> reservationState =
- allocateReservationStateAndReserve(type, libraryFrontier, parentReservationState);
-
- // If this is not a program class (or effectively a library class as it is missing) move the
- // frontier forward. This will ensure all reservations are put on the library or classpath
- // frontier for the program path.
- DexClass holder = appView.definitionFor(type);
- for (DexType subtype : subtypingInfo.allImmediateExtendsSubtypes(type)) {
- reserveNamesInClasses(
- subtype,
- holder == null || holder.isNotProgramClass() ? subtype : libraryFrontier,
- reservationState);
- }
- }
-
- private MethodReservationState<?> allocateReservationStateAndReserve(
+ private void allocateReservationStateAndReserve(
DexType type, DexType frontier, MethodReservationState<?> parent) {
- assert parent != null;
-
- if (frontier != type) {
- frontiers.put(type, frontier);
- }
-
MethodReservationState<?> state =
reservationStates.computeIfAbsent(frontier, ignore -> parent.createChild());
-
DexClass holder = appView.definitionFor(type);
if (holder != null) {
for (DexEncodedMethod method : shuffleMethods(holder.methods(), appView.options())) {
@@ -293,8 +305,6 @@
}
}
}
-
- return state;
}
private MethodNamingState<?> getOrAllocateMethodNamingStates(DexType type) {
diff --git a/src/main/java/com/android/tools/r8/naming/Minifier.java b/src/main/java/com/android/tools/r8/naming/Minifier.java
index e7106a6..738be08 100644
--- a/src/main/java/com/android/tools/r8/naming/Minifier.java
+++ b/src/main/java/com/android/tools/r8/naming/Minifier.java
@@ -66,7 +66,7 @@
MemberNamingStrategy minifyMembers = new MinifierMemberNamingStrategy(appView);
timing.begin("MinifyMethods");
MethodRenaming methodRenaming =
- new MethodNameMinifier(appView, subtypingInfo, minifyMembers)
+ new MethodNameMinifier(appView, minifyMembers)
.computeRenaming(interfaces, executorService, timing);
timing.end();
diff --git a/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java b/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
index 5c07e4a..7ee0681 100644
--- a/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ProguardMapMinifier.java
@@ -147,7 +147,7 @@
new ApplyMappingMemberNamingStrategy(appView, memberNames);
timing.begin("MinifyMethods");
MethodRenaming methodRenaming =
- new MethodNameMinifier(appView, subtypingInfo, nameStrategy)
+ new MethodNameMinifier(appView, nameStrategy)
.computeRenaming(interfaces, executorService, timing);
// Amend the method renamings with the default interface methods.
methodRenaming.renaming.putAll(defaultInterfaceMethodImplementationNames);
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterDevirtualizationTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterDevirtualizationTest.java
index ccdaae7..99554bf 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterDevirtualizationTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterDevirtualizationTest.java
@@ -4,9 +4,12 @@
package com.android.tools.r8.naming.applymapping;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentAndNotRenamed;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentAndRenamed;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
+import com.android.tools.r8.NoMethodStaticizing;
import com.android.tools.r8.R8TestCompileResult;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
@@ -38,6 +41,7 @@
public static class LibClassA implements LibInterfaceA {
@Override
+ @NoMethodStaticizing
public void foo() {
System.out.println("LibClassA::foo");
}
@@ -47,10 +51,12 @@
public static class LibClassB implements LibInterfaceB {
@Override
+ @NoMethodStaticizing
public void foo() {
System.out.println("LibClassB::foo");
}
+ @NoMethodStaticizing
public void bar() {
System.out.println("LibClassB::bar");
}
@@ -74,7 +80,7 @@
}
}
- private static final Class<?>[] LIBRARY_CLASSES = {
+ private static final Class<?>[] CLASSPATH_CLASSES = {
LibInterfaceA.class, LibInterfaceB.class, LibClassA.class, LibClassB.class
};
@@ -92,7 +98,7 @@
public void runOnJvm() throws Throwable {
Assume.assumeTrue(parameters.isCfRuntime());
testForJvm()
- .addProgramClasses(LIBRARY_CLASSES)
+ .addProgramClasses(CLASSPATH_CLASSES)
.addProgramClasses(PROGRAM_CLASSES)
.run(parameters.getRuntime(), ProgramClass.class)
.assertSuccessWithOutput(EXPECTED_OUTPUT);
@@ -102,15 +108,18 @@
public void devirtualizingNoRenamingOfOverriddenNotKeptInterfaceMethods() throws Exception {
R8TestCompileResult libraryResult =
testForR8(parameters.getBackend())
- .addProgramClasses(LIBRARY_CLASSES)
- .addKeepClassAndMembersRulesWithAllowObfuscation(LibClassA.class, LibClassB.class)
+ .addProgramClasses(CLASSPATH_CLASSES)
+ .addKeepClassAndMembersRulesWithAllowObfuscation(LibClassA.class)
+ .addKeepMainRule(LibClassB.class)
+ .addKeepClassAndDefaultConstructor(LibClassB.class)
.addOptionsModification(options -> options.inlinerOptions().enableInlining = false)
+ .enableNoMethodStaticizingAnnotations()
.setMinApi(parameters.getApiLevel())
.compile();
CodeInspector inspector = libraryResult.inspector();
- assertThat(inspector.clazz(LibClassA.class), isPresent());
- assertThat(inspector.clazz(LibClassB.class), isPresent());
+ assertThat(inspector.clazz(LibClassA.class), isPresentAndRenamed());
+ assertThat(inspector.clazz(LibClassB.class), isPresentAndNotRenamed());
// LibInterfaceX should have been moved into LibClassX.
assertThat(inspector.clazz(LibInterfaceA.class), not(isPresent()));
@@ -121,8 +130,7 @@
.noMinification()
.addProgramClasses(PROGRAM_CLASSES)
.addApplyMapping(libraryResult.getProguardMap())
- .addLibraryClasses(LIBRARY_CLASSES)
- .addDefaultRuntimeLibrary(parameters)
+ .addClasspathClasses(CLASSPATH_CLASSES)
.setMinApi(parameters.getApiLevel())
.compile()
.addRunClasspathFiles(libraryResult.writeToZip())
@@ -134,19 +142,20 @@
public void devirtualizingNoRenamingOfOverriddenKeptInterfaceMethods() throws Exception {
R8TestCompileResult libraryResult =
testForR8(parameters.getBackend())
- .addProgramClasses(LIBRARY_CLASSES)
+ .addProgramClasses(CLASSPATH_CLASSES)
.addKeepClassAndMembersRulesWithAllowObfuscation(
LibClassA.class, LibClassB.class, LibInterfaceA.class)
.addOptionsModification(options -> options.inlinerOptions().enableInlining = false)
+ .enableNoMethodStaticizingAnnotations()
.setMinApi(parameters.getApiLevel())
.compile();
CodeInspector inspector = libraryResult.inspector();
- assertThat(inspector.clazz(LibClassA.class), isPresent());
- assertThat(inspector.clazz(LibClassB.class), isPresent());
+ assertThat(inspector.clazz(LibClassA.class), isPresentAndRenamed());
+ assertThat(inspector.clazz(LibClassB.class), isPresentAndRenamed());
// LibInterfaceA is now kept.
- assertThat(inspector.clazz(LibInterfaceA.class), isPresent());
+ assertThat(inspector.clazz(LibInterfaceA.class), isPresentAndRenamed());
assertThat(inspector.clazz(LibInterfaceB.class), not(isPresent()));
testForR8(parameters.getBackend())
@@ -154,8 +163,7 @@
.noMinification()
.addProgramClasses(PROGRAM_CLASSES)
.addApplyMapping(libraryResult.getProguardMap())
- .addLibraryClasses(LIBRARY_CLASSES)
- .addDefaultRuntimeLibrary(parameters)
+ .addClasspathClasses(CLASSPATH_CLASSES)
.setMinApi(parameters.getApiLevel())
.compile()
.addRunClasspathFiles(libraryResult.writeToZip())
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingInterfaceInvokeTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingInterfaceInvokeTest.java
index a1ef87f..b4b6807 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingInterfaceInvokeTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingInterfaceInvokeTest.java
@@ -4,13 +4,10 @@
package com.android.tools.r8.naming.applymapping;
-import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.R8TestCompileResult;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import java.io.IOException;
-import java.util.concurrent.ExecutionException;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -57,7 +54,7 @@
@Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimes().build();
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
}
public ApplyMappingInterfaceInvokeTest(TestParameters parameters) {
@@ -65,14 +62,13 @@
}
@Test
- public void testInvokeVirtual()
- throws IOException, CompilationFailedException, ExecutionException {
+ public void testInvokeVirtual() throws Exception {
Class<?>[] classPathClasses = {I.class, A.class, B.class, C.class};
R8TestCompileResult libraryResult =
testForR8(parameters.getBackend())
.addProgramClasses(classPathClasses)
.addKeepAllClassesRuleWithAllowObfuscation()
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.compile();
testForR8(parameters.getBackend())
.addClasspathClasses(classPathClasses)
@@ -80,7 +76,7 @@
.noMinification()
.noTreeShaking()
.addApplyMapping(libraryResult.getProguardMap())
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.compile()
.addRunClasspathFiles(libraryResult.writeToZip())
.run(parameters.getRuntime(), TestApp.class)
