Temporarily disable string concat optimization for roll
Revert "Rewrite StringBuilder.appends with non-constants to String.concat"
This reverts commit 1a0a373dd8f7eed2f9dcbd715b097445a4e17f96.
Revert "Fix missing call to newInstances"
This reverts commit ed5226ebcca8663777670307e3520b556cee6bd4.
Change-Id: If4ad3fbf7801834218e4b90048e64a11ce057b2f
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 44506fd..8fe1c7e 100644
--- a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
+++ b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
@@ -2118,7 +2118,7 @@
}
public boolean isAppendCharSequenceMethod(DexMethod method) {
- return method == appendCharSequence || method == appendSubCharSequence;
+ return method == appendCharSequence;
}
public boolean isAppendObjectOrCharSequenceMethod(DexMethod method) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAction.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAction.java
index 4383519..05a91e5 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAction.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAction.java
@@ -7,8 +7,6 @@
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexMethod;
-import com.android.tools.r8.ir.analysis.type.Nullability;
-import com.android.tools.r8.ir.analysis.type.TypeElement;
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.InstructionListIterator;
@@ -104,7 +102,28 @@
Instruction previous = iterator.previous();
InvokeMethodWithReceiver invoke = previous.asInvokeMethodWithReceiver();
assert invoke != null;
- Value value = insertStringConstantInstruction(appView, code, iterator, invoke, replacement);
+ // If the block has catch handlers, inserting a constant string in the same block as the
+ // append violates our block representation in DEX since constant string is throwing. If the
+ // append is in a block with catch handlers, we simply insert a new constant string in the
+ // entry block after all arguments.
+ Value value;
+ if (!invoke.getBlock().hasCatchHandlers()) {
+ value =
+ iterator.insertConstStringInstruction(
+ appView, code, appView.dexItemFactory().createString(replacement));
+ } else {
+ InstructionListIterator stringInsertIterator = code.entryBlock().listIterator(code);
+ while (stringInsertIterator.hasNext()) {
+ Instruction next = stringInsertIterator.next();
+ if (!next.isArgument()) {
+ stringInsertIterator.previous();
+ break;
+ }
+ }
+ value =
+ stringInsertIterator.insertConstStringInstruction(
+ appView, code, appView.dexItemFactory().createString(replacement));
+ }
iterator.next();
DexMethod invokedMethod = invoke.getInvokedMethod();
if (invoke.isInvokeConstructor(appView.dexItemFactory())) {
@@ -132,6 +151,16 @@
|| factory.stringBuilderMethods.isAppendStringMethod(method);
}
+ private DexMethod getConstructorWithStringParameter(
+ DexMethod invokedMethod, DexItemFactory factory) {
+ if (invokedMethod.getHolderType() == factory.stringBufferType) {
+ return factory.stringBufferMethods.stringConstructor;
+ } else {
+ assert invokedMethod.getHolderType() == factory.stringBuilderType;
+ return factory.stringBuilderMethods.stringConstructor;
+ }
+ }
+
private DexMethod getAppendWithStringParameter(
DexMethod invokedMethod, DexItemFactory factory) {
if (invokedMethod.getHolderType() == factory.stringBufferType) {
@@ -142,128 +171,4 @@
}
}
}
-
- class ReplaceByExistingString implements StringBuilderAction {
-
- private final Value existingString;
-
- public ReplaceByExistingString(Value existingString) {
- assert existingString.isNeverNull();
- this.existingString = existingString;
- }
-
- @Override
- public void perform(
- AppView<?> appView,
- IRCode code,
- InstructionListIterator iterator,
- Instruction instruction,
- StringBuilderOracle oracle) {
- instruction.outValue().replaceUsers(existingString);
- iterator.removeOrReplaceByDebugLocalRead();
- }
- }
-
- class ReplaceByStringConcat implements StringBuilderAction {
-
- private final Value first;
- private final Value second;
-
- private final String newConstant;
-
- private ReplaceByStringConcat(Value first, Value second, String newConstant) {
- assert first != null || newConstant != null;
- assert second != null || newConstant != null;
- this.first = first;
- this.second = second;
- this.newConstant = newConstant;
- }
-
- public static ReplaceByStringConcat replaceByValues(Value first, Value second) {
- return new ReplaceByStringConcat(first, second, null);
- }
-
- public static ReplaceByStringConcat replaceByNewConstantConcatValue(
- String newConstant, Value second) {
- return new ReplaceByStringConcat(null, second, newConstant);
- }
-
- public static ReplaceByStringConcat replaceByValueConcatNewConstant(
- Value first, String newConstant) {
- return new ReplaceByStringConcat(first, null, newConstant);
- }
-
- @Override
- public void perform(
- AppView<?> appView,
- IRCode code,
- InstructionListIterator iterator,
- Instruction instruction,
- StringBuilderOracle oracle) {
- Value constString = null;
- if (newConstant != null) {
- Instruction previous = iterator.previous();
- constString =
- insertStringConstantInstruction(appView, code, iterator, previous, newConstant);
- iterator.next();
- }
- assert first != null || constString != null;
- assert second != null || constString != null;
- // To ensure that we do not fail narrowing when evaluating String.concat, we mark the type
- // as maybe null.
- Value newOutValue =
- code.createValue(
- TypeElement.stringClassType(appView, Nullability.maybeNull()),
- instruction.getLocalInfo());
- iterator.replaceCurrentInstruction(
- InvokeVirtual.builder()
- .setOutValue(newOutValue)
- .setMethod(appView.dexItemFactory().stringMembers.concat)
- .setArguments(
- ImmutableList.of(
- first != null ? first : constString, second != null ? second : constString))
- .build());
- }
- }
-
- static DexMethod getConstructorWithStringParameter(
- DexMethod invokedMethod, DexItemFactory factory) {
- if (invokedMethod.getHolderType() == factory.stringBufferType) {
- return factory.stringBufferMethods.stringConstructor;
- } else {
- assert invokedMethod.getHolderType() == factory.stringBuilderType;
- return factory.stringBuilderMethods.stringConstructor;
- }
- }
-
- static Value insertStringConstantInstruction(
- AppView<?> appView,
- IRCode code,
- InstructionListIterator iterator,
- Instruction instruction,
- String newString) {
- // If the block has catch handlers, inserting a constant string in the same block as the
- // append violates our block representation in DEX since constant string is throwing. If the
- // append is in a block with catch handlers, we simply insert a new constant string in the
- // entry block after all arguments.
- Value value;
- if (!instruction.getBlock().hasCatchHandlers()) {
- value =
- iterator.insertConstStringInstruction(
- appView, code, appView.dexItemFactory().createString(newString));
- } else {
- InstructionListIterator stringInsertIterator = code.entryBlock().listIterator(code);
- while (stringInsertIterator.hasNext()) {
- Instruction next = stringInsertIterator.next();
- if (!next.isArgument()) {
- stringInsertIterator.previous();
- break;
- }
- }
- value =
- stringInsertIterator.insertConstStringInstruction(
- appView, code, appView.dexItemFactory().createString(newString));
- }
- return value;
- }
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAppendOptimizer.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAppendOptimizer.java
index 93c004f..ee85c13 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAppendOptimizer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderAppendOptimizer.java
@@ -33,7 +33,7 @@
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.EscapeNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.ImplicitToStringNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.InitNode;
-import com.android.tools.r8.ir.optimize.string.StringBuilderNode.InitOrAppendNode;
+import com.android.tools.r8.ir.optimize.string.StringBuilderNode.InitOrAppend;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.LoopNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.NewInstanceNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.SplitReferenceNode;
@@ -273,13 +273,7 @@
Value receiver = invoke.getReceiver();
if (oracle.isInit(instruction)) {
InitNode initNode = createInitNode(instruction.asInvokeDirect());
- String constantArgument = oracle.getConstantArgument(instruction);
- initNode.setConstantArgument(constantArgument);
- if (constantArgument == null
- && oracle.isStringConstructor(instruction)
- && invoke.getFirstNonReceiverArgument().isNeverNull()) {
- initNode.setNonConstantArgument(invoke.getFirstNonReceiverArgument());
- }
+ initNode.setConstantArgument(oracle.getConstantArgument(instruction));
if (invoke.arguments().size() == 2) {
Value arg = invoke.getOperand(1);
if (oracle.hasStringBuilderType(arg)) {
@@ -294,15 +288,9 @@
escaped -> nodeConsumer.accept(escaped, createInspectionNode(instruction)));
} else if (oracle.isAppend(instruction)) {
AppendNode appendNode = createAppendNode(instruction.asInvokeVirtual());
- String constantArgument = oracle.getConstantArgument(instruction);
- appendNode.setConstantArgument(constantArgument);
- Value arg = invoke.getFirstNonReceiverArgument();
- if (constantArgument == null
- && oracle.isAppendString(instruction)
- && arg.isNeverNull()) {
- appendNode.setNonConstantArgument(arg);
- }
- if (oracle.hasStringBuilderType(arg.getAliasedValue())) {
+ appendNode.setConstantArgument(oracle.getConstantArgument(instruction));
+ Value arg = invoke.getFirstNonReceiverArgument().getAliasedValue();
+ if (oracle.hasStringBuilderType(arg)) {
insertImplicitToStringNode(
arg, instruction, appendNode, escapeState, nodeConsumer);
}
@@ -353,7 +341,7 @@
private void insertImplicitToStringNode(
Value value,
Instruction instruction,
- InitOrAppendNode node,
+ InitOrAppend node,
StringBuilderEscapeState escapeState,
BiConsumer<Value, StringBuilderNode> nodeConsumer) {
assert escapeState.isLiveStringBuilder(value);
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNode.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNode.java
index 5e01833..c189299 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNode.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNode.java
@@ -9,7 +9,6 @@
import com.android.tools.r8.ir.code.InvokeMethod;
import com.android.tools.r8.ir.code.InvokeVirtual;
import com.android.tools.r8.ir.code.NewInstance;
-import com.android.tools.r8.ir.code.Value;
import com.google.common.collect.Sets;
import java.util.Set;
@@ -28,15 +27,7 @@
StringBuilderInstruction asStringBuilderInstructionNode();
}
- interface InitOrAppendNode extends StringBuilderInstruction {
-
- boolean isInitNode();
-
- boolean isAppendNode();
-
- InitNode asInitNode();
-
- AppendNode asAppendNode();
+ interface InitOrAppend extends StringBuilderInstruction {
boolean hasConstantArgument();
@@ -47,22 +38,6 @@
void setImplicitToStringNode(ImplicitToStringNode node);
ImplicitToStringNode getImplicitToStringNode();
-
- boolean hasNonConstantArgument();
-
- Value getNonConstantArgument();
-
- default boolean hasConstantOrNonConstantArgument() {
- return hasConstantArgument() || hasNonConstantArgument();
- }
-
- boolean hasSingleSuccessor();
-
- boolean hasSinglePredecessor();
-
- StringBuilderNode getSingleSuccessor();
-
- StringBuilderNode getSinglePredecessor();
}
private final Set<StringBuilderNode> successors = Sets.newLinkedHashSet();
@@ -94,11 +69,11 @@
return false;
}
- public boolean isInitNode() {
+ boolean isInitNode() {
return false;
}
- public boolean isAppendNode() {
+ boolean isAppendNode() {
return false;
}
@@ -130,15 +105,15 @@
return null;
}
- public InitNode asInitNode() {
+ InitNode asInitNode() {
return null;
}
- public AppendNode asAppendNode() {
+ AppendNode asAppendNode() {
return null;
}
- InitOrAppendNode asInitOrAppend() {
+ InitOrAppend asInitOrAppend() {
return null;
}
@@ -166,11 +141,11 @@
return isDead;
}
- public boolean hasSingleSuccessor() {
+ boolean hasSingleSuccessor() {
return successors.size() == 1;
}
- public StringBuilderNode getSingleSuccessor() {
+ StringBuilderNode getSingleSuccessor() {
assert hasSingleSuccessor();
return successors.iterator().next();
}
@@ -184,11 +159,11 @@
return successors;
}
- public boolean hasSinglePredecessor() {
+ boolean hasSinglePredecessor() {
return predecessors.size() == 1;
}
- public StringBuilderNode getSinglePredecessor() {
+ StringBuilderNode getSinglePredecessor() {
assert hasSinglePredecessor();
return predecessors.iterator().next();
}
@@ -303,19 +278,18 @@
/** An initNode is where a StringBuilder/StringBuffer is initialized. */
static class InitNode extends StringBuilderNode
- implements InitOrAppendNode, StringBuilderInstruction {
+ implements InitOrAppend, StringBuilderInstruction {
private final InvokeDirect instruction;
private ImplicitToStringNode implicitToStringNode;
private String constantArgument;
- private Value nonConstantArgument;
private InitNode(InvokeDirect instruction) {
this.instruction = instruction;
}
@Override
- public boolean isInitNode() {
+ boolean isInitNode() {
return true;
}
@@ -325,12 +299,12 @@
}
@Override
- public InitNode asInitNode() {
+ InitNode asInitNode() {
return this;
}
@Override
- InitOrAppendNode asInitOrAppend() {
+ InitOrAppend asInitOrAppend() {
return this;
}
@@ -355,6 +329,16 @@
}
@Override
+ public void setImplicitToStringNode(ImplicitToStringNode node) {
+ implicitToStringNode = node;
+ }
+
+ @Override
+ public ImplicitToStringNode getImplicitToStringNode() {
+ return implicitToStringNode;
+ }
+
+ @Override
public String getConstantArgument() {
return constantArgument;
}
@@ -363,53 +347,22 @@
public boolean hasConstantArgument() {
return constantArgument != null;
}
-
- public void setNonConstantArgument(Value value) {
- assert value.isNeverNull();
- this.nonConstantArgument = value;
- }
-
- @Override
- public void setImplicitToStringNode(ImplicitToStringNode node) {
- implicitToStringNode = node;
- }
-
- @Override
- public ImplicitToStringNode getImplicitToStringNode() {
- return implicitToStringNode;
- }
-
- @Override
- public boolean hasNonConstantArgument() {
- return nonConstantArgument != null;
- }
-
- @Override
- public Value getNonConstantArgument() {
- assert nonConstantArgument != null;
- return nonConstantArgument;
- }
-
- boolean isConstructorInvokeSideEffectFree(StringBuilderOracle oracle) {
- return oracle.isConstructorInvokeSideEffectFree(instruction);
- }
}
/** AppendNodes are StringBuilder.append or StringBuffer.append. */
static class AppendNode extends StringBuilderNode
- implements InitOrAppendNode, StringBuilderInstruction {
+ implements InitOrAppend, StringBuilderInstruction {
private final InvokeVirtual instruction;
private ImplicitToStringNode implicitToStringNode;
private String constantArgument;
- private Value nonConstantArgument;
private AppendNode(InvokeVirtual instruction) {
this.instruction = instruction;
}
@Override
- public boolean isAppendNode() {
+ boolean isAppendNode() {
return true;
}
@@ -419,12 +372,12 @@
}
@Override
- public AppendNode asAppendNode() {
+ AppendNode asAppendNode() {
return this;
}
@Override
- InitOrAppendNode asInitOrAppend() {
+ InitOrAppend asInitOrAppend() {
return this;
}
@@ -459,21 +412,6 @@
}
@Override
- public boolean hasNonConstantArgument() {
- return nonConstantArgument != null;
- }
-
- public void setNonConstantArgument(Value value) {
- assert value.isNeverNull();
- this.nonConstantArgument = value;
- }
-
- @Override
- public Value getNonConstantArgument() {
- return nonConstantArgument;
- }
-
- @Override
public String getConstantArgument() {
return constantArgument;
}
@@ -602,13 +540,13 @@
*/
static class ImplicitToStringNode extends StringBuilderNode {
- private final InitOrAppendNode initOrAppend;
+ private final InitOrAppend initOrAppend;
- ImplicitToStringNode(InitOrAppendNode initOrAppend) {
+ ImplicitToStringNode(InitOrAppend initOrAppend) {
this.initOrAppend = initOrAppend;
}
- public InitOrAppendNode getInitOrAppend() {
+ public InitOrAppend getInitOrAppend() {
return initOrAppend;
}
@@ -663,7 +601,7 @@
return new OtherStringBuilderNode(instruction);
}
- static ImplicitToStringNode createImplicitToStringNode(InitOrAppendNode otherNode) {
+ static ImplicitToStringNode createImplicitToStringNode(InitOrAppend otherNode) {
return new ImplicitToStringNode(otherNode);
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNodeMuncher.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNodeMuncher.java
index 3bf5773..0659a8a 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNodeMuncher.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderNodeMuncher.java
@@ -9,18 +9,14 @@
import com.android.tools.r8.ir.optimize.string.StringBuilderAction.AppendWithNewConstantString;
import com.android.tools.r8.ir.optimize.string.StringBuilderAction.RemoveStringBuilderAction;
import com.android.tools.r8.ir.optimize.string.StringBuilderAction.ReplaceByConstantString;
-import com.android.tools.r8.ir.optimize.string.StringBuilderAction.ReplaceByExistingString;
-import com.android.tools.r8.ir.optimize.string.StringBuilderAction.ReplaceByStringConcat;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.AppendNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.ImplicitToStringNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.InitNode;
-import com.android.tools.r8.ir.optimize.string.StringBuilderNode.InitOrAppendNode;
+import com.android.tools.r8.ir.optimize.string.StringBuilderNode.InitOrAppend;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.NewInstanceNode;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.StringBuilderInstruction;
import com.android.tools.r8.ir.optimize.string.StringBuilderNode.ToStringNode;
import com.android.tools.r8.utils.WorkList;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
@@ -71,14 +67,6 @@
public boolean isLooping(StringBuilderNode root) {
return looping.contains(root);
}
-
- public boolean isEscaping(StringBuilderNode root) {
- return escaping.contains(root);
- }
-
- public boolean isInspecting(StringBuilderNode root) {
- return inspectingCapacity.contains(root);
- }
}
private interface PeepholePattern {
@@ -105,32 +93,30 @@
@Override
public boolean optimize(
StringBuilderNode root, StringBuilderNode currentNode, MunchingState munchingState) {
- AppendNode appendNode = currentNode.asAppendNode();
- if (appendNode == null || !appendNode.hasSinglePredecessor()) {
+ if (!currentNode.isAppendNode()) {
return false;
}
- InitOrAppendNode previous = currentNode.getSinglePredecessor().asInitOrAppend();
- if (previous == null || !previous.hasSingleSuccessor()) {
+ String currentConstantArgument = getConstantArgumentForNode(currentNode, munchingState);
+ if (currentConstantArgument == null || !currentNode.hasSinglePredecessor()) {
+ return false;
+ }
+ StringBuilderNode previous = currentNode.getSinglePredecessor();
+ String previousConstantArgument = getConstantArgumentForNode(previous, munchingState);
+ if (previousConstantArgument == null || !previous.hasSingleSuccessor()) {
return false;
}
// The capacity changes based on the init call (on JVM it adds 16 to length of input).
if (previous.isInitNode() && munchingState.inspectingCapacity.contains(root)) {
return false;
}
- String currentConstantArgument = getConstantArgumentForNode(appendNode, munchingState);
- if (currentConstantArgument == null) {
- return false;
- }
- String previousConstantArgument = getConstantArgumentForNode(previous, munchingState);
- if (previousConstantArgument == null) {
- return false;
- }
+ assert previous.isInitOrAppend();
String newConstant = previousConstantArgument + currentConstantArgument;
- previous.setConstantArgument(newConstant);
+ InitOrAppend initOrAppend = previous.asInitOrAppend();
+ initOrAppend.setConstantArgument(newConstant);
munchingState.actions.put(
- previous.getInstruction(), new AppendWithNewConstantString(newConstant));
+ initOrAppend.getInstruction(), new AppendWithNewConstantString(newConstant));
munchingState.actions.put(
- appendNode.getInstruction(), RemoveStringBuilderAction.getInstance());
+ currentNode.asAppendNode().getInstruction(), RemoveStringBuilderAction.getInstance());
currentNode.removeNode();
return true;
}
@@ -140,13 +126,13 @@
* This peephole will try to remove toString nodes and replace by a constant string:
*
* <pre>
- * newInstance -> init("foo") -> toString() => newInstance -> init("foo") -> append("bar")
- * actions: [toString() => ReplaceByConstantString("foo")]
+ * newInstance -> init("foo") -> append("bar") -> toString() =>
+ * newInstance -> init("foo") -> append("bar")
* </pre>
*
* <p>If the node is an implicitToString, we update the append of another builder to have the new
- * constant value directly. If not, we keep track of the outValue toString() being replaced by a
- * constant by updating {@code MunchingState.optimizedStrings}
+ * constant value directly. If not, we keep track of the outValue toString() had is replaced by a
+ * constant, by updating {@code MunchingState.optimizedStrings}
*/
private static class MunchToString implements PeepholePattern {
@@ -155,9 +141,6 @@
StringBuilderNode originalRoot,
StringBuilderNode currentNode,
MunchingState munchingState) {
- if (munchingState.isEscaping(originalRoot) || munchingState.isInspecting(originalRoot)) {
- return false;
- }
if (!currentNode.isToStringNode() && !currentNode.isImplicitToStringNode()) {
return false;
}
@@ -165,127 +148,48 @@
if (newInstanceNode == null || !newInstanceNode.hasSingleSuccessor()) {
return false;
}
- InitNode init = newInstanceNode.getSingleSuccessor().asInitNode();
- if (init == null || !init.hasSingleSuccessor()) {
+ StringBuilderNode init = newInstanceNode.getSingleSuccessor();
+ String rootConstantArgument = getConstantArgumentForNode(init, munchingState);
+ if (rootConstantArgument == null || !init.isInitNode()) {
return false;
}
- if (!currentNode.hasSinglePredecessor() || currentNode.getSinglePredecessor() != init) {
- return false;
- }
- String initConstantArgument = getConstantArgumentForNode(init, munchingState);
- if (initConstantArgument == null) {
- return false;
- }
+ // This is either <init>(str) -> toString() or <init>(str) -> append(str) -> toString()
// If the string builder dependency is directly given to another string builder, there is
// no toString() but an append with this string builder as argument.
+ if (!currentNode.hasSinglePredecessor() || !init.hasSingleSuccessor()) {
+ return false;
+ }
+ String constantArgument = null;
+ if (currentNode.getSinglePredecessor() == init) {
+ constantArgument = rootConstantArgument;
+ } else {
+ StringBuilderNode expectedAppend = init.getSingleSuccessor();
+ StringBuilderNode expectedSameAppend = currentNode.getSinglePredecessor();
+ String appendConstantArgument = getConstantArgumentForNode(expectedAppend, munchingState);
+ if (expectedAppend == expectedSameAppend && appendConstantArgument != null) {
+ // TODO(b/190489514): See if this larger pattern is necessary.
+ assert false : "See why this larger pattern is necessary";
+ constantArgument = rootConstantArgument + appendConstantArgument;
+ }
+ }
+ if (constantArgument == null) {
+ return false;
+ }
if (currentNode.isToStringNode()) {
ToStringNode toStringNode = currentNode.asToStringNode();
munchingState.actions.put(
- toStringNode.getInstruction(), new ReplaceByConstantString(initConstantArgument));
+ toStringNode.getInstruction(), new ReplaceByConstantString(constantArgument));
String oldValue =
munchingState.optimizedStrings.put(
- toStringNode.getInstruction().outValue(), initConstantArgument);
+ toStringNode.getInstruction().outValue(), constantArgument);
assert oldValue == null;
} else {
assert currentNode.isImplicitToStringNode();
ImplicitToStringNode implicitToStringNode = currentNode.asImplicitToStringNode();
- InitOrAppendNode initOrAppend = implicitToStringNode.getInitOrAppend();
- initOrAppend.setConstantArgument(initConstantArgument);
+ InitOrAppend initOrAppend = implicitToStringNode.getInitOrAppend();
+ initOrAppend.setConstantArgument(constantArgument);
munchingState.actions.put(
- initOrAppend.getInstruction(), new AppendWithNewConstantString(initConstantArgument));
- }
- munchingState.materializingInstructions.get(originalRoot).remove(currentNode);
- currentNode.removeNode();
- return true;
- }
- }
-
- /**
- * This peephole will try to remove toString nodes and replace by an invoke to String.concat:
- *
- * <pre>
- * newInstance -> init(notNull(string)) -> append(notNull(otherString)) -> toString() =>
- * newInstance -> init(notNull(string)) -> append(otherString)
- * actions: [toString() => string.concat(otherString)]
- * </pre>
- *
- * <p>This pattern only triggers when a constant munching of toString could happen.
- */
- private static class MunchToStringIntoStringConcat implements PeepholePattern {
-
- @Override
- public boolean optimize(
- StringBuilderNode originalRoot,
- StringBuilderNode currentNode,
- MunchingState munchingState) {
- if (munchingState.isEscaping(originalRoot) || munchingState.isInspecting(originalRoot)) {
- return false;
- }
- // TODO(b/129200243): Handle implicit tostring nodes.
- if (!currentNode.isToStringNode() || !currentNode.hasSinglePredecessor()) {
- return false;
- }
- NewInstanceNode newInstanceNode = munchingState.getNewInstanceNode(originalRoot);
- if (newInstanceNode == null || !newInstanceNode.hasSingleSuccessor()) {
- return false;
- }
- InitOrAppendNode firstNode = newInstanceNode.getSingleSuccessor().asInitNode();
- if (firstNode == null || !firstNode.hasSingleSuccessor()) {
- return false;
- }
- if (firstNode.asInitNode().isConstructorInvokeSideEffectFree(munchingState.oracle)
- && "".equals(firstNode.getConstantArgument())
- && firstNode.hasSingleSuccessor()) {
- firstNode = firstNode.getSingleSuccessor().asAppendNode();
- if (firstNode == null
- || !firstNode.hasSinglePredecessor()
- || !firstNode.hasSingleSuccessor()) {
- return false;
- }
- }
- // We cannot String.concat or return the string safely when it is not constant and maybe null.
- if (!firstNode.hasConstantOrNonConstantArgument()) {
- return false;
- }
- List<InitOrAppendNode> initOrAppends = Lists.newArrayList(firstNode);
- if (currentNode.getSinglePredecessor() != firstNode) {
- AppendNode appendAfterFirstNode = firstNode.getSingleSuccessor().asAppendNode();
- AppendNode appendBeforeToString = currentNode.getSinglePredecessor().asAppendNode();
- if (appendAfterFirstNode == null
- || appendAfterFirstNode != appendBeforeToString
- || !appendAfterFirstNode.hasConstantOrNonConstantArgument()) {
- return false;
- }
- initOrAppends.add(appendAfterFirstNode);
- }
- // Check that all values are not constant otherwise we can compute the constant value and
- // replace all entirely.
- if (Iterables.all(initOrAppends, InitOrAppendNode::hasConstantArgument)) {
- return false;
- }
- // Replace with the string itself.
- Instruction currentInstruction = currentNode.asToStringNode().getInstruction();
- InitOrAppendNode first = initOrAppends.get(0);
- if (initOrAppends.size() == 1) {
- munchingState.actions.put(
- currentInstruction, new ReplaceByExistingString(first.getNonConstantArgument()));
- } else {
- InitOrAppendNode second = initOrAppends.get(1);
- ReplaceByStringConcat concatAction;
- if (first.hasConstantArgument()) {
- concatAction =
- ReplaceByStringConcat.replaceByNewConstantConcatValue(
- first.getConstantArgument(), second.getNonConstantArgument());
- } else if (second.hasConstantArgument()) {
- concatAction =
- ReplaceByStringConcat.replaceByValueConcatNewConstant(
- first.getNonConstantArgument(), second.getConstantArgument());
- } else {
- concatAction =
- ReplaceByStringConcat.replaceByValues(
- first.getNonConstantArgument(), second.getNonConstantArgument());
- }
- munchingState.actions.put(currentInstruction, concatAction);
+ initOrAppend.getInstruction(), new AppendWithNewConstantString(constantArgument));
}
munchingState.materializingInstructions.get(originalRoot).remove(currentNode);
currentNode.removeNode();
@@ -294,11 +198,21 @@
}
private static String getConstantArgumentForNode(
- InitOrAppendNode node, MunchingState munchingState) {
- if (node.hasConstantArgument()) {
- return node.getConstantArgument();
+ StringBuilderNode node, MunchingState munchingState) {
+ if (node.isAppendNode()) {
+ AppendNode appendNode = node.asAppendNode();
+ if (appendNode.hasConstantArgument()) {
+ return appendNode.getConstantArgument();
+ }
+ return getOptimizedConstantArgument(appendNode, munchingState);
+ } else if (node.isInitNode()) {
+ InitNode initNode = node.asInitNode();
+ if (initNode.hasConstantArgument()) {
+ return initNode.getConstantArgument();
+ }
+ return getOptimizedConstantArgument(initNode, munchingState);
}
- return getOptimizedConstantArgument(node, munchingState);
+ return null;
}
private static String getOptimizedConstantArgument(
@@ -348,6 +262,7 @@
removeNode = true;
}
} else if (currentNode.isInitNode()
+ && currentNode.asInitNode().hasConstantArgument()
&& currentNode.hasSinglePredecessor()
&& currentNode.getSinglePredecessor().isNewInstanceNode()
&& currentNode.getSuccessors().isEmpty()
@@ -397,12 +312,7 @@
}
private static final PeepholePattern[] peepholePatterns =
- new PeepholePattern[] {
- new MunchAppends(),
- new MunchToString(),
- new MunchToStringIntoStringConcat(),
- new MunchNonMaterializing()
- };
+ new PeepholePattern[] {new MunchAppends(), new MunchToString(), new MunchNonMaterializing()};
static boolean optimize(
StringBuilderNode root, StringBuilderNode currentNode, MunchingState munchingState) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOracle.java b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOracle.java
index 8b59c2f..063f791 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOracle.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/string/StringBuilderOracle.java
@@ -43,12 +43,6 @@
boolean isInit(Instruction instruction);
- boolean isAppendString(Instruction instruction);
-
- boolean isStringConstructor(Instruction instruction);
-
- boolean isConstructorInvokeSideEffectFree(Instruction instruction);
-
class DefaultStringBuilderOracle implements StringBuilderOracle {
private final DexItemFactory factory;
@@ -191,12 +185,12 @@
}
DexMethod invokedMethod = instruction.asInvokeMethod().getInvokedMethod();
if (factory.stringBuilderMethods.isAppendObjectOrCharSequenceMethod(invokedMethod)
- || factory.stringBufferMethods.isAppendObjectOrCharSequenceMethod(invokedMethod)) {
+ || factory.stringBufferMethods.isAppendObjectOrCharSequenceMethod(invokedMethod)
+ || factory.stringBuilderMethods.charSequenceConstructor == invokedMethod
+ || factory.stringBufferMethods.charSequenceConstructor == invokedMethod) {
+ assert instruction.inValues().size() == 2;
return !instruction.inValues().get(1).getType().isStringType(factory);
}
- if (invokedMethod.isInstanceInitializer(factory)) {
- return !isConstructorInvokeSideEffectFree(instruction);
- }
if (factory.stringBuilderMethods.isAppendCharArrayMethod(invokedMethod)
|| factory.stringBufferMethods.isAppendCharArrayMethod(invokedMethod)) {
return instruction.asInvokeVirtual().getFirstNonReceiverArgument().isMaybeNull();
@@ -213,41 +207,5 @@
return factory.stringBuilderMethods.isConstructorMethod(invokedMethod)
|| factory.stringBufferMethods.isConstructorMethod(invokedMethod);
}
-
- @Override
- public boolean isAppendString(Instruction instruction) {
- if (!instruction.isInvokeMethod()) {
- return false;
- }
- DexMethod invokedMethod = instruction.asInvokeMethod().getInvokedMethod();
- return factory.stringBuilderMethods.isAppendStringMethod(invokedMethod)
- || factory.stringBufferMethods.isAppendStringMethod(invokedMethod);
- }
-
- @Override
- public boolean isStringConstructor(Instruction instruction) {
- if (!instruction.isInvokeMethod()) {
- return false;
- }
- DexMethod invokedMethod = instruction.asInvokeMethod().getInvokedMethod();
- return invokedMethod == factory.stringBuilderMethods.stringConstructor
- || invokedMethod == factory.stringBufferMethods.stringConstructor;
- }
-
- @Override
- public boolean isConstructorInvokeSideEffectFree(Instruction instruction) {
- if (!instruction.isInvokeConstructor(factory)) {
- return false;
- }
- DexMethod invokedMethod = instruction.asInvokeMethod().getInvokedMethod();
- if (invokedMethod.getHolderType() == factory.stringBuilderType) {
- return factory.stringBuilderMethods.constructorInvokeIsSideEffectFree(
- invokedMethod, instruction.inValues());
- } else {
- assert invokedMethod.getHolderType() == factory.stringBufferType;
- return factory.stringBufferMethods.constructorInvokeIsSideEffectFree(
- invokedMethod, instruction.inValues());
- }
- }
}
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullAppendConstantAppendNotNullTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullAppendConstantAppendNotNullTest.java
index 4646691..d4ac494 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullAppendConstantAppendNotNullTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullAppendConstantAppendNotNullTest.java
@@ -53,6 +53,7 @@
MethodSubject methodSubject = inspect.clazz(Main.class).mainMethod();
assertThat(methodSubject, isPresent());
assertEquals(1, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
+ // TODO(b/129200243): Should only have two appends.
assertEquals(3, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
});
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullAppendNotNullTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullAppendNotNullTest.java
index cbe8fa7..c28729a 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullAppendNotNullTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullAppendNotNullTest.java
@@ -13,7 +13,6 @@
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.utils.codeinspector.CodeMatchers;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -53,9 +52,9 @@
inspect -> {
MethodSubject methodSubject = inspect.clazz(Main.class).mainMethod();
assertThat(methodSubject, isPresent());
- assertEquals(0, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
- assertEquals(0, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
- assertThat(methodSubject, CodeMatchers.invokesMethodWithName("concat"));
+ // TODO(b/129200243): Should be String.concat.
+ assertEquals(1, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
+ assertEquals(2, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
});
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullTest.java
index 8005ed5..d2c95ab 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatAppendNotNullTest.java
@@ -7,14 +7,12 @@
import static com.android.tools.r8.ir.optimize.string.utils.StringBuilderCodeMatchers.countStringBuilderAppends;
import static com.android.tools.r8.ir.optimize.string.utils.StringBuilderCodeMatchers.countStringBuilderInits;
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 com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.utils.codeinspector.CodeMatchers;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -54,9 +52,9 @@
inspect -> {
MethodSubject methodSubject = inspect.clazz(Main.class).mainMethod();
assertThat(methodSubject, isPresent());
- assertEquals(0, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
- assertEquals(0, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
- assertThat(methodSubject, not(CodeMatchers.invokesMethodWithName("concat")));
+ // TODO(b/129200243): This should just be arg.toString()
+ assertEquals(1, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
+ assertEquals(1, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
});
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatConstantInitTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatConstantInitTest.java
index 0f7d286..9b9280f 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatConstantInitTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatConstantInitTest.java
@@ -13,7 +13,6 @@
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.utils.codeinspector.CodeMatchers;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -53,9 +52,9 @@
inspect -> {
MethodSubject methodSubject = inspect.clazz(Main.class).mainMethod();
assertThat(methodSubject, isPresent());
- assertEquals(0, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
- assertEquals(0, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
- assertThat(methodSubject, CodeMatchers.invokesMethodWithName("concat"));
+ // TODO(b/129200243): Should be String.concat.
+ assertEquals(1, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
+ assertEquals(1, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
});
}
@@ -64,7 +63,7 @@
public static void main(String[] args) {
String arg = System.currentTimeMillis() > 0 ? "o" : null;
if (arg != null) {
- // The optimization should join "ok" into init and then concat with arg.
+ // The optimization should join "ok" into init and then append with arg.
System.out.println(new StringBuilder("o").append("k").append(arg).toString());
}
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatInitNotNullAppendNotNullTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatInitNotNullAppendNotNullTest.java
index aca301c..4dce8fd 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatInitNotNullAppendNotNullTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatInitNotNullAppendNotNullTest.java
@@ -13,7 +13,6 @@
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.utils.codeinspector.CodeMatchers;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -53,9 +52,9 @@
inspect -> {
MethodSubject methodSubject = inspect.clazz(Main.class).mainMethod();
assertThat(methodSubject, isPresent());
- assertEquals(0, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
- assertEquals(0, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
- assertThat(methodSubject, CodeMatchers.invokesMethodWithName("concat"));
+ // TODO(b/129200243): Should be concat.
+ assertEquals(1, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
+ assertEquals(1, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
});
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatInitNotNullTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatInitNotNullTest.java
index 4bb550e..690a604 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatInitNotNullTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatInitNotNullTest.java
@@ -7,14 +7,12 @@
import static com.android.tools.r8.ir.optimize.string.utils.StringBuilderCodeMatchers.countStringBuilderAppends;
import static com.android.tools.r8.ir.optimize.string.utils.StringBuilderCodeMatchers.countStringBuilderInits;
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 com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.utils.codeinspector.CodeMatchers;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -54,9 +52,9 @@
inspect -> {
MethodSubject methodSubject = inspect.clazz(Main.class).mainMethod();
assertThat(methodSubject, isPresent());
- assertEquals(0, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
+ // TODO(b/129200243): This should just be the string as is.
+ assertEquals(1, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
assertEquals(0, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
- assertThat(methodSubject, not(CodeMatchers.invokesMethodWithName("concat")));
});
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatNoRewriteConstantsTest.java b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatNoRewriteConstantsTest.java
index 33320dc..73e61bc 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatNoRewriteConstantsTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/string/StringConcatNoRewriteConstantsTest.java
@@ -7,14 +7,12 @@
import static com.android.tools.r8.ir.optimize.string.utils.StringBuilderCodeMatchers.countStringBuilderAppends;
import static com.android.tools.r8.ir.optimize.string.utils.StringBuilderCodeMatchers.countStringBuilderInits;
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 com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.utils.codeinspector.CodeMatchers;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -56,7 +54,6 @@
assertThat(methodSubject, isPresent());
assertEquals(0, countStringBuilderInits(methodSubject.asFoundMethodSubject()));
assertEquals(0, countStringBuilderAppends(methodSubject.asFoundMethodSubject()));
- assertThat(methodSubject, not(CodeMatchers.invokesMethodWithName("concat")));
});
}
