Allow class merging in presence of annotations
Bug: 179019716
Change-Id: Iada4c17529ca417d456007750f9cd46daeaf5268
diff --git a/src/main/java/com/android/tools/r8/ResourceShrinker.java b/src/main/java/com/android/tools/r8/ResourceShrinker.java
index b01f9a3..77abde6 100644
--- a/src/main/java/com/android/tools/r8/ResourceShrinker.java
+++ b/src/main/java/com/android/tools/r8/ResourceShrinker.java
@@ -240,7 +240,7 @@
Stream<DexAnnotation> classAnnotations = classDef.annotations().stream();
Stream<DexAnnotation> fieldAnnotations =
Streams.stream(classDef.fields())
- .filter(DexEncodedField::hasAnnotation)
+ .filter(DexEncodedField::hasAnnotations)
.flatMap(f -> f.annotations().stream());
Stream<DexAnnotation> methodAnnotations =
Streams.stream(classDef.methods())
diff --git a/src/main/java/com/android/tools/r8/graph/DexDefinition.java b/src/main/java/com/android/tools/r8/graph/DexDefinition.java
index c3e6998..8eeda6b 100644
--- a/src/main/java/com/android/tools/r8/graph/DexDefinition.java
+++ b/src/main/java/com/android/tools/r8/graph/DexDefinition.java
@@ -21,6 +21,10 @@
this.annotations = annotations;
}
+ public boolean hasAnnotations() {
+ return !annotations().isEmpty();
+ }
+
public DexAnnotationSet annotations() {
return annotations;
}
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedField.java b/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
index 8c501db..1a2b411 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
@@ -246,10 +246,6 @@
return accessFlags.isVolatile();
}
- public boolean hasAnnotation() {
- return !annotations().isEmpty();
- }
-
public boolean hasExplicitStaticValue() {
assert accessFlags.isStatic();
return staticValue != null;
diff --git a/src/main/java/com/android/tools/r8/graph/DexProgramClass.java b/src/main/java/com/android/tools/r8/graph/DexProgramClass.java
index aa724b4..a4ccb8e 100644
--- a/src/main/java/com/android/tools/r8/graph/DexProgramClass.java
+++ b/src/main/java/com/android/tools/r8/graph/DexProgramClass.java
@@ -543,7 +543,7 @@
private boolean hasAnnotations(DexEncodedField[] fields) {
synchronized (fields) {
- return Arrays.stream(fields).anyMatch(DexEncodedField::hasAnnotation);
+ return Arrays.stream(fields).anyMatch(DexEncodedField::hasAnnotations);
}
}
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 0603b48..9be5702 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/ClassMerger.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/ClassMerger.java
@@ -12,6 +12,7 @@
import com.android.tools.r8.graph.CfCode;
import com.android.tools.r8.graph.DexAnnotationSet;
import com.android.tools.r8.graph.DexClass;
+import com.android.tools.r8.graph.DexDefinition;
import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexItemFactory;
@@ -239,6 +240,16 @@
}
}
+ private void mergeAnnotations() {
+ assert group.getClasses().stream().filter(DexDefinition::hasAnnotations).count() <= 1;
+ for (DexProgramClass clazz : group.getSources()) {
+ if (clazz.hasAnnotations()) {
+ group.getTarget().setAnnotations(clazz.annotations());
+ break;
+ }
+ }
+ }
+
private void mergeInterfaces() {
DexTypeList previousInterfaces = group.getTarget().getInterfaces();
Set<DexType> interfaces = Sets.newLinkedHashSet(previousInterfaces);
@@ -261,6 +272,7 @@
fixAccessFlags();
appendClassIdField();
+ mergeAnnotations();
mergeInterfaces();
mergeVirtualMethods();
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 c1d531d..b700953 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/HorizontalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/HorizontalClassMerger.java
@@ -13,13 +13,14 @@
import com.android.tools.r8.horizontalclassmerging.policies.DontMergeSynchronizedClasses;
import com.android.tools.r8.horizontalclassmerging.policies.LimitGroups;
import com.android.tools.r8.horizontalclassmerging.policies.MinimizeFieldCasts;
-import com.android.tools.r8.horizontalclassmerging.policies.NoAnnotations;
+import com.android.tools.r8.horizontalclassmerging.policies.NoAnnotationClasses;
+import com.android.tools.r8.horizontalclassmerging.policies.NoClassAnnotationCollisions;
import com.android.tools.r8.horizontalclassmerging.policies.NoClassInitializerWithObservableSideEffects;
-import com.android.tools.r8.horizontalclassmerging.policies.NoClassesOrMembersWithAnnotations;
import com.android.tools.r8.horizontalclassmerging.policies.NoDirectRuntimeTypeChecks;
import com.android.tools.r8.horizontalclassmerging.policies.NoEnums;
import com.android.tools.r8.horizontalclassmerging.policies.NoIndirectRuntimeTypeChecks;
import com.android.tools.r8.horizontalclassmerging.policies.NoInnerClasses;
+import com.android.tools.r8.horizontalclassmerging.policies.NoInstanceFieldAnnotations;
import com.android.tools.r8.horizontalclassmerging.policies.NoInterfaces;
import com.android.tools.r8.horizontalclassmerging.policies.NoKeepRules;
import com.android.tools.r8.horizontalclassmerging.policies.NoKotlinMetadata;
@@ -119,36 +120,44 @@
}
private List<Policy> getPolicies(RuntimeTypeCheckInfo runtimeTypeCheckInfo) {
- return ImmutableList.of(
- new NotMatchedByNoHorizontalClassMerging(appView),
- new SameInstanceFields(appView),
- new NoInterfaces(),
- new NoAnnotations(),
- new NoEnums(appView),
- new CheckAbstractClasses(appView),
- new SyntheticItemsPolicy(appView),
- new NoClassesOrMembersWithAnnotations(appView),
- new NoInnerClasses(),
- new NoClassInitializerWithObservableSideEffects(),
- new NoNativeMethods(),
- new NoKeepRules(appView),
- new NoKotlinMetadata(),
- new NoServiceLoaders(appView),
- new NotVerticallyMergedIntoSubtype(appView),
- new NoDirectRuntimeTypeChecks(runtimeTypeCheckInfo),
- new NoIndirectRuntimeTypeChecks(appView, runtimeTypeCheckInfo),
- new PreventMethodImplementation(appView),
- new DontInlinePolicy(appView),
- new PreventMergeIntoDifferentMainDexGroups(appView),
- new AllInstantiatedOrUninstantiated(appView),
- new SameParentClass(),
- new SameNestHost(appView),
- new PreserveMethodCharacteristics(appView),
- new SameFeatureSplit(appView),
- new RespectPackageBoundaries(appView),
- new DontMergeSynchronizedClasses(appView),
- new MinimizeFieldCasts(),
- new LimitGroups(appView));
+ List<SingleClassPolicy> singleClassPolicies =
+ ImmutableList.of(
+ new NotMatchedByNoHorizontalClassMerging(appView),
+ new NoAnnotationClasses(),
+ new NoEnums(appView),
+ new NoInnerClasses(),
+ new NoInstanceFieldAnnotations(),
+ new NoInterfaces(),
+ new NoClassInitializerWithObservableSideEffects(),
+ new NoNativeMethods(),
+ new NoKeepRules(appView),
+ new NoKotlinMetadata(),
+ new NoServiceLoaders(appView),
+ new NotVerticallyMergedIntoSubtype(appView),
+ new NoDirectRuntimeTypeChecks(runtimeTypeCheckInfo),
+ new DontInlinePolicy(appView));
+ List<MultiClassPolicy> multiClassPolicies =
+ ImmutableList.of(
+ new SameInstanceFields(appView),
+ new NoClassAnnotationCollisions(),
+ new CheckAbstractClasses(appView),
+ new SyntheticItemsPolicy(appView),
+ new NoIndirectRuntimeTypeChecks(appView, runtimeTypeCheckInfo),
+ new PreventMethodImplementation(appView),
+ new PreventMergeIntoDifferentMainDexGroups(appView),
+ new AllInstantiatedOrUninstantiated(appView),
+ new SameParentClass(),
+ new SameNestHost(appView),
+ new PreserveMethodCharacteristics(appView),
+ new SameFeatureSplit(appView),
+ new RespectPackageBoundaries(appView),
+ new DontMergeSynchronizedClasses(appView),
+ new MinimizeFieldCasts(),
+ new LimitGroups(appView));
+ return ImmutableList.<Policy>builder()
+ .addAll(singleClassPolicies)
+ .addAll(multiClassPolicies)
+ .build();
}
/**
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/MergeGroup.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/MergeGroup.java
index 9e4ebf5..4b9ad6a 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/MergeGroup.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/MergeGroup.java
@@ -30,6 +30,11 @@
this.classes = new LinkedList<>();
}
+ public MergeGroup(DexProgramClass clazz) {
+ this();
+ add(clazz);
+ }
+
public MergeGroup(Collection<DexProgramClass> classes) {
this();
addAll(classes);
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoAnnotations.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoAnnotationClasses.java
similarity index 88%
rename from src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoAnnotations.java
rename to src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoAnnotationClasses.java
index 9190247..1effb8b 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoAnnotations.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoAnnotationClasses.java
@@ -7,7 +7,7 @@
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.horizontalclassmerging.SingleClassPolicy;
-public class NoAnnotations extends SingleClassPolicy {
+public class NoAnnotationClasses extends SingleClassPolicy {
@Override
public boolean canMerge(DexProgramClass program) {
return !program.isAnnotation();
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoClassAnnotationCollisions.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoClassAnnotationCollisions.java
new file mode 100644
index 0000000..9d3d730
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoClassAnnotationCollisions.java
@@ -0,0 +1,44 @@
+// 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.horizontalclassmerging.policies;
+
+import static com.android.tools.r8.utils.IteratorUtils.createCircularIterator;
+
+import com.android.tools.r8.graph.DexProgramClass;
+import com.android.tools.r8.horizontalclassmerging.MergeGroup;
+import com.android.tools.r8.horizontalclassmerging.MultiClassPolicy;
+import com.google.common.collect.ImmutableList;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
+
+public class NoClassAnnotationCollisions extends MultiClassPolicy {
+
+ @Override
+ public Collection<MergeGroup> apply(MergeGroup group) {
+ // Create a new merge group for each class that has annotations.
+ List<MergeGroup> newGroups = new LinkedList<>();
+ for (DexProgramClass clazz : group) {
+ if (clazz.hasAnnotations()) {
+ newGroups.add(new MergeGroup(clazz));
+ }
+ }
+
+ // If there were at most one class with annotations, then just return the original merge group.
+ if (newGroups.size() <= 1) {
+ return ImmutableList.of(group);
+ }
+
+ // Otherwise, fill up the new merge groups with the classes that do not have annotations.
+ Iterator<MergeGroup> newGroupsIterator = createCircularIterator(newGroups);
+ for (DexProgramClass clazz : group) {
+ if (!clazz.hasAnnotations()) {
+ newGroupsIterator.next().add(clazz);
+ }
+ }
+ return removeTrivialGroups(newGroups);
+ }
+}
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoClassesOrMembersWithAnnotations.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoClassesOrMembersWithAnnotations.java
deleted file mode 100644
index f0b2a6e..0000000
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoClassesOrMembersWithAnnotations.java
+++ /dev/null
@@ -1,31 +0,0 @@
-// 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.horizontalclassmerging.policies;
-
-import com.android.tools.r8.graph.AppView;
-import com.android.tools.r8.graph.DexProgramClass;
-import com.android.tools.r8.horizontalclassmerging.SingleClassPolicy;
-import com.android.tools.r8.shaking.AppInfoWithLiveness;
-import com.android.tools.r8.utils.InternalOptions.HorizontalClassMergerOptions;
-
-public class NoClassesOrMembersWithAnnotations extends SingleClassPolicy {
-
- private final HorizontalClassMergerOptions options;
-
- public NoClassesOrMembersWithAnnotations(AppView<AppInfoWithLiveness> appView) {
- this.options = appView.options().horizontalClassMergerOptions();
- }
-
- @Override
- public boolean canMerge(DexProgramClass program) {
- return !program.hasClassOrMemberAnnotations();
- }
-
- @Override
- public boolean shouldSkipPolicy() {
- // TODO(b/179019716): Add support for merging in presence of annotations.
- return options.skipNoClassesOrMembersWithAnnotationsPolicyForTesting;
- }
-}
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoAnnotations.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoInstanceFieldAnnotations.java
similarity index 62%
copy from src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoAnnotations.java
copy to src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoInstanceFieldAnnotations.java
index 9190247..610af7e 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoAnnotations.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/NoInstanceFieldAnnotations.java
@@ -4,12 +4,19 @@
package com.android.tools.r8.horizontalclassmerging.policies;
+import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.horizontalclassmerging.SingleClassPolicy;
-public class NoAnnotations extends SingleClassPolicy {
+public class NoInstanceFieldAnnotations extends SingleClassPolicy {
+
@Override
public boolean canMerge(DexProgramClass program) {
- return !program.isAnnotation();
+ for (DexEncodedField instanceField : program.instanceFields()) {
+ if (instanceField.hasAnnotations()) {
+ return false;
+ }
+ }
+ return true;
}
}
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 c9ee155..cf7e877 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -1163,9 +1163,6 @@
public int syntheticArgumentCount = 3;
public int maxGroupSize = 30;
- // TODO(b/179019716): Add support for merging in presence of annotations.
- public boolean skipNoClassesOrMembersWithAnnotationsPolicyForTesting = false;
-
public void disable() {
enable = false;
}
diff --git a/src/main/java/com/android/tools/r8/utils/IteratorUtils.java b/src/main/java/com/android/tools/r8/utils/IteratorUtils.java
index 93b7e4a..3ecdc4f 100644
--- a/src/main/java/com/android/tools/r8/utils/IteratorUtils.java
+++ b/src/main/java/com/android/tools/r8/utils/IteratorUtils.java
@@ -9,6 +9,7 @@
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.InstructionIterator;
import com.android.tools.r8.ir.code.InstructionListIterator;
+import com.google.common.collect.Iterables;
import java.util.Iterator;
import java.util.ListIterator;
import java.util.NoSuchElementException;
@@ -16,6 +17,27 @@
public class IteratorUtils {
+ public static <T> Iterator<T> createCircularIterator(Iterable<T> iterable) {
+ assert !Iterables.isEmpty(iterable);
+ return new Iterator<T>() {
+
+ private Iterator<T> iterator = iterable.iterator();
+
+ @Override
+ public boolean hasNext() {
+ return true;
+ }
+
+ @Override
+ public T next() {
+ if (!iterator.hasNext()) {
+ iterator = iterable.iterator();
+ }
+ return iterator.next();
+ }
+ };
+ }
+
public static <T> int countRemaining(Iterator<T> iterator) {
IntBox counter = new IntBox();
iterator.forEachRemaining(ignore -> counter.increment());
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/NoClassesOrMembersWithAnnotationsTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/NoClassesOrMembersWithAnnotationsTest.java
index 74d0ad6..441934e 100644
--- a/src/test/java/com/android/tools/r8/classmerging/horizontal/NoClassesOrMembersWithAnnotationsTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/NoClassesOrMembersWithAnnotationsTest.java
@@ -5,6 +5,7 @@
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 com.android.tools.r8.NeverClassInline;
@@ -33,6 +34,9 @@
.addOptionsModification(
options ->
options.horizontalClassMergerOptions().enableIf(enableHorizontalClassMerging))
+ .addHorizontallyMergedClassesInspectorIf(
+ enableHorizontalClassMerging,
+ inspector -> inspector.assertIsCompleteMergeGroup(A.class, C.class))
.enableNeverClassInliningAnnotations()
.enableInliningAnnotations()
.setMinApi(parameters.getApiLevel())
@@ -45,7 +49,8 @@
assertThat(codeInspector.clazz(MethodAnnotation.class), isPresent());
assertThat(codeInspector.clazz(A.class), isPresent());
assertThat(codeInspector.clazz(B.class), isPresent());
- assertThat(codeInspector.clazz(C.class), isPresent());
+ assertThat(
+ codeInspector.clazz(C.class), notIf(isPresent(), enableHorizontalClassMerging));
});
}
@@ -57,6 +62,7 @@
@Target({ElementType.METHOD})
public @interface MethodAnnotation {}
+ @TypeAnnotation
@NeverClassInline
public static class A {
public A() {
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/UnusedAnnotatedArgumentsWithMissingAnnotationsTest.java b/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/UnusedAnnotatedArgumentsWithMissingAnnotationsTest.java
index ef269dc..49b90e7 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/UnusedAnnotatedArgumentsWithMissingAnnotationsTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/unusedarguments/UnusedAnnotatedArgumentsWithMissingAnnotationsTest.java
@@ -32,7 +32,7 @@
@Parameterized.Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimes().build();
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
}
public UnusedAnnotatedArgumentsWithMissingAnnotationsTest(TestParameters parameters) {
@@ -72,17 +72,20 @@
dumpAnnotation2(),
dumpAnnotation3())
.addKeepMainRule("Test")
- .addKeepRules("-keep @interface Annotation?")
- .addKeepAttributes("RuntimeVisibleParameterAnnotations")
+ .addKeepRules(
+ "-keep @interface Annotation?",
+ "-neverclassinline class *",
+ "-nohorizontalclassmerging class Test$Inner?")
+ .addKeepRuntimeVisibleParameterAnnotations()
.enableProguardTestOptions()
- .addKeepRules("-neverclassinline class *")
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.compile()
- .inspect(inspector -> {
- checkClass(inspector.clazz("Test$Inner1"), "Annotation1");
- checkClass(inspector.clazz("Test$Inner2"), "Annotation2");
- checkClass(inspector.clazz("Test$Inner2"), "Annotation2");
- })
+ .inspect(
+ inspector -> {
+ checkClass(inspector.clazz("Test$Inner1"), "Annotation1");
+ checkClass(inspector.clazz("Test$Inner2"), "Annotation2");
+ checkClass(inspector.clazz("Test$Inner2"), "Annotation2");
+ })
.run(parameters.getRuntime(), "Test")
.assertSuccessWithOutput(expectedOutput);
}
diff --git a/src/test/java/com/android/tools/r8/naming/b139991218/TestRunner.java b/src/test/java/com/android/tools/r8/naming/b139991218/TestRunner.java
index d9b9a3c..218e037 100644
--- a/src/test/java/com/android/tools/r8/naming/b139991218/TestRunner.java
+++ b/src/test/java/com/android/tools/r8/naming/b139991218/TestRunner.java
@@ -69,11 +69,6 @@
options -> {
options.testing.validInliningReasons = ImmutableSet.of(Reason.FORCE);
options.enableClassInlining = false;
-
- // TODO(b/179019716): Add support for merging in presence of annotations.
- options.horizontalClassMergerOptions()
- .skipNoClassesOrMembersWithAnnotationsPolicyForTesting =
- true;
})
.addHorizontallyMergedClassesInspector(
inspector ->
