Initial class inlining of proto builders
Bug: 142772856, 142762134, 145903274
Change-Id: I24a19fbb359d3520926aafe19b10f386c3ff7d08
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
index 279bd8d..9afba98 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
@@ -335,6 +335,8 @@
continue;
}
}
+ assert !isExtraMethodCall(invoke);
+ return user; // Not eligible.
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java b/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java
index 8f880b6..614258b 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java
@@ -18,6 +18,10 @@
import static com.android.tools.r8.ir.code.Opcodes.INSTANCE_OF;
import static com.android.tools.r8.ir.code.Opcodes.INSTANCE_PUT;
import static com.android.tools.r8.ir.code.Opcodes.INVOKE_DIRECT;
+import static com.android.tools.r8.ir.code.Opcodes.INVOKE_INTERFACE;
+import static com.android.tools.r8.ir.code.Opcodes.INVOKE_STATIC;
+import static com.android.tools.r8.ir.code.Opcodes.INVOKE_VIRTUAL;
+import static com.android.tools.r8.ir.code.Opcodes.NEW_INSTANCE;
import static com.android.tools.r8.ir.code.Opcodes.RETURN;
import static com.android.tools.r8.ir.code.Opcodes.STATIC_GET;
import static com.android.tools.r8.ir.code.Opcodes.THROW;
@@ -49,6 +53,8 @@
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.InstructionIterator;
import com.android.tools.r8.ir.code.InvokeDirect;
+import com.android.tools.r8.ir.code.InvokeMethod;
+import com.android.tools.r8.ir.code.NewInstance;
import com.android.tools.r8.ir.code.Return;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.ir.optimize.DynamicTypeOptimization;
@@ -396,6 +402,7 @@
|| instancePut.instructionInstanceCanThrow(appView, clazz.type).isThrowing()) {
builder.setMayHaveOtherSideEffectsThanInstanceFieldAssignments();
}
+
Value value = instancePut.value().getAliasedValue();
// TODO(b/142762134): Replace the use of onlyDependsOnArgument() by
// ValueMayDependOnEnvironmentAnalysis.
@@ -429,8 +436,16 @@
builder.merge(singleTarget.getOptimizationInfo().getInstanceInitializerInfo());
for (int i = 1; i < invoke.arguments().size(); i++) {
Value argument = invoke.arguments().get(i).getAliasedValue();
- if (argument == receiver || !argument.onlyDependsOnArgument()) {
- return null;
+ if (argument == receiver) {
+ // In the analysis of the parent constructor, we don't consider the non-receiver
+ // arguments as being aliases of the receiver. Therefore, we explicitly mark
+ // that the receiver escapes from this constructor.
+ builder.setReceiverMayEscapeOutsideConstructorChain();
+ }
+ if (!argument.onlyDependsOnArgument()) {
+ // If the parent constructor assigns this argument into a field, then the value
+ // of the field may depend on the environment.
+ builder.setInstanceFieldInitializationMayDependOnEnvironment();
}
}
builder.setParent(invokedMethod);
@@ -448,6 +463,31 @@
}
break;
+ case INVOKE_INTERFACE:
+ case INVOKE_STATIC:
+ case INVOKE_VIRTUAL:
+ InvokeMethod invoke = instruction.asInvokeMethod();
+ builder.markAllFieldsAsRead().setMayHaveOtherSideEffectsThanInstanceFieldAssignments();
+ for (Value inValue : invoke.inValues()) {
+ if (inValue.getAliasedValue() == receiver) {
+ builder.setReceiverMayEscapeOutsideConstructorChain();
+ break;
+ }
+ }
+ break;
+
+ case NEW_INSTANCE:
+ {
+ NewInstance newInstance = instruction.asNewInstance();
+ if (newInstance.instructionMayHaveSideEffects(appView, clazz.type)) {
+ // It could trigger a class initializer.
+ builder
+ .markAllFieldsAsRead()
+ .setMayHaveOtherSideEffectsThanInstanceFieldAssignments();
+ }
+ }
+ break;
+
default:
builder
.markAllFieldsAsRead()
diff --git a/src/test/java/com/android/tools/r8/bridgeremoval/bridgestokeep/KeepNonVisibilityBridgeMethodsTest.java b/src/test/java/com/android/tools/r8/bridgeremoval/bridgestokeep/KeepNonVisibilityBridgeMethodsTest.java
index 249e39e..c188c87 100644
--- a/src/test/java/com/android/tools/r8/bridgeremoval/bridgestokeep/KeepNonVisibilityBridgeMethodsTest.java
+++ b/src/test/java/com/android/tools/r8/bridgeremoval/bridgestokeep/KeepNonVisibilityBridgeMethodsTest.java
@@ -6,7 +6,6 @@
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.Assert.assertTrue;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
@@ -27,7 +26,9 @@
@Parameters(name = "{1}, minification: {0}")
public static List<Object[]> data() {
- return buildParameters(BooleanUtils.values(), getTestParameters().withDexRuntimes().build());
+ return buildParameters(
+ BooleanUtils.values(),
+ getTestParameters().withDexRuntimes().withAllRuntimesAndApiLevels().build());
}
public KeepNonVisibilityBridgeMethodsTest(boolean minification, TestParameters parameters) {
@@ -50,12 +51,13 @@
" synthetic void registerObserver(...);",
"}")
.allowAccessModification()
+ .enableClassInliningAnnotations()
// TODO(b/120764902): MemberSubject.getOriginalName() is not working without the @NeverMerge
// annotation on DataAdapter.Observer.
.enableMergeAnnotations()
.enableProguardTestOptions()
.minification(minification)
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.compile()
.inspect(
inspector -> {
@@ -63,7 +65,7 @@
assertThat(classSubject, isPresent());
MethodSubject subject = classSubject.uniqueMethodWithName("registerObserver");
- assertTrue(subject.isPresent());
+ assertThat(subject, isPresent());
})
.run(parameters.getRuntime(), Main.class)
.assertSuccess();
diff --git a/src/test/java/com/android/tools/r8/bridgeremoval/bridgestokeep/SimpleDataAdapter.java b/src/test/java/com/android/tools/r8/bridgeremoval/bridgestokeep/SimpleDataAdapter.java
index 148a61e..cc60012 100644
--- a/src/test/java/com/android/tools/r8/bridgeremoval/bridgestokeep/SimpleDataAdapter.java
+++ b/src/test/java/com/android/tools/r8/bridgeremoval/bridgestokeep/SimpleDataAdapter.java
@@ -4,8 +4,10 @@
package com.android.tools.r8.bridgeremoval.bridgestokeep;
-public class SimpleDataAdapter
- extends SimpleObservableList<DataAdapter.Observer>
+import com.android.tools.r8.NeverClassInline;
+
+@NeverClassInline
+public class SimpleDataAdapter extends SimpleObservableList<DataAdapter.Observer>
implements DataAdapter {
public SimpleDataAdapter() {
diff --git a/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderShrinkingTest.java b/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderShrinkingTest.java
index 3127150..82362aa 100644
--- a/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderShrinkingTest.java
+++ b/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderShrinkingTest.java
@@ -7,7 +7,6 @@
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.Assume.assumeTrue;
import com.android.tools.r8.R8TestCompileResult;
import com.android.tools.r8.TestParameters;
@@ -80,10 +79,6 @@
.compile()
.inspect(this::inspect);
- // TODO(b/112437944): Should never allow dynamicMethod() to be inlined unless MethodToInvoke is
- // guaranteed to be different from MethodToInvoke.BUILD_MESSAGE_INFO.
- assumeTrue(mains.size() > 1);
-
for (String main : mains) {
result.run(parameters.getRuntime(), main).assertSuccessWithOutput(getExpectedOutput(main));
}
@@ -160,39 +155,19 @@
}
private void inspect(CodeInspector outputInspector) {
- // TODO(b/112437944): Should only be present if proto2.BuilderWithReusedSettersTestClass.main()
- // is kept.
- assertThat(outputInspector.clazz(LITE_BUILDER), isPresent());
-
- // TODO(b/112437944): Should be absent.
+ boolean primitivesBuilderShouldBeLive =
+ mains.contains("proto2.BuilderWithReusedSettersTestClass");
assertThat(
- outputInspector.clazz("com.android.tools.r8.proto2.TestProto$NestedMessage$Builder"),
- isNestedMessageBuilderUsed(mains) ? isPresent() : not(isPresent()));
-
- // TODO(b/112437944): Should be absent.
- assertThat(
- outputInspector.clazz("com.android.tools.r8.proto2.TestProto$OuterMessage$Builder"),
- isOuterMessageBuilderUsed(mains) ? isPresent() : not(isPresent()));
-
- // TODO(b/112437944): Should only be present if proto2.BuilderWithReusedSettersTestClass.main()
- // is kept.
+ outputInspector.clazz(LITE_BUILDER),
+ primitivesBuilderShouldBeLive ? isPresent() : not(isPresent()));
assertThat(
outputInspector.clazz("com.android.tools.r8.proto2.TestProto$Primitives$Builder"),
- isPrimitivesBuilderUsed(mains) ? isPresent() : not(isPresent()));
- }
-
- private static boolean isNestedMessageBuilderUsed(List<String> mains) {
- return mains.contains("proto2.BuilderWithProtoBuilderSetterTestClass")
- || mains.contains("proto2.BuilderWithProtoSetterTestClass");
- }
-
- private static boolean isOuterMessageBuilderUsed(List<String> mains) {
- return isNestedMessageBuilderUsed(mains);
- }
-
- private static boolean isPrimitivesBuilderUsed(List<String> mains) {
- return mains.contains("proto2.BuilderWithOneofSetterTestClass")
- || mains.contains("proto2.BuilderWithPrimitiveSettersTestClass")
- || mains.contains("proto2.BuilderWithReusedSettersTestClass");
+ primitivesBuilderShouldBeLive ? isPresent() : not(isPresent()));
+ assertThat(
+ outputInspector.clazz("com.android.tools.r8.proto2.TestProto$OuterMessage$Builder"),
+ not(isPresent()));
+ assertThat(
+ outputInspector.clazz("com.android.tools.r8.proto2.TestProto$NestedMessage$Builder"),
+ not(isPresent()));
}
}
diff --git a/src/test/java/com/android/tools/r8/internal/proto/Proto2ShrinkingTest.java b/src/test/java/com/android/tools/r8/internal/proto/Proto2ShrinkingTest.java
index aed2c26..eb62734 100644
--- a/src/test/java/com/android/tools/r8/internal/proto/Proto2ShrinkingTest.java
+++ b/src/test/java/com/android/tools/r8/internal/proto/Proto2ShrinkingTest.java
@@ -79,7 +79,6 @@
.addKeepMainRule("proto2.TestClass")
.addKeepRuleFiles(PROTOBUF_LITE_PROGUARD_RULES)
.addKeepRules(allGeneratedMessageLiteSubtypesAreInstantiatedRule())
- .addKeepRules(alwaysInlineNewSingularGeneratedExtensionRule())
.addOptionsModification(
options -> {
options.enableFieldBitAccessAnalysis = true;
diff --git a/src/test/java/com/android/tools/r8/internal/proto/ProtoShrinkingTestBase.java b/src/test/java/com/android/tools/r8/internal/proto/ProtoShrinkingTestBase.java
index 19b3857..a1ecab4 100644
--- a/src/test/java/com/android/tools/r8/internal/proto/ProtoShrinkingTestBase.java
+++ b/src/test/java/com/android/tools/r8/internal/proto/ProtoShrinkingTestBase.java
@@ -77,14 +77,6 @@
"}");
}
- static String alwaysInlineNewSingularGeneratedExtensionRule() {
- return StringUtils.lines(
- "-alwaysinline class com.google.protobuf.GeneratedMessageLite {",
- " com.google.protobuf.GeneratedMessageLite$GeneratedExtension"
- + " newSingularGeneratedExtension(...);",
- "}");
- }
-
static String keepAllProtosRule() {
return StringUtils.lines(
"-keep class * extends com.google.protobuf.GeneratedMessageLite { *; }");
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlineInstanceInitializerWithIndirectEscapingReceiverTest.java b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlineInstanceInitializerWithIndirectEscapingReceiverTest.java
new file mode 100644
index 0000000..c86e0fc
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlineInstanceInitializerWithIndirectEscapingReceiverTest.java
@@ -0,0 +1,93 @@
+// Copyright (c) 2019, 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.ir.optimize.classinliner;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.NeverMerge;
+import com.android.tools.r8.NeverPropagateValue;
+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.CodeInspector;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class ClassInlineInstanceInitializerWithIndirectEscapingReceiverTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public ClassInlineInstanceInitializerWithIndirectEscapingReceiverTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(ClassInlineInstanceInitializerWithIndirectEscapingReceiverTest.class)
+ .addKeepMainRule(TestClass.class)
+ .enableInliningAnnotations()
+ .enableMemberValuePropagationAnnotations()
+ .enableMergeAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("Hello world!");
+ }
+
+ private void inspect(CodeInspector inspector) {
+ assertThat(inspector.clazz(Candidate.class), isPresent());
+ assertThat(inspector.clazz(CandidateBase.class), isPresent());
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ System.out.println(new Candidate().get());
+ }
+ }
+
+ @NeverMerge
+ static class CandidateBase {
+
+ CandidateBase() {
+ Escape.escape(getReceiver());
+ }
+
+ @NeverInline
+ Object getReceiver() {
+ return System.currentTimeMillis() >= 0 ? this : null;
+ }
+ }
+
+ static class Candidate extends CandidateBase {
+
+ @NeverInline
+ @NeverPropagateValue
+ String get() {
+ return "Hello world!";
+ }
+ }
+
+ static class Escape {
+
+ @NeverInline
+ static void escape(Object o) {
+ if (System.currentTimeMillis() < 0) {
+ System.out.println(o);
+ }
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/shaking/librarymethodoverride/LibraryMethodOverrideTest.java b/src/test/java/com/android/tools/r8/shaking/librarymethodoverride/LibraryMethodOverrideTest.java
index a5fbc59..18188ff 100644
--- a/src/test/java/com/android/tools/r8/shaking/librarymethodoverride/LibraryMethodOverrideTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/librarymethodoverride/LibraryMethodOverrideTest.java
@@ -66,16 +66,7 @@
for (Class<?> nonEscapingClass : nonEscapingClasses) {
ClassSubject classSubject = inspector.clazz(nonEscapingClass);
assertThat(classSubject, isPresent());
-
- // TODO(b/142772856): None of the non-escaping classes should have a toString() method. It is
- // a requirement that the instance initializers are considered trivial for this to work,
- // though, even when they have a side effect (as long as the receiver does not escape via the
- // side effecting instruction).
- if (nonEscapingClass == DoesNotEscapeWithSubThatDoesNotOverrideSub.class) {
- assertThat(classSubject.uniqueMethodWithName("toString"), not(isPresent()));
- } else {
- assertThat(classSubject.uniqueMethodWithName("toString"), isPresent());
- }
+ assertThat(classSubject.uniqueMethodWithName("toString"), not(isPresent()));
}
}
@@ -135,8 +126,6 @@
@NeverClassInline
static class DoesNotEscape {
- // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
- // side effect.
DoesNotEscape() {
// Side effect to ensure that the constructor is not removed from main().
System.out.print("");
@@ -151,8 +140,6 @@
@NeverClassInline
static class DoesNotEscapeWithSubThatDoesNotOverride {
- // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
- // side effect.
DoesNotEscapeWithSubThatDoesNotOverride() {
// Side effect to ensure that the constructor is not removed from main().
System.out.print("");
@@ -168,8 +155,6 @@
static class DoesNotEscapeWithSubThatDoesNotOverrideSub
extends DoesNotEscapeWithSubThatDoesNotOverride {
- // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
- // side effect.
DoesNotEscapeWithSubThatDoesNotOverrideSub() {
// Side effect to ensure that the constructor is not removed from main().
System.out.print("");
@@ -179,8 +164,6 @@
@NeverClassInline
static class DoesNotEscapeWithSubThatOverrides {
- // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
- // side effect.
DoesNotEscapeWithSubThatOverrides() {
// Side effect to ensure that the constructor is not removed from main().
System.out.print("");
@@ -195,8 +178,6 @@
@NeverClassInline
static class DoesNotEscapeWithSubThatOverridesSub extends DoesNotEscapeWithSubThatOverrides {
- // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
- // side effect.
DoesNotEscapeWithSubThatOverridesSub() {
// Side effect to ensure that the constructor is not removed from main().
System.out.print("");
@@ -215,8 +196,6 @@
@NeverClassInline
static class DoesNotEscapeWithSubThatOverridesAndEscapes {
- // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
- // side effect.
DoesNotEscapeWithSubThatOverridesAndEscapes() {
// Side effect to ensure that the constructor is not removed from main().
System.out.print("");
@@ -232,8 +211,6 @@
static class DoesNotEscapeWithSubThatOverridesAndEscapesSub
extends DoesNotEscapeWithSubThatOverridesAndEscapes {
- // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
- // side effect.
DoesNotEscapeWithSubThatOverridesAndEscapesSub() {
// Side effect to ensure that the constructor is not removed from main().
System.out.print("");
