Remove static visibility bridges if possible
Bug: 197490166
Change-Id: Id56690625669c2772edadbd246a8a9b05055ee7a
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java
index 91f6172..2ab471a 100644
--- a/src/main/java/com/android/tools/r8/R8.java
+++ b/src/main/java/com/android/tools/r8/R8.java
@@ -473,7 +473,7 @@
// We can now remove visibility bridges. Note that we do not need to update the
// invoke-targets here, as the existing invokes will simply dispatch to the now
// visible super-method. MemberRebinding, if run, will then dispatch it correctly.
- new VisibilityBridgeRemover(appView.withLiveness()).run();
+ new VisibilityBridgeRemover(appView.withLiveness()).run(executorService);
}
}
@@ -720,7 +720,7 @@
// Remove unneeded visibility bridges that have been inserted for member rebinding.
// This can only be done if we have AppInfoWithLiveness.
if (appView.appInfo().hasLiveness()) {
- new VisibilityBridgeRemover(appView.withLiveness()).run();
+ new VisibilityBridgeRemover(appView.withLiveness()).run(executorService);
} else {
// If we don't have AppInfoWithLiveness here, it must be because we are not shrinking. When
// we are not shrinking, we can't move visibility bridges. In principle, though, it would be
diff --git a/src/main/java/com/android/tools/r8/graph/AppInfoWithClassHierarchy.java b/src/main/java/com/android/tools/r8/graph/AppInfoWithClassHierarchy.java
index c8b99a4..dc8e0bb 100644
--- a/src/main/java/com/android/tools/r8/graph/AppInfoWithClassHierarchy.java
+++ b/src/main/java/com/android/tools/r8/graph/AppInfoWithClassHierarchy.java
@@ -465,7 +465,7 @@
}
assert potentialHolder.isInterface();
for (DexEncodedMethod virtualMethod : potentialHolder.virtualMethods()) {
- if (virtualMethod.getReference().hasSameProtoAndName(method.getReference())
+ if (virtualMethod.getReference().match(method.getReference())
&& virtualMethod.accessFlags.isSameVisibility(method.accessFlags)) {
return true;
}
diff --git a/src/main/java/com/android/tools/r8/graph/DexMethod.java b/src/main/java/com/android/tools/r8/graph/DexMethod.java
index 5eaf805..c2eeffa 100644
--- a/src/main/java/com/android/tools/r8/graph/DexMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/DexMethod.java
@@ -218,14 +218,6 @@
return false;
}
- /**
- * Returns true if the other method has the same name and prototype (including signature and
- * return type), false otherwise.
- */
- public boolean hasSameProtoAndName(DexMethod other) {
- return name == other.name && proto == other.proto;
- }
-
@Override
public boolean match(DexMethod method) {
return match(method.getProto(), method.getName());
diff --git a/src/main/java/com/android/tools/r8/optimize/VisibilityBridgeRemover.java b/src/main/java/com/android/tools/r8/optimize/VisibilityBridgeRemover.java
index f073314..a990959 100644
--- a/src/main/java/com/android/tools/r8/optimize/VisibilityBridgeRemover.java
+++ b/src/main/java/com/android/tools/r8/optimize/VisibilityBridgeRemover.java
@@ -3,18 +3,22 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.optimize;
+import static com.android.tools.r8.utils.ThreadUtils.processItems;
+
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.ProgramMethod;
+import com.android.tools.r8.graph.PrunedItems;
import com.android.tools.r8.logging.Log;
import com.android.tools.r8.optimize.InvokeSingleTargetExtractor.InvokeKind;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
-import com.android.tools.r8.utils.ForEachable;
-import com.android.tools.r8.utils.IntBox;
import com.google.common.collect.Sets;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
public class VisibilityBridgeRemover {
@@ -24,50 +28,14 @@
this.appView = appView;
}
- private void removeUnneededVisibilityBridgesFromClass(DexProgramClass clazz) {
- DexEncodedMethod[] newDirectMethods =
- removeUnneededVisibilityBridges(
- clazz::forEachProgramDirectMethod, clazz.getMethodCollection().numberOfDirectMethods());
- if (newDirectMethods != null) {
- clazz.setDirectMethods(newDirectMethods);
- }
- DexEncodedMethod[] newVirtualMethods =
- removeUnneededVisibilityBridges(
- clazz::forEachProgramVirtualMethod,
- clazz.getMethodCollection().numberOfVirtualMethods());
- if (newVirtualMethods != null) {
- clazz.setVirtualMethods(newVirtualMethods);
- }
- }
-
- private DexEncodedMethod[] removeUnneededVisibilityBridges(
- ForEachable<ProgramMethod> methods, int size) {
- Set<DexEncodedMethod> methodsToBeRemoved = Sets.newIdentityHashSet();
- methods.forEach(
- method -> {
- if (isUnneededVisibilityBridge(method)) {
- methodsToBeRemoved.add(method.getDefinition());
- }
- });
- if (!methodsToBeRemoved.isEmpty()) {
- DexEncodedMethod[] newMethods = new DexEncodedMethod[size - methodsToBeRemoved.size()];
- IntBox i = new IntBox(0);
- methods.forEach(
- method -> {
- if (!methodsToBeRemoved.contains(method.getDefinition())) {
- newMethods[i.getAndIncrement()] = method.getDefinition();
- }
- });
- return newMethods;
- }
- return null;
- }
-
private boolean isUnneededVisibilityBridge(ProgramMethod method) {
+ // Clean-up the predicate check.
if (appView.appInfo().isPinned(method.getReference())) {
return false;
}
DexEncodedMethod definition = method.getDefinition();
+ // TODO(b/198133259): Extend to definitions that are not defined as bridges.
+ // TODO(b/197490164): Remove if method is abstract.
if (!definition.isBridge() || definition.isAbstract()) {
return false;
}
@@ -75,33 +43,67 @@
new InvokeSingleTargetExtractor(appView.dexItemFactory());
method.registerCodeReferences(targetExtractor);
DexMethod target = targetExtractor.getTarget();
- InvokeKind kind = targetExtractor.getKind();
// javac-generated visibility forward bridge method has same descriptor (name, signature and
// return type).
- if (target != null && target.hasSameProtoAndName(method.getReference())) {
- assert !definition.isPrivate() && !definition.isInstanceInitializer();
- if (kind == InvokeKind.SUPER) {
- // This is a visibility forward, so check for the direct target.
- DexEncodedMethod targetMethod =
- appView.appInfo().unsafeResolveMethodDueToDexFormat(target).getSingleTarget();
- if (targetMethod != null && targetMethod.accessFlags.isPublic()) {
- if (Log.ENABLED) {
- Log.info(
- getClass(),
- "Removing visibility forwarding %s -> %s",
- method,
- targetMethod.getReference());
- }
- return true;
- }
- }
+ if (target == null || !target.match(method.getReference())) {
+ return false;
}
+ assert !definition.isPrivate() && !definition.isInstanceInitializer();
+ if (!isTargetingSuperMethod(method, targetExtractor.getKind(), target)) {
+ return false;
+ }
+ // This is a visibility forward, so check for the direct target.
+ DexEncodedMethod targetMethod =
+ appView.appInfo().unsafeResolveMethodDueToDexFormat(target).getSingleTarget();
+ if (targetMethod == null || !targetMethod.accessFlags.isPublic()) {
+ return false;
+ }
+ if (Log.ENABLED) {
+ Log.info(
+ getClass(),
+ "Removing visibility forwarding %s -> %s",
+ method,
+ targetMethod.getReference());
+ }
+ return true;
+ }
+
+ private boolean isTargetingSuperMethod(ProgramMethod method, InvokeKind kind, DexMethod target) {
+ if (kind == InvokeKind.SUPER) {
+ return true;
+ }
+ if (kind == InvokeKind.STATIC) {
+ return appView.appInfo().isStrictSubtypeOf(method.getHolderType(), target.holder);
+ }
+ assert false : "Unexpected invoke-kind for visibility bridge";
return false;
}
- public void run() {
- for (DexProgramClass clazz : appView.appInfo().classes()) {
- removeUnneededVisibilityBridgesFromClass(clazz);
- }
+ public void run(ExecutorService executorService) throws ExecutionException {
+ // Collect all visibility bridges to remove.
+ ConcurrentHashMap<DexProgramClass, Set<DexEncodedMethod>> visibilityBridgesToRemove =
+ new ConcurrentHashMap<>();
+ processItems(
+ appView.appInfo().classes(),
+ clazz -> {
+ Set<DexEncodedMethod> bridgesToRemoveForClass = Sets.newIdentityHashSet();
+ clazz.forEachProgramMethod(
+ method -> {
+ if (isUnneededVisibilityBridge(method)) {
+ bridgesToRemoveForClass.add(method.getDefinition());
+ }
+ });
+ if (!bridgesToRemoveForClass.isEmpty()) {
+ visibilityBridgesToRemove.put(clazz, bridgesToRemoveForClass);
+ }
+ },
+ executorService);
+ // Remove all bridges found.
+ PrunedItems.Builder builder = PrunedItems.builder();
+ visibilityBridgesToRemove.forEach(
+ (clazz, methods) -> {
+ clazz.getMethodCollection().removeMethods(methods);
+ methods.forEach(method -> builder.addRemovedMethod(method.getReference()));
+ });
}
}
diff --git a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
index be31b16..acc8410 100644
--- a/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
+++ b/src/main/java/com/android/tools/r8/shaking/AppInfoWithLiveness.java
@@ -1048,6 +1048,10 @@
keepInfo.mutate(
keepInfo -> keepInfo.removeKeepInfoForPrunedItems(prunedItems.getRemovedClasses()));
}
+ if (!prunedItems.getRemovedMethods().isEmpty()) {
+ keepInfo.mutate(
+ keepInfo -> keepInfo.removeKeepInfoForPrunedItems(prunedItems.getRemovedMethods()));
+ }
return new AppInfoWithLiveness(this, prunedItems);
}
diff --git a/src/main/java/com/android/tools/r8/shaking/KeepInfoCollection.java b/src/main/java/com/android/tools/r8/shaking/KeepInfoCollection.java
index 0dcddda..c260f3e 100644
--- a/src/main/java/com/android/tools/r8/shaking/KeepInfoCollection.java
+++ b/src/main/java/com/android/tools/r8/shaking/KeepInfoCollection.java
@@ -244,10 +244,20 @@
this.methodRuleInstances = methodRuleInstances;
}
- public void removeKeepInfoForPrunedItems(Set<DexType> removedClasses) {
- keepClassInfo.keySet().removeIf(removedClasses::contains);
- keepFieldInfo.keySet().removeIf(field -> removedClasses.contains(field.getHolderType()));
- keepMethodInfo.keySet().removeIf(method -> removedClasses.contains(method.getHolderType()));
+ public void removeKeepInfoForPrunedItems(Set<? extends DexReference> removedReferences) {
+ keepClassInfo.keySet().removeIf(removedReferences::contains);
+ keepFieldInfo
+ .keySet()
+ .removeIf(
+ field ->
+ (removedReferences.contains(field)
+ || removedReferences.contains(field.getHolderType())));
+ keepMethodInfo
+ .keySet()
+ .removeIf(
+ method ->
+ (removedReferences.contains(method)
+ || removedReferences.contains(method.getHolderType())));
}
@Override
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningIntoVisibilityBridgeTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningIntoVisibilityBridgeTest.java
index 46727f2..75679e5 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningIntoVisibilityBridgeTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/InliningIntoVisibilityBridgeTest.java
@@ -9,16 +9,19 @@
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentAndRenamed;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
import com.android.tools.r8.R8TestBuilder;
import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
import com.android.tools.r8.ir.optimize.inliner.testclasses.InliningIntoVisibilityBridgeTestClasses;
import com.android.tools.r8.ir.optimize.inliner.testclasses.InliningIntoVisibilityBridgeTestClasses.InliningIntoVisibilityBridgeTestClassB;
import com.android.tools.r8.utils.BooleanUtils;
import com.android.tools.r8.utils.StringUtils;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -28,14 +31,17 @@
@RunWith(Parameterized.class)
public class InliningIntoVisibilityBridgeTest extends TestBase {
+ private final TestParameters parameters;
private final boolean neverInline;
- @Parameters(name = "Never inline: {0}")
- public static Boolean[] data() {
- return BooleanUtils.values();
+ @Parameters(name = "{0}, never inline: {1}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ getTestParameters().withAllRuntimesAndApiLevels().build(), BooleanUtils.values());
}
- public InliningIntoVisibilityBridgeTest(boolean neverInline) {
+ public InliningIntoVisibilityBridgeTest(TestParameters parameters, boolean neverInline) {
+ this.parameters = parameters;
this.neverInline = neverInline;
}
@@ -44,7 +50,7 @@
String expectedOutput = StringUtils.lines("Hello world", "Hello world", "Hello world");
R8TestRunResult result =
- testForR8(Backend.DEX)
+ testForR8(parameters.getBackend())
.addInnerClasses(InliningIntoVisibilityBridgeTest.class)
.addInnerClasses(InliningIntoVisibilityBridgeTestClasses.class)
.addKeepMainRule(TestClass.class)
@@ -54,8 +60,9 @@
.applyIf(!neverInline, R8TestBuilder::enableForceInliningAnnotations)
.enableNoVerticalClassMergingAnnotations()
.enableProguardTestOptions()
+ .setMinApi(parameters.getApiLevel())
.compile()
- .run(TestClass.class)
+ .run(parameters.getRuntime(), TestClass.class)
.assertSuccessWithOutput(expectedOutput);
// Verify that A.method() is only there if there is an explicit -neverinline rule.
@@ -75,9 +82,11 @@
assertThat(classSubject, isPresent());
MethodSubject methodSubject = classSubject.uniqueMethodWithName("method");
- assertThat(methodSubject, isPresentAndRenamed());
- assertEquals(neverInline, methodSubject.isBridge());
- assertEquals(neverInline, methodSubject.isSynthetic());
+ if (!neverInline) {
+ assertThat(methodSubject, isPresentAndRenamed());
+ assertFalse(methodSubject.isBridge());
+ assertFalse(methodSubject.isSynthetic());
+ }
}
}
diff --git a/src/test/java/com/android/tools/r8/memberrebinding/MemberRebindingRemoveStaticBridgeTest.java b/src/test/java/com/android/tools/r8/memberrebinding/MemberRebindingRemoveStaticBridgeTest.java
index b0a7e59..8bdaa09 100644
--- a/src/test/java/com/android/tools/r8/memberrebinding/MemberRebindingRemoveStaticBridgeTest.java
+++ b/src/test/java/com/android/tools/r8/memberrebinding/MemberRebindingRemoveStaticBridgeTest.java
@@ -4,10 +4,9 @@
package com.android.tools.r8.memberrebinding;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isBridge;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
import com.android.tools.r8.NeverInline;
import com.android.tools.r8.NoVerticalClassMerging;
@@ -16,7 +15,6 @@
import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.utils.DescriptorUtils;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
-import com.android.tools.r8.utils.codeinspector.FoundMethodSubject;
import com.android.tools.r8.utils.codeinspector.HorizontallyMergedClassesInspector;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -57,10 +55,7 @@
inspector -> {
ClassSubject clazz = inspector.clazz(B.class);
assertThat(clazz, isPresent());
- // TODO(b/197490166): The inserted bridge should be removed.
- assertEquals(1, clazz.allMethods().size());
- FoundMethodSubject foundMethodSubject = clazz.allMethods().get(0);
- assertThat(foundMethodSubject, isBridge());
+ assertTrue(clazz.allMethods().isEmpty());
});
}
