Limit the size of merged constructors
Bug: 163311975
Change-Id: I445aa357c8ef6710cff0075c3e39ae2ca6e7c3f5
diff --git a/src/main/java/com/android/tools/r8/graph/CfCode.java b/src/main/java/com/android/tools/r8/graph/CfCode.java
index e08f5da..dbefe85 100644
--- a/src/main/java/com/android/tools/r8/graph/CfCode.java
+++ b/src/main/java/com/android/tools/r8/graph/CfCode.java
@@ -20,6 +20,7 @@
import com.android.tools.r8.cf.code.CfPosition;
import com.android.tools.r8.cf.code.CfReturnVoid;
import com.android.tools.r8.cf.code.CfTryCatch;
+import com.android.tools.r8.code.Base5Format;
import com.android.tools.r8.errors.InvalidDebugInfoException;
import com.android.tools.r8.errors.Unimplemented;
import com.android.tools.r8.errors.Unreachable;
@@ -198,6 +199,11 @@
return countNonStackOperations(threshold) <= threshold;
}
+ @Override
+ public int estimatedDexCodeSizeUpperBoundInBytes() {
+ return estimatedSizeForInlining() * Base5Format.SIZE;
+ }
+
private int countNonStackOperations(int threshold) {
int result = 0;
for (CfInstruction instruction : instructions) {
diff --git a/src/main/java/com/android/tools/r8/graph/Code.java b/src/main/java/com/android/tools/r8/graph/Code.java
index 1f2a522..535c08f 100644
--- a/src/main/java/com/android/tools/r8/graph/Code.java
+++ b/src/main/java/com/android/tools/r8/graph/Code.java
@@ -71,6 +71,8 @@
return estimatedSizeForInlining() <= threshold;
}
+ public abstract int estimatedDexCodeSizeUpperBoundInBytes();
+
public CfCode asCfCode() {
throw new Unreachable(getClass().getCanonicalName() + ".asCfCode()");
}
diff --git a/src/main/java/com/android/tools/r8/graph/DexCode.java b/src/main/java/com/android/tools/r8/graph/DexCode.java
index eaf8d7f..7a9c724 100644
--- a/src/main/java/com/android/tools/r8/graph/DexCode.java
+++ b/src/main/java/com/android/tools/r8/graph/DexCode.java
@@ -101,6 +101,11 @@
}
@Override
+ public int estimatedDexCodeSizeUpperBoundInBytes() {
+ return codeSizeInBytes();
+ }
+
+ @Override
public DexCode asDexCode() {
return this;
}
diff --git a/src/main/java/com/android/tools/r8/graph/LazyCfCode.java b/src/main/java/com/android/tools/r8/graph/LazyCfCode.java
index e0aace9..cdafe4e 100644
--- a/src/main/java/com/android/tools/r8/graph/LazyCfCode.java
+++ b/src/main/java/com/android/tools/r8/graph/LazyCfCode.java
@@ -211,6 +211,11 @@
}
@Override
+ public int estimatedDexCodeSizeUpperBoundInBytes() {
+ return asCfCode().estimatedDexCodeSizeUpperBoundInBytes();
+ }
+
+ @Override
public IRCode buildIR(ProgramMethod method, AppView<?> appView, Origin origin) {
return asCfCode().buildIR(method, appView, origin);
}
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/ClassMerger.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/ClassMerger.java
index a6003c4..f99999a 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/ClassMerger.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/ClassMerger.java
@@ -174,6 +174,7 @@
}
public static class Builder {
+ private final AppView<AppInfoWithLiveness> appView;
private final DexProgramClass target;
private final List<DexProgramClass> toMergeGroup = new ArrayList<>();
private final Map<DexProto, ConstructorMerger.Builder> constructorMergerBuilders =
@@ -181,7 +182,8 @@
private final Map<Wrapper<DexMethod>, VirtualMethodMerger.Builder> virtualMethodMergerBuilders =
new LinkedHashMap<>();
- public Builder(DexProgramClass target) {
+ public Builder(AppView<AppInfoWithLiveness> appView, DexProgramClass target) {
+ this.appView = appView;
this.target = target;
setupForMethodMerging(target);
}
@@ -215,7 +217,7 @@
assert method.getDefinition().isInstanceInitializer();
constructorMergerBuilders
.computeIfAbsent(
- method.getDefinition().getProto(), ignore -> new ConstructorMerger.Builder())
+ method.getDefinition().getProto(), ignore -> new ConstructorMerger.Builder(appView))
.add(method.getDefinition());
}
@@ -250,7 +252,7 @@
List<ConstructorMerger> constructorMergers =
new ArrayList<>(constructorMergerBuilders.size());
for (ConstructorMerger.Builder builder : constructorMergerBuilders.values()) {
- constructorMergers.add(builder.build(appView, target, classIdField));
+ constructorMergers.addAll(builder.build(appView, target, classIdField));
}
// Try and merge the functions with the most arguments first, to avoid using synthetic
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/ConstructorMerger.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/ConstructorMerger.java
index 4740da3..f6127d9 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/ConstructorMerger.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/ConstructorMerger.java
@@ -20,7 +20,9 @@
import com.android.tools.r8.ir.conversion.ExtraConstantIntParameter;
import com.android.tools.r8.ir.conversion.ExtraParameter;
import com.android.tools.r8.ir.conversion.ExtraUnusedNullParameter;
+import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.shaking.FieldAccessInfoCollectionModifier;
+import com.android.tools.r8.utils.ListUtils;
import it.unimi.dsi.fastutil.ints.Int2ReferenceAVLTreeMap;
import it.unimi.dsi.fastutil.ints.Int2ReferenceSortedMap;
import it.unimi.dsi.fastutil.objects.Reference2IntMap;
@@ -71,20 +73,41 @@
}
public static class Builder {
- private final Collection<DexEncodedMethod> constructors;
+ private int estimatedDexCodeSize;
+ private final List<List<DexEncodedMethod>> constructorGroups = new ArrayList<>();
+ private AppView<AppInfoWithLiveness> appView;
- public Builder() {
- constructors = new ArrayList<>();
+ public Builder(AppView<AppInfoWithLiveness> appView) {
+ this.appView = appView;
+
+ createNewGroup();
+ }
+
+ private void createNewGroup() {
+ estimatedDexCodeSize = 0;
+ constructorGroups.add(new ArrayList<>());
}
public Builder add(DexEncodedMethod constructor) {
- constructors.add(constructor);
+ int estimatedMaxSizeInBytes = constructor.getCode().estimatedDexCodeSizeUpperBoundInBytes();
+ // If the constructor gets too large, then the constructor should be merged into a new group.
+ if (estimatedDexCodeSize + estimatedMaxSizeInBytes
+ > appView.options().minimumVerificationSizeLimitInBytes() / 2
+ && estimatedDexCodeSize > 0) {
+ createNewGroup();
+ }
+
+ ListUtils.last(constructorGroups).add(constructor);
+ estimatedDexCodeSize += estimatedMaxSizeInBytes;
return this;
}
- public ConstructorMerger build(
+ public List<ConstructorMerger> build(
AppView<?> appView, DexProgramClass target, DexField classIdField) {
- return new ConstructorMerger(appView, target, constructors, classIdField);
+ assert constructorGroups.stream().noneMatch(List::isEmpty);
+ return ListUtils.map(
+ constructorGroups,
+ constructors -> new ConstructorMerger(appView, target, constructors, classIdField));
}
}
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/HorizontalClassMerger.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/HorizontalClassMerger.java
index c8a1c16..5dc81b1 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/HorizontalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/HorizontalClassMerger.java
@@ -155,7 +155,7 @@
group.remove(target);
ClassMerger merger =
- new ClassMerger.Builder(target)
+ new ClassMerger.Builder(appView, target)
.addClassesToMerge(group)
.build(appView, mergedClassesBuilder, lensBuilder, fieldAccessChangesBuilder);
classMergers.add(merger);
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java b/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java
index 9e16ab0..334238d 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/Outliner.java
@@ -1605,6 +1605,11 @@
}
@Override
+ public int estimatedDexCodeSizeUpperBoundInBytes() {
+ return Integer.MAX_VALUE;
+ }
+
+ @Override
public OutlineCode asOutlineCode() {
return this;
}
diff --git a/src/main/java/com/android/tools/r8/ir/synthetic/AbstractSynthesizedCode.java b/src/main/java/com/android/tools/r8/ir/synthetic/AbstractSynthesizedCode.java
index da4daec..ad43a6c 100644
--- a/src/main/java/com/android/tools/r8/ir/synthetic/AbstractSynthesizedCode.java
+++ b/src/main/java/com/android/tools/r8/ir/synthetic/AbstractSynthesizedCode.java
@@ -95,4 +95,9 @@
public final String toString(DexEncodedMethod method, ClassNameMapper naming) {
return this.getClass().getSimpleName();
}
+
+ @Override
+ public int estimatedDexCodeSizeUpperBoundInBytes() {
+ return Integer.MAX_VALUE;
+ }
}
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 ea902d5..3f5ceb6 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -275,6 +275,13 @@
return isGeneratingClassFiles() ? 65534 : 16383;
}
+ public int minimumVerificationSizeLimitInBytes() {
+ if (testing.verificationSizeLimitInBytesOverride > -1) {
+ return testing.verificationSizeLimitInBytesOverride;
+ }
+ return 16383;
+ }
+
// TODO(b/141719453): The inlining limit at least should be consistent with normal inlining.
public int classInliningInstructionLimit = 10;
public int classInliningInstructionAllowance = 50;
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/LargeConstructorsMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/LargeConstructorsMergingTest.java
new file mode 100644
index 0000000..847db55
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/LargeConstructorsMergingTest.java
@@ -0,0 +1,83 @@
+// Copyright (c) 2020, 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.classmerging.horizontal;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.notIf;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+
+import com.android.tools.r8.NeverClassInline;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import org.junit.Test;
+
+public class LargeConstructorsMergingTest extends HorizontalClassMergingTestBase {
+ public LargeConstructorsMergingTest(
+ TestParameters parameters, boolean enableHorizontalClassMerging) {
+ super(parameters, enableHorizontalClassMerging);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(getClass())
+ .addKeepMainRule(Main.class)
+ .addOptionsModification(
+ options -> options.enableHorizontalClassMerging = enableHorizontalClassMerging)
+ .addOptionsModification(options -> options.testing.verificationSizeLimitInBytesOverride = 4)
+ .enableNeverClassInliningAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .addHorizontallyMergedClassesInspectorIf(
+ enableHorizontalClassMerging,
+ inspector ->
+ inspector.assertMergedInto(B.class, A.class).assertMergedInto(C.class, A.class))
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("a: 1", "b: 2", "c: 3")
+ .inspect(
+ codeInspector -> {
+ ClassSubject aClassSubject = codeInspector.clazz(A.class);
+ assertThat(aClassSubject, isPresent());
+ assertThat(
+ codeInspector.clazz(B.class), notIf(isPresent(), enableHorizontalClassMerging));
+ assertThat(
+ codeInspector.clazz(C.class), notIf(isPresent(), enableHorizontalClassMerging));
+
+ if (enableHorizontalClassMerging) {
+ // There should be three constructors on class A after merging.
+ assertEquals(3, aClassSubject.allMethods().size());
+ }
+ });
+ }
+
+ @NeverClassInline
+ public static class A {
+ public A(int v) {
+ System.out.println("a: " + v);
+ }
+ }
+
+ @NeverClassInline
+ public static class B {
+ public B(int v) {
+ System.out.println("b: " + v);
+ }
+ }
+
+ @NeverClassInline
+ public static class C {
+ public C(int v) {
+ System.out.println("c: " + v);
+ }
+ }
+
+ public static class Main {
+ public static void main(String[] args) {
+ new A(1);
+ new B(2);
+ new C(3);
+ }
+ }
+}
