Remove trivial StringBuilders if not used.
Bug: 137038659, 114002137
Change-Id: Ia77b0f07ddf0414c30ddfd109bcc1e2a563b8147
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 f647eb4..152fcd8 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
@@ -35,7 +35,6 @@
import it.unimi.dsi.fastutil.objects.Object2IntArrayMap;
import it.unimi.dsi.fastutil.objects.Object2IntMap;
import java.util.HashMap;
-import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedList;
import java.util.List;
@@ -89,6 +88,7 @@
private int numberOfBuildersWithNonStringArg = 0;
private int numberOfBuildersWithMergingPoints = 0;
private int numberOfBuildersWithNonDeterministicArg = 0;
+ private int numberOfDeadBuilders = 0;
private int numberOfBuildersSimplified = 0;
private final Object2IntMap<Integer> histogramOfLengthOfAppendChains;
private final Object2IntMap<Integer> histogramOfLengthOfEndResult;
@@ -151,8 +151,8 @@
"# builders w/ merging points: %s", numberOfBuildersWithMergingPoints);
Log.info(getClass(),
"# builders w/ non-deterministic arg: %s", numberOfBuildersWithNonDeterministicArg);
- Log.info(getClass(),
- "# builders simplified: %s", numberOfBuildersSimplified);
+ Log.info(getClass(), "# dead builders : %s", numberOfDeadBuilders);
+ Log.info(getClass(), "# builders simplified: %s", numberOfBuildersSimplified);
assert histogramOfLengthOfAppendChains != null;
Log.info(getClass(), "------ histogram of StringBuilder append chain lengths ------");
histogramOfLengthOfAppendChains.forEach((chainSize, count) -> {
@@ -500,9 +500,10 @@
}
}
- // StringBuilders that are simplified by this analysis. Will be used to clean up uses of the
- // builders, such as creation, <init>, and append calls.
- final Set<Value> simplifiedBuilders = new HashSet<>();
+ // StringBuilders that are simplified by this analysis or simply dead (e.g., after applying
+ // -assumenosideeffects to logging calls, then logging messages built by builders are dead).
+ // Will be used to clean up uses of the builders, such as creation, <init>, and append calls.
+ final Set<Value> simplifiedBuilders = Sets.newIdentityHashSet();
private StringConcatenationAnalysis applyConcatenationResults(Set<Value> candidateBuilders) {
InstructionIterator it = code.instructionIterator();
@@ -514,6 +515,15 @@
InvokeVirtual invoke = instr.asInvokeVirtual();
assert invoke.inValues().size() == 1;
Value builder = invoke.getReceiver().getAliasedValue();
+ assert invoke.hasOutValue();
+ if (invoke.outValue().isDead(appView, code)) {
+ it.removeOrReplaceByDebugLocalRead();
+ // Although this builder is not simplified by this analysis, add that to the set so that
+ // it can be removed at the final clean-up phase.
+ simplifiedBuilders.add(builder);
+ numberOfDeadBuilders++;
+ continue;
+ }
Map<Instruction, BuilderState> perInstrState = builderStates.get(builder);
assert perInstrState != null;
BuilderState builderState = perInstrState.get(instr);
@@ -547,6 +557,11 @@
if (!candidateBuilders.contains(builder)) {
return false;
}
+ // If the result of toString() is no longer used, computing the compile-time constant is
+ // even not necessary.
+ if (invoke.outValue().isDead(appView, code)) {
+ return true;
+ }
Map<Instruction, BuilderState> perInstrState = builderStates.get(builder);
if (perInstrState == null) {
return false;
@@ -834,7 +849,7 @@
newState.previous = this;
newState.addition = addition;
if (this.nexts == null) {
- this.nexts = new HashSet<>();
+ this.nexts = Sets.newIdentityHashSet();
}
this.nexts.add(newState);
return newState;
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 fc87c72..1e4e3d7 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
@@ -161,7 +161,7 @@
@Test
public void testR8() throws Exception {
- assumeTrue("CF does not canonicalize constants.", parameters.isDexRuntime());
+ assumeTrue("CF does not rewrite move results.", parameters.isDexRuntime());
R8TestRunResult result =
testForR8(parameters.getBackend())
@@ -178,6 +178,7 @@
// TODO(b/114002137): Once enabled, remove this test-specific setting.
private void configure(InternalOptions options) {
+ assert !options.enableStringConcatenationOptimization;
options.enableStringConcatenationOptimization = true;
}
diff --git a/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/StringBuildersAfterAssumenosideeffectsTest.java b/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/StringBuildersAfterAssumenosideeffectsTest.java
index 2114ad6..8ff68aa 100644
--- a/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/StringBuildersAfterAssumenosideeffectsTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/StringBuildersAfterAssumenosideeffectsTest.java
@@ -6,13 +6,14 @@
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assume.assumeTrue;
import com.android.tools.r8.NeverClassInline;
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
+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;
@@ -40,6 +41,8 @@
@Test
public void testR8() throws Exception {
+ assumeTrue("CF does not rewrite move results.", parameters.isDexRuntime());
+
testForR8(parameters.getBackend())
.addInnerClasses(StringBuildersAfterAssumenosideeffectsTest.class)
.enableClassInliningAnnotations()
@@ -49,6 +52,7 @@
"-assumenosideeffects class * implements " + TestLogger.class.getTypeName() + " {",
" void info(...);",
"}")
+ .addOptionsModification(this::configure)
.noMinification()
.setMinApi(parameters.getRuntime())
.run(parameters.getRuntime(), MAIN)
@@ -67,12 +71,17 @@
Streams.stream(mainMethod.iterateInstructions(
i -> i.isInvoke() && i.getMethod().name.toString().equals("info"))).count());
- // TODO(b/137038659): StringBuilders (and all other calls) can be gone.
- assertNotEquals(
+ assertEquals(
0,
Streams.stream(mainMethod.iterateInstructions(
i -> i.isInvoke()
- && i.getMethod().holder.toDescriptorString().contains("StringBuilder"))));
+ && i.getMethod().holder.toDescriptorString().contains("StringBuilder"))).count());
+ }
+
+ // TODO(b/114002137): Once enabled, remove this test-specific setting.
+ private void configure(InternalOptions options) {
+ assert !options.enableStringConcatenationOptimization;
+ options.enableStringConcatenationOptimization = true;
}
interface TestLogger {
