Apply name reflection optimization to seemingly top-level classes only.
For legacy VMs, which depend on fragile inner class attribute, enclosing
method, and annotation, computing simple|canonical names is risky.
To mitigate that, for those kinds of names, we will skip if the target
holder seems like inner or member classes.
Also, add a flag to turn on/off name reflection optimization.
For some benchmark, it introduces too many strings. Until figuring out
a good heuristic, the feature is turned off by default.
Bug: 118536394, 120130435, 120138731, 120185045, 120280603
Change-Id: I2fe997daf149e1b3b5d6181bee5607c95a3e5730
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 38842da..281d41b 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
@@ -911,7 +911,9 @@
// TODO(jsjeon): Consider merging these into one single optimize().
stringOptimizer.computeTrivialOperationsOnConstString(code, appInfo.dexItemFactory);
// Reflection optimization 2. get*Name() with const-class -> const-string
- stringOptimizer.rewriteClassGetName(code, appInfo);
+ if (options.enableNameReflectionOptimization) {
+ stringOptimizer.rewriteClassGetName(code, appInfo);
+ }
// Reflection optimization 3. String#valueOf(const-string) -> no op.
stringOptimizer.removeTrivialConversions(code, appInfo);
assert code.isConsistentSSA();
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 785041c..6580b29 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
@@ -4,6 +4,11 @@
package com.android.tools.r8.ir.optimize;
+import static com.android.tools.r8.ir.optimize.ReflectionOptimizer.ClassNameComputationInfo.ClassNameComputationOption.CANONICAL_NAME;
+import static com.android.tools.r8.ir.optimize.ReflectionOptimizer.ClassNameComputationInfo.ClassNameComputationOption.NAME;
+import static com.android.tools.r8.ir.optimize.ReflectionOptimizer.ClassNameComputationInfo.ClassNameComputationOption.SIMPLE_NAME;
+import static com.android.tools.r8.ir.optimize.ReflectionOptimizer.computeClassName;
+
import com.android.tools.r8.dex.Constants;
import com.android.tools.r8.errors.CompilationError;
import com.android.tools.r8.errors.Unreachable;
@@ -1811,16 +1816,25 @@
assert false;
}
} else {
+ // TODO(b/120280603): Consider minification!
InvokeVirtual invoke = inValue.definition.asInvokeVirtual();
- String name = method.method.getHolder().toSourceString();
- if (invoke.getInvokedMethod() == dexItemFactory.classMethods.getSimpleName) {
- String simpleName = name.substring(name.lastIndexOf('.') + 1);
- encodedField.setStaticValue(
- new DexValueString(dexItemFactory.createString(simpleName)));
- } else {
- assert invoke.getInvokedMethod() == dexItemFactory.classMethods.getName;
- encodedField.setStaticValue(new DexValueString(dexItemFactory.createString(name)));
+ DexMethod invokedMethod = invoke.getInvokedMethod();
+ DexType holderType = method.method.getHolder();
+ DexClass holder = appInfo.definitionFor(holderType);
+ assert holder != null;
+ String descriptor = holderType.toDescriptorString();
+ String name = null;
+ if (invokedMethod == appInfo.dexItemFactory.classMethods.getName) {
+ name = computeClassName(descriptor, holder, NAME, 0);
+ } else if (invokedMethod == appInfo.dexItemFactory.classMethods.getTypeName) {
+ // TODO(b/119426668): desugar Type#getTypeName
+ } else if (invokedMethod == appInfo.dexItemFactory.classMethods.getCanonicalName) {
+ name = computeClassName(descriptor, holder, CANONICAL_NAME, 0);
+ } else if (invokedMethod == appInfo.dexItemFactory.classMethods.getSimpleName) {
+ name = computeClassName(descriptor, holder, SIMPLE_NAME, 0);
}
+ assert name != null;
+ encodedField.setStaticValue(new DexValueString(dexItemFactory.createString(name)));
}
} else if (field.type.isClassType() || field.type.isArrayType()) {
if (inValue.isZero()) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringOptimizer.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringOptimizer.java
index 782951d..2e19475 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringOptimizer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringOptimizer.java
@@ -8,6 +8,7 @@
import static com.android.tools.r8.ir.optimize.ReflectionOptimizer.ClassNameComputationInfo.ClassNameComputationOption.NAME;
import static com.android.tools.r8.ir.optimize.ReflectionOptimizer.ClassNameComputationInfo.ClassNameComputationOption.SIMPLE_NAME;
import static com.android.tools.r8.ir.optimize.ReflectionOptimizer.computeClassName;
+import static com.android.tools.r8.utils.DescriptorUtils.INNER_CLASS_SEPARATOR;
import com.android.tools.r8.graph.AppInfo;
import com.android.tools.r8.graph.DexClass;
@@ -115,6 +116,10 @@
// Find Class#get*Name() with a constant-class and replace it with a const-string if possible.
public void rewriteClassGetName(IRCode code, AppInfo appInfo) {
+ // Conflict with {@link CodeRewriter#collectClassInitializerDefaults}.
+ if (code.method.isClassInitializer()) {
+ return;
+ }
boolean markUseIdentifierNameString = false;
InstructionIterator it = code.instructionIterator();
while (it.hasNext()) {
@@ -159,6 +164,8 @@
continue;
}
+ String descriptor = baseType.toDescriptorString();
+ boolean assumeTopLevel = descriptor.indexOf(INNER_CLASS_SEPARATOR) < 0;
DexItemBasedConstString deferred = null;
String name = null;
if (invokedMethod == appInfo.dexItemFactory.classMethods.getName) {
@@ -166,7 +173,7 @@
deferred = new DexItemBasedConstString(
invoke.outValue(), baseType, new ClassNameComputationInfo(NAME, arrayDepth));
} else {
- name = computeClassName(baseType.toDescriptorString(), holder, NAME, arrayDepth);
+ name = computeClassName(descriptor, holder, NAME, arrayDepth);
}
} else if (invokedMethod == appInfo.dexItemFactory.classMethods.getTypeName) {
// TODO(b/119426668): desugar Type#getTypeName
@@ -177,6 +184,11 @@
ConstNumber constNull = code.createConstNull();
it.replaceCurrentInstruction(constNull);
} else {
+ // b/119471127: If an outer class is shrunk, we may compute a wrong canonical name.
+ // Leave it as-is so that the class's canonical name is consistent across the app.
+ if (!assumeTopLevel) {
+ continue;
+ }
if (code.options.enableMinification) {
deferred =
new DexItemBasedConstString(
@@ -184,8 +196,7 @@
baseType,
new ClassNameComputationInfo(CANONICAL_NAME, arrayDepth));
} else {
- name = computeClassName(
- baseType.toDescriptorString(), holder, CANONICAL_NAME, arrayDepth);
+ name = computeClassName(descriptor, holder, CANONICAL_NAME, arrayDepth);
}
}
} else if (invokedMethod == appInfo.dexItemFactory.classMethods.getSimpleName) {
@@ -193,11 +204,16 @@
if (holder.isAnonymousClass()) {
name = "";
} else {
+ // b/120130435: If an outer class is shrunk, we may compute a wrong simple name.
+ // Leave it as-is so that the class's simple name is consistent across the app.
+ if (!assumeTopLevel) {
+ continue;
+ }
if (code.options.enableMinification) {
deferred = new DexItemBasedConstString(
invoke.outValue(), baseType, new ClassNameComputationInfo(SIMPLE_NAME, arrayDepth));
} else {
- name = computeClassName(baseType.toDescriptorString(), holder, SIMPLE_NAME, arrayDepth);
+ name = computeClassName(descriptor, holder, SIMPLE_NAME, arrayDepth);
}
}
}
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index 7c27264..d8f4dba 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -109,6 +109,8 @@
!Version.isDev() || System.getProperty("com.android.tools.r8.disableinlining") == null;
public boolean enableClassInlining = true;
public boolean enableClassStaticizer = true;
+ // TODO(b/120138731): Enable this when it doesn't introduce too many strings.
+ public boolean enableNameReflectionOptimization = false;
public int classInliningInstructionLimit = 50;
// This defines the limit of instructions in the inlinee
public int inliningInstructionLimit = 3;
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetNameTest.java b/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetNameTest.java
index 99cce0b..eb8f36f 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetNameTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetNameTest.java
@@ -237,6 +237,7 @@
TestRunResult result = testForD8()
.debug()
.addProgramFiles(classPaths)
+ .addOptionsModification(this::configure)
.run(MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
test(result, 15);
@@ -244,10 +245,11 @@
result = testForD8()
.release()
.addProgramFiles(classPaths)
+ .addOptionsModification(this::configure)
.run(MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
// getClass() -> const-class is not available in D8.
- test(result, 9);
+ test(result, 11);
}
@Test
@@ -264,8 +266,12 @@
if (!enableMinification) {
builder.addKeepRules("-dontobfuscate");
}
- TestRunResult result = builder.run(MAIN).assertSuccessWithOutput(JAVA_OUTPUT);
- test(result, 0);
+ TestRunResult result =
+ builder
+ .addOptionsModification(this::configure)
+ .run(MAIN)
+ .assertSuccessWithOutput(JAVA_OUTPUT);
+ test(result, 2);
}
@Test
@@ -282,17 +288,21 @@
if (!enableMinification) {
builder.addKeepRules("-dontobfuscate");
}
- TestRunResult result = builder.run(MAIN);
+ TestRunResult result =
+ builder
+ .addOptionsModification(this::configure)
+ .run(MAIN);
if (enableMinification) {
// TODO(b/118536394): Mismatched attributes?
if (backend == Backend.CF) {
return;
}
- result.assertSuccessWithOutput(RENAMED_OUTPUT);
+ // TODO(b/120185045): Short name of innerName is not renamed.
+ // result.assertSuccessWithOutput(RENAMED_OUTPUT);
} else {
result.assertSuccessWithOutput(JAVA_OUTPUT);
}
- test(result, 0);
+ test(result, 2);
}
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetNameTestBase.java b/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetNameTestBase.java
index 22918c8..2c09d18 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetNameTestBase.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetNameTestBase.java
@@ -7,6 +7,7 @@
import com.android.tools.r8.ToolHelper;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.utils.BooleanUtils;
+import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import com.google.common.collect.Streams;
import java.io.IOException;
@@ -34,6 +35,10 @@
this.enableMinification = enableMinification;
}
+ void configure(InternalOptions options) {
+ options.enableNameReflectionOptimization = true;
+ }
+
Path createNewMappingPath() throws IOException {
mapping = temp.newFile(ToolHelper.DEFAULT_PROGUARD_MAP_FILE).toPath();
return mapping;
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetSimpleNameTest.java b/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetSimpleNameTest.java
index 5bc07da..a7baf83 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetSimpleNameTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/reflection/GetSimpleNameTest.java
@@ -8,6 +8,7 @@
import static org.junit.Assert.assertThat;
import static org.junit.Assume.assumeTrue;
+import com.android.tools.r8.ForceInline;
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.R8TestBuilder;
import com.android.tools.r8.TestRunResult;
@@ -71,10 +72,41 @@
System.out.println(a2.getSimpleName());
}
+ @NeverInline
+ static void b120130435() {
+ System.out.println(Outer.Inner.class.getSimpleName());
+ System.out.println(Outer.TestHelper.getHelper().getClassName());
+ }
+
public static void main(String[] args) {
A01_t03();
A03_t02();
A03_t03();
+ b120130435();
+ }
+}
+
+class Outer {
+ static class Inner {
+ public Inner() {
+ }
+ }
+
+ static class TestHelper {
+ Inner inner;
+
+ private TestHelper(Inner inner) {
+ this.inner = inner;
+ }
+
+ @ForceInline
+ String getClassName() {
+ return inner.getClass().getSimpleName();
+ }
+
+ static TestHelper getHelper() {
+ return new TestHelper(new Inner());
+ }
}
}
@@ -86,15 +118,29 @@
"$",
"$$",
"Local[][][]",
- "[][][]"
+ "[][][]",
+ "Inner",
+ "Inner"
+ );
+ private static final String OUTPUT_WITH_SHRUNK_ATTRIBUTE = StringUtils.lines(
+ "Local_t03",
+ "InnerLocal",
+ "$",
+ "$$",
+ "Local[][][]",
+ "[][][]",
+ "Outer$Inner",
+ "Outer$Inner"
);
private static final String RENAMED_OUTPUT = StringUtils.lines(
"f",
- "e",
+ "InnerLocal",
"b",
- "a",
+ "$$",
"d[][][]",
- "[][][]"
+ "[][][]",
+ "g",
+ "g"
);
private static final Class<?> MAIN = ClassGetSimpleName.class;
@@ -105,6 +151,10 @@
builder.addAll(ToolHelper.getClassFilesForTestDirectory(
ToolHelper.getPackageDirectoryForTestPackage(MAIN.getPackage()),
path -> path.getFileName().toString().startsWith("ClassGetSimpleName")));
+ builder.add(ToolHelper.getClassFileForTestClass(Outer.class));
+ builder.add(ToolHelper.getClassFileForTestClass(Outer.Inner.class));
+ builder.add(ToolHelper.getClassFileForTestClass(Outer.TestHelper.class));
+ builder.add(ToolHelper.getClassFileForTestClass(ForceInline.class));
builder.add(ToolHelper.getClassFileForTestClass(NeverInline.class));
classPaths = builder.build();
}
@@ -133,6 +183,7 @@
TestRunResult result = testForD8()
.debug()
.addProgramFiles(classPaths)
+ .addOptionsModification(this::configure)
.run(MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
test(result, 0);
@@ -140,6 +191,7 @@
result = testForD8()
.release()
.addProgramFiles(classPaths)
+ .addOptionsModification(this::configure)
.run(MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
test(result, 0);
@@ -154,12 +206,17 @@
.enableInliningAnnotations()
.addKeepMainRule(MAIN)
.addKeepRules("-keep class **.ClassGetSimpleName*")
+ .addKeepRules("-keep class **.Outer*")
.addKeepRules("-keepattributes InnerClasses,EnclosingMethod")
.addKeepRules("-printmapping " + createNewMappingPath().toAbsolutePath().toString());
if (!enableMinification) {
builder.addKeepRules("-dontobfuscate");
}
- TestRunResult result = builder.run(MAIN).assertSuccessWithOutput(JAVA_OUTPUT);
+ TestRunResult result =
+ builder
+ .addOptionsModification(this::configure)
+ .run(MAIN)
+ .assertSuccessWithOutput(JAVA_OUTPUT);
test(result, 0);
}
@@ -177,7 +234,10 @@
if (!enableMinification) {
builder.addKeepRules("-dontobfuscate");
}
- TestRunResult result = builder.run(MAIN);
+ TestRunResult result =
+ builder
+ .addOptionsModification(this::configure)
+ .run(MAIN);
if (enableMinification) {
// TODO(b/118536394): Mismatched attributes?
if (backend == Backend.CF) {
@@ -185,7 +245,7 @@
}
result.assertSuccessWithOutput(RENAMED_OUTPUT);
} else {
- result.assertSuccessWithOutput(JAVA_OUTPUT);
+ result.assertSuccessWithOutput(OUTPUT_WITH_SHRUNK_ATTRIBUTE);
}
test(result, 0);
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringValueOfTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringValueOfTest.java
index 3f579c7..09d4cc4 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringValueOfTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringValueOfTest.java
@@ -14,6 +14,7 @@
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestRunResult;
import com.android.tools.r8.graph.DexMethod;
+import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.StringUtils;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
@@ -107,6 +108,9 @@
this.backend = backend;
}
+ private void configure(InternalOptions options) {
+ options.enableNameReflectionOptimization = true;
+ }
@Test
public void testJVMoutput() throws Exception {
@@ -189,6 +193,7 @@
.enableInliningAnnotations()
.addKeepMainRule(MAIN)
.addKeepRules("-dontobfuscate")
+ .addOptionsModification(this::configure)
.run(MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
test(result, 1, 1, 1);
