Merge "Apply name reflection optimization to seemingly top-level classes only."
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
new file mode 100644
index 0000000..20faecf
--- /dev/null
+++ b/PRESUBMIT.py
@@ -0,0 +1,15 @@
+# Copyright (c) 2018, the R8 project authors. Please see the AUTHORS file
+# for details. All rights reserved. Use of this source code is governed by a
+# BSD-style license that can be found in the LICENSE file.
+
+def CheckDoNotMerge(input_api, output_api):
+ for l in input_api.change.FullDescriptionText().splitlines():
+ if l.lower().startswith('do not merge'):
+ msg = 'Your cl contains: \'Do not merge\' - this will break WIP bots'
+ return [output_api.PresubmitPromptWarning(msg, [])]
+ return []
+
+def CheckChangeOnUpload(input_api, output_api):
+ results = []
+ results.extend(CheckDoNotMerge(input_api, output_api))
+ return results
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java
index 6d00ebb..7f77c6b 100644
--- a/src/main/java/com/android/tools/r8/R8.java
+++ b/src/main/java/com/android/tools/r8/R8.java
@@ -14,6 +14,7 @@
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexApplication;
import com.android.tools.r8.graph.DexCallSite;
+import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.graph.GraphLense;
@@ -527,6 +528,8 @@
return;
}
+ assert application.classes().stream().allMatch(DexClass::isValid);
+
// Generate the resulting application resources.
writeApplication(
executorService,
diff --git a/src/main/java/com/android/tools/r8/graph/DexClass.java b/src/main/java/com/android/tools/r8/graph/DexClass.java
index d79276e..5a4b204 100644
--- a/src/main/java/com/android/tools/r8/graph/DexClass.java
+++ b/src/main/java/com/android/tools/r8/graph/DexClass.java
@@ -524,4 +524,9 @@
return getKotlinInfo() != null;
}
+ public boolean isValid() {
+ assert !isInterface()
+ || Arrays.stream(virtualMethods()).noneMatch(method -> method.accessFlags.isFinal());
+ return true;
+ }
}
diff --git a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
index 952038a..e9966f5 100644
--- a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
+++ b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
@@ -68,7 +68,7 @@
// ReferenceTypeLattice canonicalization.
private final ConcurrentHashMap<DexType, ReferenceTypeLatticeElement>
referenceTypeLatticeElements = new ConcurrentHashMap<>();
- public static final LRUCacheTable<Set<DexType>, Set<DexType>, Set<DexType>>
+ public final LRUCacheTable<Set<DexType>, Set<DexType>, Set<DexType>>
leastUpperBoundOfInterfacesTable = LRUCacheTable.create(8, 8);
boolean sorted = false;
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 281d41b..20c04ab 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -56,6 +56,7 @@
import com.android.tools.r8.ir.optimize.Outliner;
import com.android.tools.r8.ir.optimize.PeepholeOptimizer;
import com.android.tools.r8.ir.optimize.RedundantFieldLoadElimination;
+import com.android.tools.r8.ir.optimize.ReflectionOptimizer;
import com.android.tools.r8.ir.optimize.UninstantiatedTypeOptimization;
import com.android.tools.r8.ir.optimize.classinliner.ClassInliner;
import com.android.tools.r8.ir.optimize.lambda.LambdaMerger;
@@ -904,7 +905,7 @@
if (appInfo.hasLiveness()) {
// Reflection optimization 1. getClass() -> const-class
- codeRewriter.rewriteGetClass(code);
+ ReflectionOptimizer.rewriteGetClass(appInfo.withLiveness(), code);
}
if (!isDebugMode) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
index 6580b29..0b54a4d 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -52,7 +52,6 @@
import com.android.tools.r8.ir.code.CheckCast;
import com.android.tools.r8.ir.code.Cmp;
import com.android.tools.r8.ir.code.Cmp.Bias;
-import com.android.tools.r8.ir.code.ConstClass;
import com.android.tools.r8.ir.code.ConstInstruction;
import com.android.tools.r8.ir.code.ConstNumber;
import com.android.tools.r8.ir.code.ConstString;
@@ -1962,67 +1961,6 @@
return converter.definitionFor(type);
}
- // Rewrite getClass() call to const-class if the type of the given instance is effectively final.
- public void rewriteGetClass(IRCode code) {
- InstructionIterator it = code.instructionIterator();
- while (it.hasNext()) {
- Instruction current = it.next();
- // Conservatively bail out if the containing block has catch handlers.
- // TODO(b/118509730): unless join of all catch types is ClassNotFoundException ?
- if (current.getBlock().hasCatchHandlers()) {
- continue;
- }
- if (!current.isInvokeVirtual()) {
- continue;
- }
- InvokeVirtual invoke = current.asInvokeVirtual();
- DexMethod invokedMethod = invoke.getInvokedMethod();
- // Class<?> Object#getClass() is final and cannot be overridden.
- if (invokedMethod != appInfo.dexItemFactory.objectMethods.getClass) {
- continue;
- }
- Value in = invoke.getReceiver();
- if (in.hasLocalInfo()) {
- continue;
- }
- TypeLatticeElement inType = in.getTypeLattice();
- // Check the receiver is either class type or array type. Also make sure it is not nullable.
- if (!(inType.isClassType() || inType.isArrayType())
- || inType.isNullable()) {
- continue;
- }
- DexType type = inType.isClassType()
- ? inType.asClassTypeLatticeElement().getClassType()
- : inType.asArrayTypeLatticeElement().getArrayType(appInfo.dexItemFactory);
- DexType baseType = type.toBaseType(appInfo.dexItemFactory);
- // Make sure base type is a class type.
- if (!baseType.isClassType()) {
- continue;
- }
- // Only consider program class, e.g., platform can introduce sub types in different versions.
- DexClass clazz = appInfo.definitionFor(baseType);
- if (clazz == null || !clazz.isProgramClass()) {
- continue;
- }
- // Only consider effectively final class. Exception: new Base().getClass().
- if (!baseType.hasSubtypes()
- || !appInfo.withLiveness().isInstantiatedIndirectly(baseType)
- || (!in.isPhi() && in.definition.isCreatingInstanceOrArray())) {
- // Make sure the target (base) type is visible.
- ConstraintWithTarget constraints =
- ConstraintWithTarget.classIsVisible(code.method.method.getHolder(), baseType, appInfo);
- if (constraints == ConstraintWithTarget.NEVER) {
- continue;
- }
- TypeLatticeElement typeLattice = TypeLatticeElement.classClassType(appInfo);
- Value value = code.createValue(typeLattice, invoke.getLocalInfo());
- ConstClass constClass = new ConstClass(value, type);
- it.replaceCurrentInstruction(constClass);
- }
- }
- assert code.isConsistentSSA();
- }
-
public void removeTrivialCheckCastAndInstanceOfInstructions(
IRCode code, boolean enableWholeProgramOptimizations) {
if (!enableWholeProgramOptimizations) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/ReflectionOptimizer.java b/src/main/java/com/android/tools/r8/ir/optimize/ReflectionOptimizer.java
index ecd3a7a..496e113 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/ReflectionOptimizer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/ReflectionOptimizer.java
@@ -9,8 +9,19 @@
import com.android.tools.r8.errors.Unreachable;
import com.android.tools.r8.graph.DexClass;
+import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexString;
+import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.ir.analysis.type.TypeLatticeElement;
+import com.android.tools.r8.ir.code.ConstClass;
+import com.android.tools.r8.ir.code.IRCode;
+import com.android.tools.r8.ir.code.Instruction;
+import com.android.tools.r8.ir.code.InstructionIterator;
+import com.android.tools.r8.ir.code.InvokeVirtual;
+import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.ir.optimize.Inliner.ConstraintWithTarget;
import com.android.tools.r8.ir.optimize.ReflectionOptimizer.ClassNameComputationInfo.ClassNameComputationOption;
+import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
import com.google.common.base.Strings;
public class ReflectionOptimizer {
@@ -57,6 +68,67 @@
}
}
+ // Rewrite getClass() call to const-class if the type of the given instance is effectively final.
+ public static void rewriteGetClass(AppInfoWithLiveness appInfo, IRCode code) {
+ InstructionIterator it = code.instructionIterator();
+ while (it.hasNext()) {
+ Instruction current = it.next();
+ // Conservatively bail out if the containing block has catch handlers.
+ // TODO(b/118509730): unless join of all catch types is ClassNotFoundException ?
+ if (current.getBlock().hasCatchHandlers()) {
+ continue;
+ }
+ if (!current.isInvokeVirtual()) {
+ continue;
+ }
+ InvokeVirtual invoke = current.asInvokeVirtual();
+ DexMethod invokedMethod = invoke.getInvokedMethod();
+ // Class<?> Object#getClass() is final and cannot be overridden.
+ if (invokedMethod != appInfo.dexItemFactory.objectMethods.getClass) {
+ continue;
+ }
+ Value in = invoke.getReceiver();
+ if (in.hasLocalInfo()) {
+ continue;
+ }
+ TypeLatticeElement inType = in.getTypeLattice();
+ // Check the receiver is either class type or array type. Also make sure it is not nullable.
+ if (!(inType.isClassType() || inType.isArrayType())
+ || inType.isNullable()) {
+ continue;
+ }
+ DexType type = inType.isClassType()
+ ? inType.asClassTypeLatticeElement().getClassType()
+ : inType.asArrayTypeLatticeElement().getArrayType(appInfo.dexItemFactory);
+ DexType baseType = type.toBaseType(appInfo.dexItemFactory);
+ // Make sure base type is a class type.
+ if (!baseType.isClassType()) {
+ continue;
+ }
+ // Only consider program class, e.g., platform can introduce sub types in different versions.
+ DexClass clazz = appInfo.definitionFor(baseType);
+ if (clazz == null || !clazz.isProgramClass()) {
+ continue;
+ }
+ // Only consider effectively final class. Exception: new Base().getClass().
+ if (!baseType.hasSubtypes()
+ || !appInfo.isInstantiatedIndirectly(baseType)
+ || (!in.isPhi() && in.definition.isCreatingInstanceOrArray())) {
+ // Make sure the target (base) type is visible.
+ ConstraintWithTarget constraints =
+ ConstraintWithTarget.classIsVisible(code.method.method.getHolder(), baseType, appInfo);
+ if (constraints == ConstraintWithTarget.NEVER) {
+ continue;
+ }
+ TypeLatticeElement typeLattice = TypeLatticeElement.classClassType(appInfo);
+ Value value = code.createValue(typeLattice, invoke.getLocalInfo());
+ ConstClass constClass = new ConstClass(value, type);
+ it.replaceCurrentInstruction(constClass);
+ }
+ }
+ assert code.isConsistentSSA();
+ }
+
public static String computeClassName(
DexString descriptor, DexClass holder, ClassNameComputationInfo classNameComputationInfo) {
return computeClassName(
diff --git a/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java b/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
index 3a2f4e2..d1e0606 100644
--- a/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
@@ -13,7 +13,6 @@
import com.android.tools.r8.graph.DexString;
import com.android.tools.r8.graph.GraphLense;
import com.android.tools.r8.graph.GraphLense.NestedGraphLense;
-import com.android.tools.r8.graph.TopDownClassHierarchyTraversal;
import com.android.tools.r8.logging.Log;
import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
import com.android.tools.r8.shaking.VerticalClassMerger.IllegalAccessDetector;
@@ -127,13 +126,11 @@
}
public GraphLense run() {
- // Visit the program classes in a top-down order according to the class hierarchy.
- Iterable<DexProgramClass> classes = appView.appInfo().app.classesWithDeterministicOrder();
- TopDownClassHierarchyTraversal.visit(appView, classes, clazz -> {
+ for (DexProgramClass clazz : appView.appInfo().app.classesWithDeterministicOrder()) {
if (satisfiesMergeCriteria(clazz)) {
merge(clazz);
}
- });
+ }
if (Log.ENABLED) {
Log.info(
getClass(),
@@ -204,6 +201,11 @@
return true;
}
+ private boolean isValidRepresentative(DexProgramClass clazz) {
+ // Disallow interfaces from being representatives, since interface methods require desugaring.
+ return !clazz.isInterface();
+ }
+
private boolean merge(DexProgramClass clazz) {
assert satisfiesMergeCriteria(clazz);
@@ -216,8 +218,12 @@
private boolean mergeGlobally(DexProgramClass clazz, String pkg) {
Representative globalRepresentative = representatives.get(GLOBAL);
if (globalRepresentative == null) {
- // Make the current class the global representative.
- setRepresentative(GLOBAL, getOrCreateRepresentative(clazz, pkg));
+ if (isValidRepresentative(clazz)) {
+ // Make the current class the global representative.
+ setRepresentative(GLOBAL, getOrCreateRepresentative(clazz, pkg));
+ } else {
+ clearRepresentative(GLOBAL);
+ }
// Do not attempt to merge this class inside its own package, because that could lead to
// an increase in the global representative, which is not desirable.
@@ -227,8 +233,12 @@
globalRepresentative.include(clazz);
if (globalRepresentative.isFull()) {
- // Make the current class the global representative instead.
- setRepresentative(GLOBAL, getOrCreateRepresentative(clazz, pkg));
+ if (isValidRepresentative(clazz)) {
+ // Make the current class the global representative instead.
+ setRepresentative(GLOBAL, getOrCreateRepresentative(clazz, pkg));
+ } else {
+ clearRepresentative(GLOBAL);
+ }
// Do not attempt to merge this class inside its own package, because that could lead to
// an increase in the global representative, which is not desirable.
@@ -244,7 +254,8 @@
private boolean mergeInsidePackage(DexProgramClass clazz, String pkg) {
Representative packageRepresentative = representatives.get(pkg);
if (packageRepresentative != null) {
- if (clazz.accessFlags.isMoreVisibleThan(packageRepresentative.clazz.accessFlags)) {
+ if (isValidRepresentative(clazz)
+ && clazz.accessFlags.isMoreVisibleThan(packageRepresentative.clazz.accessFlags)) {
// Use `clazz` as a representative for this package instead.
Representative newRepresentative = getOrCreateRepresentative(clazz, pkg);
newRepresentative.include(packageRepresentative.clazz);
@@ -271,7 +282,9 @@
}
// We were unable to use the current representative for this package (if any).
- setRepresentative(pkg, getOrCreateRepresentative(clazz, pkg));
+ if (isValidRepresentative(clazz)) {
+ setRepresentative(pkg, getOrCreateRepresentative(clazz, pkg));
+ }
return false;
}
@@ -288,6 +301,7 @@
}
private void setRepresentative(String pkg, Representative representative) {
+ assert isValidRepresentative(representative.clazz);
if (Log.ENABLED) {
if (pkg.equals(GLOBAL)) {
Log.info(
@@ -305,6 +319,17 @@
representatives.put(pkg, representative);
}
+ private void clearRepresentative(String pkg) {
+ if (Log.ENABLED) {
+ if (pkg.equals(GLOBAL)) {
+ Log.info(getClass(), "Removing the global representative");
+ } else {
+ Log.info(getClass(), "Removing the representative for package %s", pkg);
+ }
+ }
+ representatives.remove(pkg);
+ }
+
private boolean mayMergeAcrossPackageBoundaries(DexProgramClass clazz) {
// Check that the class is public. Otherwise, accesses to `clazz` from within its current
// package may become illegal.
diff --git a/src/test/java/com/android/tools/r8/classmerging/StaticClassMergerInterfaceTest.java b/src/test/java/com/android/tools/r8/classmerging/StaticClassMergerInterfaceTest.java
new file mode 100644
index 0000000..294eca0
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/classmerging/StaticClassMergerInterfaceTest.java
@@ -0,0 +1,105 @@
+// Copyright (c) 2018, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.classmerging;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.core.IsNot.not;
+import static org.junit.Assert.assertThat;
+
+import com.android.tools.r8.NeverClassInline;
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.ToolHelper.DexVm.Version;
+import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class StaticClassMergerInterfaceTest extends TestBase {
+
+ private final Backend backend;
+
+ @Parameters(name = "Backend: {0}")
+ public static Backend[] data() {
+ return Backend.values();
+ }
+
+ public StaticClassMergerInterfaceTest(Backend backend) {
+ this.backend = backend;
+ }
+
+ @Test
+ public void test() throws Exception {
+ String expectedOutput = StringUtils.lines("In A.a()", "In B.b()", "In C.c()");
+
+ CodeInspector inspector =
+ testForR8(backend)
+ .addInnerClasses(StaticClassMergerInterfaceTest.class)
+ .addKeepMainRule(TestClass.class)
+ .addKeepRules("-dontobfuscate")
+ .enableInliningAnnotations()
+ .enableClassInliningAnnotations()
+ .run(TestClass.class)
+ .assertSuccessWithOutput(expectedOutput)
+ .inspector();
+
+ // Check that A has not been merged into B. The static class merger visits classes in alpha-
+ // betical order. By the time A is processed, there is no merge representative and A is not
+ // a valid merge representative itself, because it is an interface.
+ if (ToolHelper.getDexVm().getVersion().isNewerThan(Version.V6_0_1) || backend == Backend.CF) {
+ assertThat(inspector.clazz(A.class), isPresent());
+ } else {
+ assertThat(inspector.clazz(A.class.getTypeName() + "$-CC"), isPresent());
+ }
+
+
+ // By the time B is processed, there is no merge representative, so it should be present.
+ assertThat(inspector.clazz(B.class), isPresent());
+
+ // By the time C is processed, B should be merge candidate. Therefore, we should allow C.c() to
+ // be moved to B *although C is an interface*.
+ assertThat(inspector.clazz(C.class), not(isPresent()));
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ A.a();
+ B.b();
+ C.c();
+ }
+ }
+
+ @NeverClassInline
+ interface A {
+
+ @NeverInline
+ static void a() {
+ System.out.println("In A.a()");
+ }
+ }
+
+ @NeverClassInline
+ static class B {
+
+ @NeverInline
+ static void b() {
+ System.out.println("In B.b()");
+ }
+ }
+
+ @NeverClassInline
+ interface C {
+
+ @NeverInline
+ static void c() {
+ System.out.println("In C.c()");
+ }
+ }
+}
