Remove redundant work in rule evaluation
This reduces build speed for the Chrome benchmark by 8% when running locally.
Bug: b/422947619
Change-Id: I1e8921b296ae0f35ba56bb3dc3f677a84d276d19
diff --git a/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java b/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
index 0b201a2..7f2238d 100644
--- a/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
+++ b/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
@@ -228,7 +228,9 @@
boolean classNameResult = memberRule.getClassNames().matches(source.type);
assert classNameResult;
if (memberRule.hasInheritanceClassName()) {
- boolean inheritanceResult = rootSetBuilder.satisfyInheritanceRule(target, memberRule);
+ boolean inheritanceResult =
+ memberRule.satisfyInheritanceRule(
+ target, appView, rootSetBuilder::handleMatchedAnnotation);
assert inheritanceResult;
}
}
@@ -278,7 +280,7 @@
}
if (rule.hasInheritanceClassName()) {
// Try another live type since the current one doesn't satisfy the inheritance rule.
- return rootSetBuilder.satisfyInheritanceRule(clazz, rule);
+ return rule.satisfyInheritanceRule(clazz, appView, rootSetBuilder::handleMatchedAnnotation);
}
return true;
}
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardClassNameList.java b/src/main/java/com/android/tools/r8/shaking/ProguardClassNameList.java
index 9555058..fd68364 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardClassNameList.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardClassNameList.java
@@ -38,6 +38,8 @@
return new SingleClassNameList(matcher);
}
+ public abstract boolean isMatchAnyClassPattern();
+
public abstract int size();
public static class Builder {
@@ -154,6 +156,11 @@
}
@Override
+ public boolean isMatchAnyClassPattern() {
+ return false;
+ }
+
+ @Override
public int size() {
return 0;
}
@@ -207,6 +214,11 @@
}
@Override
+ public boolean isMatchAnyClassPattern() {
+ return className.isMatchAnyClassPattern();
+ }
+
+ @Override
public int size() {
return 1;
}
@@ -300,6 +312,11 @@
}
@Override
+ public boolean isMatchAnyClassPattern() {
+ return false;
+ }
+
+ @Override
public int size() {
return classNames.size();
}
@@ -412,6 +429,11 @@
}
@Override
+ public boolean isMatchAnyClassPattern() {
+ return false;
+ }
+
+ @Override
public int size() {
return classNames.size();
}
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardClassSpecification.java b/src/main/java/com/android/tools/r8/shaking/ProguardClassSpecification.java
index 98f41cd..00e4830 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardClassSpecification.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardClassSpecification.java
@@ -9,6 +9,7 @@
import com.android.tools.r8.position.TextRange;
import com.android.tools.r8.utils.StringUtils;
import com.google.common.collect.ImmutableList;
+import java.util.Collection;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
@@ -264,6 +265,19 @@
return memberRules;
}
+ public void getFieldAndMethodRules(
+ Collection<ProguardMemberRule> fieldRules, Collection<ProguardMemberRule> methodRules) {
+ for (ProguardMemberRule memberRule : memberRules) {
+ ProguardMemberType ruleType = memberRule.getRuleType();
+ if (ruleType.includesFields()) {
+ fieldRules.add(memberRule);
+ }
+ if (ruleType.includesMethods()) {
+ methodRules.add(memberRule);
+ }
+ }
+ }
+
public boolean getInheritanceIsExtends() {
return inheritanceIsExtends;
}
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationRule.java b/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationRule.java
index d8e0105..84518dd 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationRule.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardConfigurationRule.java
@@ -4,6 +4,10 @@
package com.android.tools.r8.shaking;
import static com.android.tools.r8.graph.DexProgramClass.asProgramClassOrNull;
+import static com.android.tools.r8.shaking.RootSetUtils.RootSetBuilder.satisfyAccessFlag;
+import static com.android.tools.r8.shaking.RootSetUtils.RootSetBuilder.satisfyAnnotation;
+import static com.android.tools.r8.shaking.RootSetUtils.RootSetBuilder.satisfyClassType;
+import static com.android.tools.r8.shaking.RootSetUtils.RootSetBuilder.satisfyNonSyntheticClass;
import static com.google.common.base.Predicates.alwaysTrue;
import com.android.tools.r8.graph.AppInfoWithClassHierarchy;
@@ -15,8 +19,9 @@
import com.android.tools.r8.origin.Origin;
import com.android.tools.r8.position.Position;
import com.android.tools.r8.shaking.ProguardWildcard.BackReference;
-import com.android.tools.r8.utils.BooleanBox;
+import com.android.tools.r8.shaking.RootSetUtils.RootSetBuilder;
import com.android.tools.r8.utils.IterableUtils;
+import com.android.tools.r8.utils.OptionalBool;
import com.android.tools.r8.utils.StringUtils;
import com.google.common.collect.Iterables;
import java.util.List;
@@ -30,6 +35,7 @@
// TODO(b/164019179): Since we are using the rule language for tracing main dex we can end up in
// a situation where the references to types are dead.
private boolean canReferenceDeadTypes = false;
+ private OptionalBool trivialAllClassMatch = OptionalBool.unknown();
ProguardConfigurationRule(
Origin origin,
@@ -61,21 +67,27 @@
memberRules);
}
- public boolean isTrivalAllClassMatch() {
- BooleanBox booleanBox = new BooleanBox(true);
- getClassNames()
- .forEachTypeMatcher(
- unused -> booleanBox.set(false),
- proguardTypeMatcher -> !proguardTypeMatcher.isMatchAnyClassPattern());
- return booleanBox.get()
+ public boolean isTrivialAllClassMatch() {
+ if (trivialAllClassMatch.isUnknown()) {
+ trivialAllClassMatch = OptionalBool.of(computeIsTrivialAllClassMatch());
+ }
+ assert trivialAllClassMatch.isTrue() == computeIsTrivialAllClassMatch();
+ return trivialAllClassMatch.isTrue();
+ }
+
+ public boolean isTrivialAllClassMatchWithNoMembersRules() {
+ return isTrivialAllClassMatch() && !hasMemberRules();
+ }
+
+ public boolean computeIsTrivialAllClassMatch() {
+ return getClassNames().isMatchAnyClassPattern()
&& getClassAnnotations().isEmpty()
&& getClassAccessFlags().isDefaultFlags()
&& getNegatedClassAccessFlags().isDefaultFlags()
&& !getClassTypeNegated()
&& getClassType() == ProguardClassType.CLASS
&& getInheritanceAnnotations().isEmpty()
- && getInheritanceClassName() == null
- && getMemberRules().isEmpty();
+ && !hasInheritanceClassName();
}
public boolean isUsed() {
@@ -267,4 +279,141 @@
super.append(builder);
return builder;
}
+
+ public boolean testClassCondition(
+ DexClass clazz,
+ AppView<?> appView,
+ Consumer<AnnotationMatchResult> annotationMatchResultConsumer) {
+ if (!satisfyNonSyntheticClass(clazz, appView)) {
+ return false;
+ }
+ if (!satisfyClassType(this, clazz)) {
+ return false;
+ }
+ if (!satisfyAccessFlag(this, clazz)) {
+ return false;
+ }
+ AnnotationMatchResult annotationMatchResult = satisfyAnnotation(this, clazz);
+ if (annotationMatchResult == null) {
+ return false;
+ }
+ annotationMatchResultConsumer.accept(annotationMatchResult);
+ // In principle it should make a difference whether the user specified in a class
+ // spec that a class either extends or implements another type. However, proguard
+ // seems not to care, so users have started to use this inconsistently. We are thus
+ // inconsistent, as well, but tell them.
+ // TODO(herhut): One day make this do what it says.
+ if (hasInheritanceClassName()
+ && !satisfyInheritanceRule(clazz, appView, annotationMatchResultConsumer)) {
+ return false;
+ }
+ return getClassNames().matches(clazz.type);
+ }
+
+ public boolean satisfyInheritanceRule(
+ DexClass clazz,
+ AppView<?> appView,
+ Consumer<AnnotationMatchResult> annotationMatchResultConsumer) {
+ return satisfyExtendsRule(clazz, appView, annotationMatchResultConsumer)
+ || satisfyImplementsRule(clazz, appView, annotationMatchResultConsumer);
+ }
+
+ private boolean satisfyExtendsRule(
+ DexClass clazz,
+ AppView<?> appView,
+ Consumer<AnnotationMatchResult> annotationMatchResultConsumer) {
+ if (anySuperTypeMatchesExtendsRule(clazz.superType, appView, annotationMatchResultConsumer)) {
+ return true;
+ }
+ // It is possible that this class used to inherit from another class X, but no longer does it,
+ // because X has been merged into `clazz`.
+ return anySourceMatchesInheritanceRuleDirectly(clazz, false, appView);
+ }
+
+ private boolean anySuperTypeMatchesExtendsRule(
+ DexType type,
+ AppView<?> appView,
+ Consumer<AnnotationMatchResult> annotationMatchResultConsumer) {
+ while (type != null) {
+ DexClass clazz = appView.definitionFor(type);
+ if (clazz == null) {
+ // TODO(herhut): Warn about broken supertype chain?
+ return false;
+ }
+ // TODO(b/110141157): Should the vertical class merger move annotations from the source to
+ // the target class? If so, it is sufficient only to apply the annotation-matcher to the
+ // annotations of `class`.
+ if (getInheritanceClassName().matches(clazz.type, appView)) {
+ AnnotationMatchResult annotationMatchResult =
+ RootSetBuilder.containsAllAnnotations(getInheritanceAnnotations(), clazz);
+ if (annotationMatchResult != null) {
+ annotationMatchResultConsumer.accept(annotationMatchResult);
+ return true;
+ }
+ }
+ type = clazz.superType;
+ }
+ return false;
+ }
+
+ private boolean satisfyImplementsRule(
+ DexClass clazz,
+ AppView<?> appView,
+ Consumer<AnnotationMatchResult> annotationMatchResultConsumer) {
+ if (anyImplementedInterfaceMatchesImplementsRule(
+ clazz, appView, annotationMatchResultConsumer)) {
+ return true;
+ }
+ // It is possible that this class used to implement an interface I, but no longer does it,
+ // because I has been merged into `clazz`.
+ return anySourceMatchesInheritanceRuleDirectly(clazz, true, appView);
+ }
+
+ private boolean anyImplementedInterfaceMatchesImplementsRule(
+ DexClass clazz,
+ AppView<?> appView,
+ Consumer<AnnotationMatchResult> annotationMatchResultConsumer) {
+ // TODO(herhut): Maybe it would be better to do this breadth first.
+ for (DexType iface : clazz.getInterfaces()) {
+ DexClass ifaceClass = appView.definitionFor(iface);
+ if (ifaceClass == null) {
+ // TODO(herhut): Warn about broken supertype chain?
+ continue;
+ }
+ // TODO(b/110141157): Should the vertical class merger move annotations from the source to
+ // the target class? If so, it is sufficient only to apply the annotation-matcher to the
+ // annotations of `ifaceClass`.
+ if (getInheritanceClassName().matches(iface, appView)) {
+ AnnotationMatchResult annotationMatchResult =
+ RootSetBuilder.containsAllAnnotations(getInheritanceAnnotations(), ifaceClass);
+ if (annotationMatchResult != null) {
+ annotationMatchResultConsumer.accept(annotationMatchResult);
+ return true;
+ }
+ }
+ if (anyImplementedInterfaceMatchesImplementsRule(
+ ifaceClass, appView, annotationMatchResultConsumer)) {
+ return true;
+ }
+ }
+ if (!clazz.hasSuperType()) {
+ return false;
+ }
+ DexClass superClass = appView.definitionFor(clazz.getSuperType());
+ return superClass != null
+ && anyImplementedInterfaceMatchesImplementsRule(
+ superClass, appView, annotationMatchResultConsumer);
+ }
+
+ private boolean anySourceMatchesInheritanceRuleDirectly(
+ DexClass clazz, boolean isInterface, AppView<?> appView) {
+ // TODO(b/110141157): Figure out what to do with annotations. Should the annotations of
+ // the DexClass corresponding to `sourceType` satisfy the `annotation`-matcher?
+ return appView.getVerticallyMergedClasses() != null
+ && appView.getVerticallyMergedClasses().getSourcesFor(clazz.type).stream()
+ .filter(
+ sourceType ->
+ appView.definitionFor(sourceType).accessFlags.isInterface() == isInterface)
+ .anyMatch(getInheritanceClassName()::matches);
+ }
}
diff --git a/src/main/java/com/android/tools/r8/shaking/ProguardIfRulePreconditionMatch.java b/src/main/java/com/android/tools/r8/shaking/ProguardIfRulePreconditionMatch.java
index 81919d3..000cfc3 100644
--- a/src/main/java/com/android/tools/r8/shaking/ProguardIfRulePreconditionMatch.java
+++ b/src/main/java/com/android/tools/r8/shaking/ProguardIfRulePreconditionMatch.java
@@ -33,7 +33,7 @@
public void disallowOptimizationsForReevaluation(
Enqueuer enqueuer, ConsequentRootSetBuilder rootSetBuilder) {
if (enqueuer.getMode().isInitialTreeShaking()
- && !ifRule.isTrivalAllClassMatch()
+ && !ifRule.isTrivialAllClassMatchWithNoMembersRules()
&& classMatch.isProgramClass()) {
disallowClassOptimizationsForReevaluation(rootSetBuilder);
disallowMethodOptimizationsForReevaluation(rootSetBuilder);
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java b/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
index 908ab5f..2541fcb 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
@@ -342,9 +342,10 @@
private void process(
Collection<DexClass> classes,
ProguardConfigurationRule rule,
- ProguardIfRulePreconditionMatch ifRulePreconditionMatch) {
+ ProguardIfRulePreconditionMatch ifRulePreconditionMatch,
+ Timing timing) {
for (DexClass clazz : classes) {
- process(clazz, rule, ifRulePreconditionMatch);
+ process(clazz, rule, ifRulePreconditionMatch, timing);
}
}
@@ -352,35 +353,15 @@
private void process(
DexClass clazz,
ProguardConfigurationRule rule,
- ProguardIfRulePreconditionMatch ifRulePreconditionMatch) {
- if (!satisfyNonSyntheticClass(clazz, appView)) {
+ ProguardIfRulePreconditionMatch ifRulePreconditionMatch,
+ Timing timing) {
+ if (!rule.testClassCondition(clazz, appView, this::handleMatchedAnnotation)) {
return;
}
- if (!satisfyClassType(rule, clazz)) {
- return;
- }
- if (!satisfyAccessFlag(rule, clazz)) {
- return;
- }
- AnnotationMatchResult annotationMatchResult = satisfyAnnotation(rule, clazz);
- if (annotationMatchResult == null) {
- return;
- }
- handleMatchedAnnotation(annotationMatchResult);
- // In principle it should make a difference whether the user specified in a class
- // spec that a class either extends or implements another type. However, proguard
- // seems not to care, so users have started to use this inconsistently. We are thus
- // inconsistent, as well, but tell them.
- // TODO(herhut): One day make this do what it says.
- if (rule.hasInheritanceClassName() && !satisfyInheritanceRule(clazz, rule)) {
- return;
- }
-
- if (!rule.getClassNames().matches(clazz.type)) {
- return;
- }
-
Collection<ProguardMemberRule> memberKeepRules = rule.getMemberRules();
+ Collection<ProguardMemberRule> fieldKeepRules = new ArrayList<>();
+ Collection<ProguardMemberRule> methodKeepRules = new ArrayList<>();
+ rule.getFieldAndMethodRules(fieldKeepRules, methodKeepRules);
Map<Predicate<DexDefinition>, DexClass> preconditionSupplier;
if (rule instanceof ProguardKeepRule) {
ProguardKeepRule keepRule = rule.asProguardKeepRule();
@@ -398,7 +379,7 @@
preconditionSupplier = ImmutableMap.of(definition -> true, clazz);
markMatchingVisibleMethods(
clazz,
- memberKeepRules,
+ methodKeepRules,
rule,
preconditionSupplier,
keepRule.getIncludeDescriptorClasses(),
@@ -406,7 +387,7 @@
ifRulePreconditionMatch);
markMatchingVisibleFields(
clazz,
- memberKeepRules,
+ fieldKeepRules,
rule,
preconditionSupplier,
keepRule.getIncludeDescriptorClasses(),
@@ -433,7 +414,7 @@
}
markMatchingVisibleMethods(
clazz,
- memberKeepRules,
+ methodKeepRules,
rule,
preconditionSupplier,
keepRule.getIncludeDescriptorClasses(),
@@ -441,7 +422,7 @@
ifRulePreconditionMatch);
markMatchingVisibleFields(
clazz,
- memberKeepRules,
+ fieldKeepRules,
rule,
preconditionSupplier,
keepRule.getIncludeDescriptorClasses(),
@@ -467,29 +448,29 @@
|| rule instanceof ProguardWhyAreYouKeepingRule) {
markClass(clazz, rule, ifRulePreconditionMatch);
markMatchingVisibleMethods(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, methodKeepRules, rule, null, true, true, ifRulePreconditionMatch);
markMatchingVisibleFields(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, fieldKeepRules, rule, null, true, true, ifRulePreconditionMatch);
} else if (rule instanceof ProguardAssumeMayHaveSideEffectsRule) {
markMatchingVisibleMethods(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, methodKeepRules, rule, null, true, true, ifRulePreconditionMatch);
markMatchingOverriddenMethods(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, methodKeepRules, rule, null, true, true, ifRulePreconditionMatch);
markMatchingVisibleFields(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, fieldKeepRules, rule, null, true, true, ifRulePreconditionMatch);
} else if (rule instanceof ProguardAssumeNoSideEffectRule
|| rule instanceof ProguardAssumeValuesRule) {
if (assumeInfoCollectionBuilder != null) {
markMatchingVisibleMethods(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, methodKeepRules, rule, null, true, true, ifRulePreconditionMatch, timing);
markMatchingOverriddenMethods(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, methodKeepRules, rule, null, true, true, ifRulePreconditionMatch, timing);
markMatchingVisibleFields(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, fieldKeepRules, rule, null, true, true, ifRulePreconditionMatch, timing);
}
} else if (rule instanceof NoFieldTypeStrengtheningRule
|| rule instanceof NoRedundantFieldLoadEliminationRule) {
- markMatchingFields(clazz, memberKeepRules, rule, null, ifRulePreconditionMatch);
+ markMatchingFields(clazz, fieldKeepRules, rule, ifRulePreconditionMatch);
} else if (rule instanceof InlineRule
|| rule instanceof KeepConstantArgumentRule
|| rule instanceof KeepUnusedReturnValueRule
@@ -501,7 +482,7 @@
|| rule instanceof ReprocessMethodRule
|| rule instanceof WhyAreYouNotInliningRule
|| rule.isMaximumRemovedAndroidLogLevelRule()) {
- markMatchingMethods(clazz, memberKeepRules, rule, null, ifRulePreconditionMatch);
+ markMatchingMethods(clazz, methodKeepRules, rule, ifRulePreconditionMatch);
} else if (rule instanceof ClassInlineRule
|| rule instanceof NoUnusedInterfaceRemovalRule
|| rule instanceof NoVerticalClassMergingRule
@@ -512,15 +493,15 @@
}
} else if (rule instanceof NoValuePropagationRule) {
markMatchingVisibleMethods(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, methodKeepRules, rule, null, true, true, ifRulePreconditionMatch);
markMatchingVisibleFields(
- clazz, memberKeepRules, rule, null, true, true, ifRulePreconditionMatch);
+ clazz, fieldKeepRules, rule, null, true, true, ifRulePreconditionMatch);
} else if (rule instanceof ProguardIdentifierNameStringRule) {
- markMatchingFields(clazz, memberKeepRules, rule, null, ifRulePreconditionMatch);
- markMatchingMethods(clazz, memberKeepRules, rule, null, ifRulePreconditionMatch);
+ markMatchingFields(clazz, fieldKeepRules, rule, ifRulePreconditionMatch);
+ markMatchingMethods(clazz, methodKeepRules, rule, ifRulePreconditionMatch);
} else {
assert rule instanceof ConvertCheckNotNullRule;
- markMatchingMethods(clazz, memberKeepRules, rule, null, ifRulePreconditionMatch);
+ markMatchingMethods(clazz, methodKeepRules, rule, ifRulePreconditionMatch);
}
}
@@ -538,7 +519,7 @@
DexClass clazz = application.definitionFor(type);
// Ignore keep rule iff it does not reference a class in the app.
if (clazz != null) {
- process(clazz, rule, ifRulePreconditionMatch);
+ process(clazz, rule, ifRulePreconditionMatch, timing);
}
}
timing.end();
@@ -575,8 +556,9 @@
List<DexClass> job = new ArrayList<>(classes);
tasks.submitUnchecked(
() -> {
- Timing threadTiming = timing.createThreadTiming("Process rule", options);
- process(job, rule, ifRulePreconditionMatch);
+ Timing threadTiming =
+ timing.createThreadTiming("Process rule: " + rule, options);
+ process(job, rule, ifRulePreconditionMatch, threadTiming);
threadTiming.end().notifyThreadTimingFinished();
});
classes.clear();
@@ -588,7 +570,7 @@
tasks.submitUnchecked(
() -> {
Timing threadTiming = timing.createThreadTiming("Process rule", options);
- process(classes, rule, ifRulePreconditionMatch);
+ process(classes, rule, ifRulePreconditionMatch, threadTiming);
threadTiming.end().notifyThreadTimingFinished();
});
}
@@ -599,12 +581,12 @@
Timing threadTiming = timing.createThreadTiming("Evaluate non-program", options);
if (rule.isApplicableToClasspathClasses()) {
for (DexClasspathClass clazz : application.classpathClasses()) {
- process(clazz, rule, ifRulePreconditionMatch);
+ process(clazz, rule, ifRulePreconditionMatch, threadTiming);
}
}
if (rule.isApplicableToLibraryClasses()) {
for (DexLibraryClass clazz : application.libraryClasses()) {
- process(clazz, rule, ifRulePreconditionMatch);
+ process(clazz, rule, ifRulePreconditionMatch, threadTiming);
}
}
threadTiming.end().notifyThreadTimingFinished();
@@ -626,7 +608,7 @@
subtypingInfo,
application.classes(),
alwaysTrue(),
- clazz -> process(clazz, rule, ifRulePreconditionMatch));
+ clazz -> process(clazz, rule, ifRulePreconditionMatch, threadTiming));
threadTiming.end().notifyThreadTimingFinished();
});
}
@@ -788,13 +770,37 @@
Map<Predicate<DexDefinition>, DexClass> preconditionSupplier,
boolean includeClasspathClasses,
boolean includeLibraryClasses,
+ ProguardIfRulePreconditionMatch ifRulePreconditionMatch,
+ Timing timing) {
+ timing.begin("Mark visible methods");
+ markMatchingVisibleMethods(
+ clazz,
+ memberKeepRules,
+ rule,
+ preconditionSupplier,
+ includeClasspathClasses,
+ includeLibraryClasses,
+ ifRulePreconditionMatch);
+ timing.end();
+ }
+
+ private void markMatchingVisibleMethods(
+ DexClass clazz,
+ Collection<ProguardMemberRule> memberKeepRules,
+ ProguardConfigurationRule rule,
+ Map<Predicate<DexDefinition>, DexClass> preconditionSupplier,
+ boolean includeClasspathClasses,
+ boolean includeLibraryClasses,
ProguardIfRulePreconditionMatch ifRulePreconditionMatch) {
+ if (memberKeepRules.isEmpty()) {
+ return;
+ }
+ // TODO(b/422947619): Evaluate the impact of matching the rule against the superclass.
+ boolean includeSuperclasses = !rule.isTrivialAllClassMatch();
Set<Wrapper<DexMethod>> methodsMarked =
options.forceProguardCompatibility ? null : new HashSet<>();
- Deque<DexClass> worklist = new ArrayDeque<>();
- worklist.add(clazz);
- while (!worklist.isEmpty()) {
- DexClass currentClass = worklist.pop();
+ DexClass currentClass = clazz;
+ do {
if (!includeClasspathClasses && currentClass.isClasspathClass()) {
break;
}
@@ -805,7 +811,7 @@
currentClass.forEachClassMethodMatching(
method ->
method.belongsToVirtualPool()
- || currentClass == clazz
+ || method.getHolderType().isIdenticalTo(clazz.getType())
|| (method.isStatic() && !method.isPrivate() && !method.isInitializer())
|| options.forceProguardCompatibility,
method -> {
@@ -819,13 +825,12 @@
precondition,
ifRulePreconditionMatch);
});
- if (currentClass.superType != null) {
- DexClass dexClass = application.definitionFor(currentClass.superType);
- if (dexClass != null) {
- worklist.add(dexClass);
- }
+ if (currentClass.hasSuperType() && includeSuperclasses) {
+ currentClass = application.definitionFor(currentClass.getSuperType());
+ } else {
+ break;
}
- }
+ } while (currentClass != null);
// TODO(b/143643942): Generalize the below approach to also work for subtyping hierarchies in
// fullmode.
if (clazz.isProgramClass()
@@ -978,13 +983,35 @@
Map<Predicate<DexDefinition>, DexClass> preconditionSupplier,
boolean includeClasspathClasses,
boolean includeLibraryClasses,
+ ProguardIfRulePreconditionMatch ifRulePreconditionMatch,
+ Timing timing) {
+ timing.begin("Mark overridden methods");
+ markMatchingOverriddenMethods(
+ clazz,
+ memberKeepRules,
+ rule,
+ preconditionSupplier,
+ includeClasspathClasses,
+ includeLibraryClasses,
+ ifRulePreconditionMatch);
+ timing.end();
+ }
+
+ private void markMatchingOverriddenMethods(
+ DexClass clazz,
+ Collection<ProguardMemberRule> memberKeepRules,
+ ProguardConfigurationRule rule,
+ Map<Predicate<DexDefinition>, DexClass> preconditionSupplier,
+ boolean includeClasspathClasses,
+ boolean includeLibraryClasses,
ProguardIfRulePreconditionMatch ifRulePreconditionMatch) {
+ if (memberKeepRules.isEmpty()) {
+ return;
+ }
Set<DexClass> visited = Sets.newIdentityHashSet();
- Deque<DexClass> worklist = new ArrayDeque<>();
// Intentionally skip the current `clazz`, assuming it's covered by
// markMatchingVisibleMethods.
- worklist.addAll(subtypingInfo.getSubclasses(clazz));
-
+ Deque<DexClass> worklist = new ArrayDeque<>(subtypingInfo.getSubclasses(clazz));
while (!worklist.isEmpty()) {
DexClass currentClass = worklist.poll();
if (!visited.add(currentClass)) {
@@ -1012,14 +1039,30 @@
DexClass clazz,
Collection<ProguardMemberRule> memberKeepRules,
ProguardConfigurationRule rule,
- Map<Predicate<DexDefinition>, DexClass> preconditionSupplier,
ProguardIfRulePreconditionMatch ifRulePreconditionMatch) {
clazz.forEachClassMethod(
- method -> {
- DexClass precondition =
- testAndGetPrecondition(method.getDefinition(), preconditionSupplier);
- markMethod(method, memberKeepRules, null, rule, precondition, ifRulePreconditionMatch);
- });
+ method -> markMethod(method, memberKeepRules, null, rule, null, ifRulePreconditionMatch));
+ }
+
+ private void markMatchingVisibleFields(
+ DexClass clazz,
+ Collection<ProguardMemberRule> memberKeepRules,
+ ProguardConfigurationRule rule,
+ Map<Predicate<DexDefinition>, DexClass> preconditionSupplier,
+ boolean includeClasspathClasses,
+ boolean includeLibraryClasses,
+ ProguardIfRulePreconditionMatch ifRulePreconditionMatch,
+ Timing timing) {
+ timing.begin("Mark visible fields");
+ markMatchingVisibleFields(
+ clazz,
+ memberKeepRules,
+ rule,
+ preconditionSupplier,
+ includeClasspathClasses,
+ includeLibraryClasses,
+ ifRulePreconditionMatch);
+ timing.end();
}
private void markMatchingVisibleFields(
@@ -1030,6 +1073,11 @@
boolean includeClasspathClasses,
boolean includeLibraryClasses,
ProguardIfRulePreconditionMatch ifRulePreconditionMatch) {
+ if (memberKeepRules.isEmpty()) {
+ return;
+ }
+ // TODO(b/422947619): Evaluate the impact of matching the rule against the superclass.
+ boolean includeSuperclasses = !rule.isTrivialAllClassMatch();
while (clazz != null) {
if (!includeClasspathClasses && clazz.isClasspathClass()) {
return;
@@ -1043,7 +1091,11 @@
testAndGetPrecondition(field.getDefinition(), preconditionSupplier);
markField(field, memberKeepRules, rule, precondition, ifRulePreconditionMatch);
});
- clazz = clazz.superType == null ? null : application.definitionFor(clazz.superType);
+ if (clazz.hasSuperType() && includeSuperclasses) {
+ clazz = application.definitionFor(clazz.superType);
+ } else {
+ break;
+ }
}
}
@@ -1051,14 +1103,9 @@
DexClass clazz,
Collection<ProguardMemberRule> memberKeepRules,
ProguardConfigurationRule rule,
- Map<Predicate<DexDefinition>, DexClass> preconditionSupplier,
ProguardIfRulePreconditionMatch ifRulePreconditionMatch) {
clazz.forEachClassField(
- field -> {
- DexClass precondition =
- testAndGetPrecondition(field.getDefinition(), preconditionSupplier);
- markField(field, memberKeepRules, rule, precondition, ifRulePreconditionMatch);
- });
+ field -> markField(field, memberKeepRules, rule, null, ifRulePreconditionMatch));
}
// TODO(b/67934426): Test this code.
@@ -1147,105 +1194,6 @@
return containsAllAnnotations(rule.getClassAnnotations(), clazz);
}
- boolean satisfyInheritanceRule(DexClass clazz, ProguardConfigurationRule rule) {
- if (satisfyExtendsRule(clazz, rule)) {
- return true;
- }
-
- return satisfyImplementsRule(clazz, rule);
- }
-
- boolean satisfyExtendsRule(DexClass clazz, ProguardConfigurationRule rule) {
- if (anySuperTypeMatchesExtendsRule(clazz.superType, rule)) {
- return true;
- }
- // It is possible that this class used to inherit from another class X, but no longer does it,
- // because X has been merged into `clazz`.
- return anySourceMatchesInheritanceRuleDirectly(clazz, rule, false);
- }
-
- boolean anySuperTypeMatchesExtendsRule(DexType type, ProguardConfigurationRule rule) {
- while (type != null) {
- DexClass clazz = application.definitionFor(type);
- if (clazz == null) {
- // TODO(herhut): Warn about broken supertype chain?
- return false;
- }
- // TODO(b/110141157): Should the vertical class merger move annotations from the source to
- // the target class? If so, it is sufficient only to apply the annotation-matcher to the
- // annotations of `class`.
- if (rule.getInheritanceClassName().matches(clazz.type, appView)) {
- AnnotationMatchResult annotationMatchResult =
- containsAllAnnotations(rule.getInheritanceAnnotations(), clazz);
- if (annotationMatchResult != null) {
- handleMatchedAnnotation(annotationMatchResult);
- return true;
- }
- }
- type = clazz.superType;
- }
- return false;
- }
-
- boolean satisfyImplementsRule(DexClass clazz, ProguardConfigurationRule rule) {
- if (anyImplementedInterfaceMatchesImplementsRule(clazz, rule)) {
- return true;
- }
- // It is possible that this class used to implement an interface I, but no longer does it,
- // because I has been merged into `clazz`.
- return anySourceMatchesInheritanceRuleDirectly(clazz, rule, true);
- }
-
- private boolean anyImplementedInterfaceMatchesImplementsRule(
- DexClass clazz, ProguardConfigurationRule rule) {
- // TODO(herhut): Maybe it would be better to do this breadth first.
- if (clazz == null) {
- return false;
- }
- for (DexType iface : clazz.interfaces.values) {
- DexClass ifaceClass = application.definitionFor(iface);
- if (ifaceClass == null) {
- // TODO(herhut): Warn about broken supertype chain?
- return false;
- }
- // TODO(b/110141157): Should the vertical class merger move annotations from the source to
- // the target class? If so, it is sufficient only to apply the annotation-matcher to the
- // annotations of `ifaceClass`.
- if (rule.getInheritanceClassName().matches(iface, appView)) {
- AnnotationMatchResult annotationMatchResult =
- containsAllAnnotations(rule.getInheritanceAnnotations(), ifaceClass);
- if (annotationMatchResult != null) {
- handleMatchedAnnotation(annotationMatchResult);
- return true;
- }
- }
- if (anyImplementedInterfaceMatchesImplementsRule(ifaceClass, rule)) {
- return true;
- }
- }
- if (clazz.superType == null) {
- return false;
- }
- DexClass superClass = application.definitionFor(clazz.superType);
- if (superClass == null) {
- // TODO(herhut): Warn about broken supertype chain?
- return false;
- }
- return anyImplementedInterfaceMatchesImplementsRule(superClass, rule);
- }
-
- private boolean anySourceMatchesInheritanceRuleDirectly(
- DexClass clazz, ProguardConfigurationRule rule, boolean isInterface) {
- // TODO(b/110141157): Figure out what to do with annotations. Should the annotations of
- // the DexClass corresponding to `sourceType` satisfy the `annotation`-matcher?
- return appView.getVerticallyMergedClasses() != null
- && appView.getVerticallyMergedClasses().getSourcesFor(clazz.type).stream()
- .filter(
- sourceType ->
- appView.definitionFor(sourceType).accessFlags.isInterface() == isInterface)
- .anyMatch(rule.getInheritanceClassName()::matches);
- }
-
private boolean allRulesSatisfied(
Collection<ProguardMemberRule> memberKeepRules, DexClass clazz) {
for (ProguardMemberRule rule : memberKeepRules) {
@@ -1745,8 +1693,8 @@
if (rule.getMemberRules().isEmpty()) {
evaluateCheckDiscardClassAndAllMembersRule(clazz, rule);
} else if (clazz.hasFields() || clazz.hasMethods()) {
- markMatchingFields(clazz, rule.getMemberRules(), rule, null, null);
- markMatchingMethods(clazz, rule.getMemberRules(), rule, null, null);
+ markMatchingFields(clazz, rule.getMemberRules(), rule, null);
+ markMatchingMethods(clazz, rule.getMemberRules(), rule, null);
classesWithCheckDiscardedMembers.add(clazz);
}
}
