Extend bridge removal to redundant forwarding constructors
This is still disabled by default (controlled by `enableRedundantConstructorBridgeRemoval`).
The optimization leads to non-rebound constructor calls, which are not always correctly rewritten when there are lens rewritings.
Change-Id: I77b650a9ba05d552c12b795517e8523549070f66
diff --git a/src/main/java/com/android/tools/r8/optimize/InvokeSingleTargetExtractor.java b/src/main/java/com/android/tools/r8/optimize/InvokeSingleTargetExtractor.java
index 77ea56c..11e1377 100644
--- a/src/main/java/com/android/tools/r8/optimize/InvokeSingleTargetExtractor.java
+++ b/src/main/java/com/android/tools/r8/optimize/InvokeSingleTargetExtractor.java
@@ -55,7 +55,7 @@
@Override
public void registerInvokeDirect(DexMethod method) {
- invalid();
+ setTarget(method, InvokeKind.DIRECT);
}
@Override
@@ -109,6 +109,7 @@
}
public enum InvokeKind {
+ DIRECT,
VIRTUAL,
STATIC,
SUPER,
diff --git a/src/main/java/com/android/tools/r8/optimize/RedundantBridgeRemover.java b/src/main/java/com/android/tools/r8/optimize/RedundantBridgeRemover.java
index ff42b4c..6c2af55 100644
--- a/src/main/java/com/android/tools/r8/optimize/RedundantBridgeRemover.java
+++ b/src/main/java/com/android/tools/r8/optimize/RedundantBridgeRemover.java
@@ -11,11 +11,11 @@
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.ir.optimize.info.bridge.BridgeInfo;
import com.android.tools.r8.optimize.InvokeSingleTargetExtractor.InvokeKind;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
-import com.google.common.collect.Sets;
-import java.util.Set;
+import com.android.tools.r8.utils.collections.ProgramMethodSet;
+import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
@@ -28,15 +28,16 @@
this.appView = appView;
}
- private boolean isUnneededVisibilityBridge(ProgramMethod method) {
+ private boolean isRedundantBridge(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()) {
+ BridgeInfo bridgeInfo = definition.getOptimizationInfo().getBridgeInfo();
+ boolean isBridge = definition.isBridge() || bridgeInfo != null;
+ if (!isBridge || definition.isAbstract()) {
return false;
}
InvokeSingleTargetExtractor targetExtractor = new InvokeSingleTargetExtractor(appView, method);
@@ -47,7 +48,6 @@
if (target == null || !target.match(method.getReference())) {
return false;
}
- assert !definition.isPrivate() && !definition.isInstanceInitializer();
if (!isTargetingSuperMethod(method, targetExtractor.getKind(), target)) {
return false;
}
@@ -57,9 +57,22 @@
.appInfo()
.unsafeResolveMethodDueToDexFormatLegacy(target)
.getResolvedProgramMethod();
- if (targetMethod == null || !targetMethod.getAccessFlags().isPublic()) {
+ if (targetMethod == null) {
return false;
}
+ if (method.getAccessFlags().isPublic()) {
+ if (!targetMethod.getAccessFlags().isPublic()) {
+ return false;
+ }
+ } else {
+ if (targetMethod.getAccessFlags().isProtected()
+ && !targetMethod.getHolderType().isSamePackage(method.getHolderType())) {
+ return false;
+ }
+ if (targetMethod.getAccessFlags().isPrivate()) {
+ return false;
+ }
+ }
if (definition.isStatic()
&& method.getHolder().hasClassInitializer()
&& method
@@ -67,13 +80,6 @@
.classInitializationMayHaveSideEffectsInContext(appView, targetMethod)) {
return false;
}
- if (Log.ENABLED) {
- Log.info(
- getClass(),
- "Removing visibility forwarding %s -> %s",
- method,
- targetMethod.getReference());
- }
return true;
}
@@ -81,6 +87,14 @@
if (kind == InvokeKind.ILLEGAL) {
return false;
}
+ if (kind == InvokeKind.DIRECT) {
+ return method.getDefinition().isInstanceInitializer()
+ && appView.options().canHaveNonReboundConstructorInvoke()
+ && appView.testing().enableRedundantConstructorBridgeRemoval
+ && appView.appInfo().isStrictSubtypeOf(method.getHolderType(), target.getHolderType());
+ }
+ assert !method.getAccessFlags().isPrivate();
+ assert !method.getDefinition().isInstanceInitializer();
if (kind == InvokeKind.SUPER) {
return true;
}
@@ -92,33 +106,42 @@
}
public void run(ExecutorService executorService) throws ExecutionException {
- // Collect all visibility bridges to remove.
- if (!appView.options().enableVisibilityBridgeRemoval) {
- return;
- }
- ConcurrentHashMap<DexProgramClass, Set<DexEncodedMethod>> visibilityBridgesToRemove =
- new ConcurrentHashMap<>();
+ // Collect all redundant bridges to remove.
+ Map<DexProgramClass, ProgramMethodSet> bridgesToRemove =
+ computeBridgesToRemove(executorService);
+ pruneApp(bridgesToRemove, executorService);
+ }
+
+ private Map<DexProgramClass, ProgramMethodSet> computeBridgesToRemove(
+ ExecutorService executorService) throws ExecutionException {
+ Map<DexProgramClass, ProgramMethodSet> bridgesToRemove = new ConcurrentHashMap<>();
processItems(
appView.appInfo().classes(),
clazz -> {
- Set<DexEncodedMethod> bridgesToRemoveForClass = Sets.newIdentityHashSet();
+ ProgramMethodSet bridgesToRemoveForClass = ProgramMethodSet.create();
clazz.forEachProgramMethod(
method -> {
- if (isUnneededVisibilityBridge(method)) {
- bridgesToRemoveForClass.add(method.getDefinition());
+ if (isRedundantBridge(method)) {
+ bridgesToRemoveForClass.add(method);
}
});
if (!bridgesToRemoveForClass.isEmpty()) {
- visibilityBridgesToRemove.put(clazz, bridgesToRemoveForClass);
+ bridgesToRemove.put(clazz, bridgesToRemoveForClass);
}
},
executorService);
- // Remove all bridges found.
- PrunedItems.Builder builder = PrunedItems.builder();
- visibilityBridgesToRemove.forEach(
+ return bridgesToRemove;
+ }
+
+ private void pruneApp(
+ Map<DexProgramClass, ProgramMethodSet> bridgesToRemove, ExecutorService executorService)
+ throws ExecutionException {
+ PrunedItems.Builder prunedItemsBuilder = PrunedItems.builder().setPrunedApp(appView.app());
+ bridgesToRemove.forEach(
(clazz, methods) -> {
- clazz.getMethodCollection().removeMethods(methods);
- methods.forEach(method -> builder.addRemovedMethod(method.getReference()));
+ clazz.getMethodCollection().removeMethods(methods.toDefinitionSet());
+ methods.forEach(method -> prunedItemsBuilder.addRemovedMethod(method.getReference()));
});
+ appView.pruneItems(prunedItemsBuilder.build(), executorService);
}
}
diff --git a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
index 90b9dfb..bbbe4c2 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -2508,39 +2508,41 @@
private void handleInvokeOfDirectTarget(
DexMethod reference, ProgramDefinition context, KeepReason reason) {
- DexType holder = reference.holder;
- DexProgramClass clazz = getProgramClassOrNull(holder, context);
- if (clazz == null) {
- recordMethodReference(reference, context);
- return;
- }
- // TODO(zerny): Is it ok that we lookup in both the direct and virtual pool here?
- DexEncodedMethod encodedMethod = clazz.lookupMethod(reference);
- if (encodedMethod == null) {
- failedMethodResolutionTargets.add(reference);
- return;
- }
+ resolveMethod(reference, context, reason)
+ .forEachMethodResolutionResult(
+ resolutionResult -> {
+ if (resolutionResult.isFailedResolution()) {
+ failedMethodResolutionTargets.add(reference);
+ return;
+ }
- ProgramMethod method = new ProgramMethod(clazz, encodedMethod);
+ if (!resolutionResult.isSingleResolution()
+ || !resolutionResult.getResolvedHolder().isProgramClass()) {
+ return;
+ }
- // We have to mark the resolved method as targeted even if it cannot actually be invoked
- // to make sure the invocation will keep failing in the appropriate way.
- markMethodAsTargeted(method, reason);
+ ProgramMethod resolvedMethod =
+ resolutionResult.asSingleResolution().getResolvedProgramMethod();
- // Only mark methods for which invocation will succeed at runtime live.
- if (encodedMethod.isStatic()) {
- return;
- }
+ // We have to mark the resolved method as targeted even if it cannot actually be
+ // invoked to make sure the invocation will keep failing in the appropriate way.
+ markMethodAsTargeted(resolvedMethod, reason);
- markDirectStaticOrConstructorMethodAsLive(method, reason);
+ // Only mark methods for which invocation will succeed at runtime live.
+ if (resolvedMethod.getAccessFlags().isStatic()) {
+ return;
+ }
- // It is valid to have an invoke-direct instruction in a default interface method that
- // targets another default method in the same interface (see testInvokeSpecialToDefault-
- // Method). In a class, that would lead to a verification error.
- if (encodedMethod.isNonPrivateVirtualMethod()
- && virtualMethodsTargetedByInvokeDirect.add(encodedMethod.getReference())) {
- workList.enqueueMarkMethodLiveAction(method, context, reason);
- }
+ markDirectStaticOrConstructorMethodAsLive(resolvedMethod, reason);
+
+ // It is valid to have an invoke-direct instruction in a default interface method that
+ // targets another default method in the same interface. In a class, that would lead
+ // to a verification error. See also testInvokeSpecialToDefaultMethod.
+ if (resolvedMethod.getDefinition().isNonPrivateVirtualMethod()
+ && virtualMethodsTargetedByInvokeDirect.add(resolvedMethod.getReference())) {
+ workList.enqueueMarkMethodLiveAction(resolvedMethod, context, reason);
+ }
+ });
}
private void ensureFromLibraryOrThrow(DexType type, DexLibraryClass context) {
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 9c2c3b1..70495fb 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -347,7 +347,6 @@
public boolean readDebugSetFileEvent = false;
public boolean disableL8AnnotationRemoval =
System.getProperty("com.android.tools.r8.disableL8AnnotationRemoval") != null;
- public boolean enableVisibilityBridgeRemoval = true;
public int callGraphLikelySpuriousCallEdgeThreshold = 50;
@@ -1920,6 +1919,7 @@
public boolean enableDeadSwitchCaseElimination = true;
public boolean enableInvokeSuperToInvokeVirtualRewriting = true;
public boolean enableMultiANewArrayDesugaringForClassFiles = false;
+ public boolean enableRedundantConstructorBridgeRemoval = false;
public boolean enableSwitchToIfRewriting = true;
public boolean enableEnumUnboxingDebugLogs =
System.getProperty("com.android.tools.r8.enableEnumUnboxingDebugLogs") != null;
@@ -2702,4 +2702,8 @@
public boolean canHaveDalvikEmptyAnnotationSetBug() {
return canHaveBugPresentUntil(AndroidApiLevel.J_MR1);
}
+
+ public boolean canHaveNonReboundConstructorInvoke() {
+ return isGeneratingDex() && minApiLevel.isGreaterThanOrEqualTo(AndroidApiLevel.L);
+ }
}
diff --git a/src/test/java/com/android/tools/r8/memberrebinding/StaticFieldClassInitMemberRebindingTest.java b/src/test/java/com/android/tools/r8/memberrebinding/StaticFieldClassInitMemberRebindingTest.java
index c31c93d..7ab14c7 100644
--- a/src/test/java/com/android/tools/r8/memberrebinding/StaticFieldClassInitMemberRebindingTest.java
+++ b/src/test/java/com/android/tools/r8/memberrebinding/StaticFieldClassInitMemberRebindingTest.java
@@ -47,7 +47,6 @@
.setMinApi(parameters.getApiLevel())
.addKeepMainRule(Main.class)
.enableInliningAnnotations()
- .addOptionsModification(options -> options.enableVisibilityBridgeRemoval = false)
.run(parameters.getRuntime(), Main.class)
// TODO(b/220668540): R8 should not change class loading semantics.
.assertSuccessWithOutputLines(R8_EXPECTED);
diff --git a/src/test/java/com/android/tools/r8/shaking/ForwardingConstructorShakingOnDexTest.java b/src/test/java/com/android/tools/r8/shaking/ForwardingConstructorShakingOnDexTest.java
index 78d2096..6ff7000 100644
--- a/src/test/java/com/android/tools/r8/shaking/ForwardingConstructorShakingOnDexTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/ForwardingConstructorShakingOnDexTest.java
@@ -14,6 +14,7 @@
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.android.tools.r8.utils.codeinspector.FoundMethodSubject;
@@ -39,6 +40,8 @@
testForR8(parameters.getBackend())
.addInnerClasses(getClass())
.addKeepMainRule(Main.class)
+ .addOptionsModification(
+ options -> options.testing.enableRedundantConstructorBridgeRemoval = true)
.enableConstantArgumentAnnotations()
.enableNeverClassInliningAnnotations()
.enableNoVerticalClassMergingAnnotations()
@@ -50,19 +53,25 @@
}
private void inspect(CodeInspector inspector) {
+ boolean canHaveNonReboundConstructorInvoke =
+ parameters.isDexRuntime()
+ && parameters.getApiLevel().isGreaterThanOrEqualTo(AndroidApiLevel.L);
+
ClassSubject aClassSubject = inspector.clazz(A.class);
assertThat(aClassSubject, isPresent());
assertEquals(2, aClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
ClassSubject bClassSubject = inspector.clazz(B.class);
assertThat(bClassSubject, isPresent());
- // TODO(b/173751869): Should be 0 when compiling for dex and API is above Dalvik.
- assertEquals(2, bClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
+ assertEquals(
+ canHaveNonReboundConstructorInvoke ? 0 : 2,
+ bClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
ClassSubject cClassSubject = inspector.clazz(C.class);
assertThat(cClassSubject, isPresent());
- // TODO(b/173751869): Should be 0 when compiling for dex and API is above Dalvik.
- assertEquals(2, cClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
+ assertEquals(
+ canHaveNonReboundConstructorInvoke ? 0 : 2,
+ cClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
}
static class Main {
diff --git a/src/test/java/com/android/tools/r8/shaking/RetainIndirectlyReferencedConstructorShakingOnDexTest.java b/src/test/java/com/android/tools/r8/shaking/RetainIndirectlyReferencedConstructorShakingOnDexTest.java
index 172249c..f914a87 100644
--- a/src/test/java/com/android/tools/r8/shaking/RetainIndirectlyReferencedConstructorShakingOnDexTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/RetainIndirectlyReferencedConstructorShakingOnDexTest.java
@@ -12,6 +12,7 @@
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.android.tools.r8.utils.codeinspector.FoundMethodSubject;
@@ -37,6 +38,8 @@
testForR8(parameters.getBackend())
.addInnerClasses(getClass())
.addKeepMainRule(Main.class)
+ .addOptionsModification(
+ options -> options.testing.enableRedundantConstructorBridgeRemoval = true)
.enableNoVerticalClassMergingAnnotations()
.setMinApi(parameters.getApiLevel())
.compile()
@@ -46,6 +49,10 @@
}
private void inspect(CodeInspector inspector) {
+ boolean canHaveNonReboundConstructorInvoke =
+ parameters.isDexRuntime()
+ && parameters.getApiLevel().isGreaterThanOrEqualTo(AndroidApiLevel.L);
+
// A.<init> should be retained despite the fact that there is no invoke-direct in the program
// that directly targets A.<init> when B.<init> is removed.
ClassSubject aClassSubject = inspector.clazz(A.class);
@@ -54,8 +61,9 @@
ClassSubject bClassSubject = inspector.clazz(B.class);
assertThat(bClassSubject, isPresent());
- // TODO(b/173751869): Should be 0 when compiling for dex and API is above Dalvik.
- assertEquals(1, bClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
+ assertEquals(
+ canHaveNonReboundConstructorInvoke ? 0 : 1,
+ bClassSubject.allMethods(FoundMethodSubject::isInstanceInitializer).size());
}
static class Main {
