Regard null outValue as unused StringBuilder.
Bug: 114002137, 137038659
Change-Id: I190c875c41755c130a77594c829db8e7e2a7661d
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizer.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizer.java
index 516ef27..ad890ec 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizer.java
@@ -567,11 +567,10 @@
InvokeVirtual invoke = instr.asInvokeVirtual();
assert invoke.inValues().size() == 1;
Value builder = invoke.getReceiver().getAliasedValue();
- assert invoke.hasOutValue();
Value outValue = invoke.outValue();
- if (outValue.isDead(appView, code)) {
+ if (outValue == null || outValue.isDead(appView, code)) {
// If the out value is still used but potentially dead, replace it with a dummy string.
- if (outValue.isUsed()) {
+ if (outValue != null && outValue.isUsed()) {
Value dummy =
code.createValue(
TypeLatticeElement.stringClassType(appView, definitelyNotNull()),
@@ -610,9 +609,6 @@
return false;
}
InvokeVirtual invoke = instr.asInvokeVirtual();
- if (!invoke.hasOutValue()) {
- return false;
- }
DexMethod invokedMethod = invoke.getInvokedMethod();
if (!optimizationConfiguration.isToStringMethod(invokedMethod)) {
return false;
@@ -624,7 +620,7 @@
}
// If the result of toString() is no longer used, computing the compile-time constant is
// even not necessary.
- if (invoke.outValue().isDead(appView, code)) {
+ if (!invoke.hasOutValue() || invoke.outValue().isDead(appView, code)) {
return true;
}
Map<Instruction, BuilderState> perInstrState = builderStates.get(builder);
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizerAnalysisTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizerAnalysisTest.java
index 627ea8e..f186803 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizerAnalysisTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringBuilderOptimizerAnalysisTest.java
@@ -37,6 +37,22 @@
}
@Test
+ public void testUnusedBuilder() throws Exception {
+ buildAndCheckIR(
+ "unusedBuilder",
+ checkOptimizerStates(appView, optimizer -> {
+ assertEquals(1, optimizer.analysis.builderStates.size());
+ for (Value builder : optimizer.analysis.builderStates.keySet()) {
+ Map<Instruction, BuilderState> perBuilderState =
+ optimizer.analysis.builderStates.get(builder);
+ checkBuilderState(optimizer, perBuilderState, "42", true);
+ }
+ assertEquals(1, optimizer.analysis.deadBuilders.size());
+ assertEquals(0, optimizer.analysis.simplifiedBuilders.size());
+ }));
+ }
+
+ @Test
public void testTrivialSequence() throws Exception {
buildAndCheckIR(
"trivialSequence",
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTest.java
index e8fa90f..1b16aa6 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTest.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.ir.optimize.string;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assume.assumeTrue;
@@ -19,7 +20,6 @@
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.android.tools.r8.utils.codeinspector.InstructionSubject.JumboStringMode;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
-import com.google.common.collect.Streams;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -28,19 +28,32 @@
public class StringConcatenationTest extends TestBase {
private static final Class<?> MAIN = StringConcatenationTestClass.class;
private static final String JAVA_OUTPUT = StringUtils.lines(
+ // trivialSequence
"xyz",
+ // builderWithInitialValue
"Hello,R8",
+ // builderWithCapacity
"42",
+ // nonStringArgs
"42",
+ // typeConversion
"0.14 0 false null",
+ // typeConversion_withPhis
"3.14 3 0",
+ // nestedBuilders_appendBuilderItself
"Hello,R8",
+ // nestedBuilders_appendBuilderResult
"Hello,R8",
+ // simplePhi
"Hello,",
"Hello,D8",
+ // phiAtInit
"Hello,R8",
+ // phiWithDifferentInits
"Hello,R8",
+ // loop
"na;na;na;na;na;na;na;na;Batman!",
+ // loopWithBuilder
"na;na;na;na;na;na;na;na;Batman!"
);
@@ -64,96 +77,92 @@
.assertSuccessWithOutput(JAVA_OUTPUT);
}
- private void test(
- TestRunResult result,
- int expectedStringCountInTrivialSequence,
- int expectedStringCountInBuilderWithInitialValue,
- int expectedStringCountInBuilderWithCapacity,
- int expectedStringCountInNonStringArgs,
- int expectedStringCountInTypeConversion,
- int expectedStringCountInNestedBuilderAppendItself,
- int expectedStringCountInNestedBuilderAppendResult)
- throws Exception {
+ private void test(TestRunResult result, boolean isR8, boolean isReleaseMode) throws Exception {
+ // TODO(b/114002137): The lack of subtyping made the escape analysis to regard
+ // StringBuilder#toString as an alias-introducing instruction.
+ // For now, debug v.s. release mode of D8 have the same result.
+
+ // Smaller is better in general. If the counter part is zero, that means non-string arguments
+ // are used, and in that case bigger is better.
+ // If the fixed count is used, that means StringBuilderOptimization should keep things as-is
+ // or there would be no observable differences (# of builders could be different).
+ int expectedCount;
+
CodeInspector codeInspector = result.inspector();
ClassSubject mainClass = codeInspector.clazz(MAIN);
- MethodSubject method = mainClass.uniqueMethodWithName("trivialSequence");
+ MethodSubject method = mainClass.uniqueMethodWithName("unusedBuilder");
+ if (isR8) {
+ assertThat(method, not(isPresent()));
+ } else {
+ assertThat(method, isPresent());
+ assertEquals(0, countConstString(method));
+ }
+
+ method = mainClass.uniqueMethodWithName("trivialSequence");
assertThat(method, isPresent());
- long count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(expectedStringCountInTrivialSequence, count);
+ expectedCount = isR8 ? 1 : 3;
+ assertEquals(expectedCount, countConstString(method));
method = mainClass.uniqueMethodWithName("builderWithInitialValue");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(expectedStringCountInBuilderWithInitialValue, count);
+ expectedCount = isR8 ? 1 : 3;
+ assertEquals(expectedCount, countConstString(method));
method = mainClass.uniqueMethodWithName("builderWithCapacity");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(expectedStringCountInBuilderWithCapacity, count);
+ expectedCount = isR8 ? 1 : 0;
+ assertEquals(expectedCount, countConstString(method));
method = mainClass.uniqueMethodWithName("nonStringArgs");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(expectedStringCountInNonStringArgs, count);
+ expectedCount = isR8 ? 1 : 0;
+ assertEquals(expectedCount, countConstString(method));
method = mainClass.uniqueMethodWithName("typeConversion");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(expectedStringCountInTypeConversion, count);
+ expectedCount = isR8 ? 1 : 0;
+ assertEquals(expectedCount, countConstString(method));
method = mainClass.uniqueMethodWithName("typeConversion_withPhis");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(0, count);
+ assertEquals(0, countConstString(method));
method = mainClass.uniqueMethodWithName("nestedBuilders_appendBuilderItself");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(expectedStringCountInNestedBuilderAppendItself, count);
+ // TODO(b/113859361): merge builders
+ expectedCount = 3;
+ assertEquals(expectedCount, countConstString(method));
method = mainClass.uniqueMethodWithName("nestedBuilders_appendBuilderResult");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(expectedStringCountInNestedBuilderAppendResult, count);
+ // TODO(b/113859361): merge builders
+ expectedCount = 3;
+ assertEquals(expectedCount, countConstString(method));
method = mainClass.uniqueMethodWithName("simplePhi");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(4, count);
+ assertEquals(4, countConstString(method));
method = mainClass.uniqueMethodWithName("phiAtInit");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(3, count);
+ assertEquals(3, countConstString(method));
method = mainClass.uniqueMethodWithName("phiWithDifferentInits");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(3, count);
+ assertEquals(3, countConstString(method));
method = mainClass.uniqueMethodWithName("loop");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(3, count);
+ assertEquals(3, countConstString(method));
method = mainClass.uniqueMethodWithName("loopWithBuilder");
assertThat(method, isPresent());
- count = Streams.stream(method.iterateInstructions(
- i -> i.isConstString(JumboStringMode.ALLOW))).count();
- assertEquals(2, count);
+ assertEquals(2, countConstString(method));
+ }
+
+ private long countConstString(MethodSubject method) {
+ return method.streamInstructions().filter(i -> i.isConstString(JumboStringMode.ALLOW)).count();
}
@Test
@@ -167,7 +176,7 @@
.setMinApi(parameters.getRuntime())
.run(parameters.getRuntime(), MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
- test(result, 3, 3, 0, 0, 0, 3, 3);
+ test(result, false, false);
result =
testForD8()
@@ -176,9 +185,7 @@
.setMinApi(parameters.getRuntime())
.run(parameters.getRuntime(), MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
- // TODO(b/114002137): The lack of subtyping made the escape analysis to regard
- // StringBuilder#toString as an alias-introducing instruction.
- test(result, 3, 3, 0, 0, 0, 3, 3);
+ test(result, false, true);
}
@Test
@@ -194,6 +201,6 @@
.setMinApi(parameters.getRuntime())
.run(parameters.getRuntime(), MAIN)
.assertSuccessWithOutput(JAVA_OUTPUT);
- test(result, 1, 1, 1, 1, 1, 3, 3);
+ test(result, true, true);
}
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTestClass.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTestClass.java
index e00d97a..980148b 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTestClass.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatenationTestClass.java
@@ -7,6 +7,14 @@
class StringConcatenationTestClass {
@NeverInline
+ public static void unusedBuilder() {
+ StringBuilder builder = new StringBuilder();
+ builder.append(4);
+ builder.append(2);
+ builder.toString();
+ }
+
+ @NeverInline
public static void trivialSequence() {
String x = "x";
String y = "y";
@@ -149,6 +157,7 @@
}
public static void main(String[] args) {
+ unusedBuilder();
trivialSequence();
builderWithInitialValue();
builderWithCapacity();
