Only split virtual methods called by invoke-super in class merger
Bug: b/323022702
Change-Id: I0f9e0e104af6a500dd7a975dff0078cab06af8a0
diff --git a/src/main/java/com/android/tools/r8/verticalclassmerging/ClassMerger.java b/src/main/java/com/android/tools/r8/verticalclassmerging/ClassMerger.java
index f4e1b95..ce747d5 100644
--- a/src/main/java/com/android/tools/r8/verticalclassmerging/ClassMerger.java
+++ b/src/main/java/com/android/tools/r8/verticalclassmerging/ClassMerger.java
@@ -6,6 +6,7 @@
import static com.android.tools.r8.graph.DexProgramClass.asProgramClassOrNull;
import static com.android.tools.r8.ir.code.InvokeType.STATIC;
import static com.android.tools.r8.ir.code.InvokeType.VIRTUAL;
+import static com.google.common.base.Predicates.alwaysTrue;
import static java.util.function.Predicate.not;
import com.android.tools.r8.cf.CfVersion;
@@ -73,6 +74,7 @@
private final ClassMergerSharedData sharedData;
private final List<IncompleteVerticalClassMergerBridgeCode> synthesizedBridges;
+ private Predicate<DexEncodedMethod> virtualMethodsTargetedByInvokeSuperInImmediateSubclass;
ClassMerger(
AppView<AppInfoWithLiveness> appView,
@@ -93,6 +95,7 @@
}
public void setup() {
+ setupVirtualMethodsTargetedByInvokeSuperInImmediateSubclass();
setupInvokeSuperMapping();
}
@@ -101,6 +104,11 @@
}
private void redirectSuperCallsToMethod(ProgramMethod sourceMethod) {
+ if (!virtualMethodsTargetedByInvokeSuperInImmediateSubclass.test(
+ sourceMethod.getDefinition())) {
+ // Not targeted by invoke-super.
+ return;
+ }
if (sourceMethod.getAccessFlags().belongsToDirectPool()) {
redirectSuperCallsToDirectMethod(sourceMethod);
} else {
@@ -214,6 +222,22 @@
for (DexEncodedMethod virtualMethod :
source.virtualMethods(not(DexEncodedMethod::isAbstract))) {
DexEncodedMethod shadowedBy = findMethodInTarget(virtualMethod);
+ if (!virtualMethodsTargetedByInvokeSuperInImmediateSubclass.test(virtualMethod)) {
+ if (shadowedBy != null) {
+ // Similarly to abstract methods that are overridden, we remove overridden non-abstract
+ // methods that are never targeted by invoke-super. These are guaranteed to be dead as we
+ // currently only allow merging uninstantiated classes into instantiated classes.
+ lensBuilder.recordSplit(virtualMethod, shadowedBy, null, null);
+ } else {
+ DexEncodedMethod movedMethod =
+ virtualMethod.toTypeSubstitutedMethodAsInlining(
+ virtualMethod.getReference().withHolder(target, dexItemFactory), dexItemFactory);
+ virtualMethods.put(movedMethod, movedMethod);
+ lensBuilder.recordMove(virtualMethod, movedMethod);
+ }
+ continue;
+ }
+
DexEncodedMethod resultingMethod;
if (source.isInterface()) {
// Moving a default interface method into its subtype. This method could be hit directly
@@ -367,6 +391,25 @@
verticallyMergedClassesBuilder.add(source, target);
}
+ private void setupVirtualMethodsTargetedByInvokeSuperInImmediateSubclass() {
+ if (!appView.options().getVerticalClassMergerOptions().isBridgeAnalysisEnabled()) {
+ virtualMethodsTargetedByInvokeSuperInImmediateSubclass = alwaysTrue();
+ return;
+ }
+ DexMethodSignatureSet methodsOfInterest = DexMethodSignatureSet.create();
+ source.forEachProgramMethodMatching(
+ method -> method.hasCode() && method.isVirtualMethod(), methodsOfInterest::add);
+ DexMethodSignatureSet result = DexMethodSignatureSet.create();
+ target.forEachProgramMethodMatching(
+ DexEncodedMethod::hasCode,
+ method -> {
+ InvokeSuperExtractor invokeSuperExtractor =
+ new InvokeSuperExtractor(appView, method, methodsOfInterest, result, source);
+ method.registerCodeReferences(invokeSuperExtractor);
+ });
+ virtualMethodsTargetedByInvokeSuperInImmediateSubclass = result::contains;
+ }
+
private void fixupAccessFlags() {
if (source.getAccessFlags().isEnum()) {
target.getAccessFlags().setEnum();
diff --git a/src/main/java/com/android/tools/r8/verticalclassmerging/InvokeSuperExtractor.java b/src/main/java/com/android/tools/r8/verticalclassmerging/InvokeSuperExtractor.java
new file mode 100644
index 0000000..b0770ee
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/verticalclassmerging/InvokeSuperExtractor.java
@@ -0,0 +1,69 @@
+// Copyright (c) 2024, 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.verticalclassmerging;
+
+import com.android.tools.r8.graph.AppView;
+import com.android.tools.r8.graph.DefaultUseRegistry;
+import com.android.tools.r8.graph.DexClassAndMethod;
+import com.android.tools.r8.graph.DexMethod;
+import com.android.tools.r8.graph.DexProgramClass;
+import com.android.tools.r8.graph.MethodResolutionResult;
+import com.android.tools.r8.graph.ProgramMethod;
+import com.android.tools.r8.graph.lens.GraphLens;
+import com.android.tools.r8.graph.lens.MethodLookupResult;
+import com.android.tools.r8.shaking.AppInfoWithLiveness;
+import com.android.tools.r8.utils.collections.DexMethodSignatureSet;
+
+public class InvokeSuperExtractor extends DefaultUseRegistry<ProgramMethod> {
+
+ private final AppView<AppInfoWithLiveness> appViewWithLiveness;
+ private final GraphLens graphLens;
+ private final GraphLens codeLens;
+ private final DexMethodSignatureSet methodsOfInterest;
+ private final DexMethodSignatureSet result;
+ private final DexProgramClass source;
+
+ public InvokeSuperExtractor(
+ AppView<AppInfoWithLiveness> appView,
+ ProgramMethod context,
+ DexMethodSignatureSet methodsOfInterest,
+ DexMethodSignatureSet result,
+ DexProgramClass source) {
+ super(appView, context);
+ this.appViewWithLiveness = appView;
+ this.graphLens = appView.graphLens();
+ this.codeLens = context.getDefinition().getCode().getCodeLens(appView);
+ this.methodsOfInterest = methodsOfInterest;
+ this.result = result;
+ this.source = source;
+ }
+
+ // TODO(b/323022702): This should not need to be overridden, but we sometimes (correctly) map an
+ // invoke-special to invoke-direct, but then later decides to map the same invoke-special to
+ // invoke-super.
+ @Override
+ public void registerInvokeSpecial(DexMethod method) {
+ handleInvokeSpecial(method);
+ }
+
+ @Override
+ public void registerInvokeSuper(DexMethod method) {
+ handleInvokeSpecial(method);
+ }
+
+ private void handleInvokeSpecial(DexMethod method) {
+ MethodLookupResult lookupResult = graphLens.lookupInvokeSuper(method, getContext(), codeLens);
+ DexMethod rewrittenMethod = lookupResult.getReference();
+ if (!methodsOfInterest.contains(rewrittenMethod) || result.contains(rewrittenMethod)) {
+ return;
+ }
+ MethodResolutionResult currentResolutionResult =
+ appViewWithLiveness.appInfo().unsafeResolveMethodDueToDexFormat(rewrittenMethod);
+ DexClassAndMethod superTarget =
+ currentResolutionResult.lookupInvokeSuperTarget(getContext(), appViewWithLiveness);
+ if (superTarget != null && superTarget.getHolder() == source) {
+ result.add(superTarget);
+ }
+ }
+}
diff --git a/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMerger.java b/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMerger.java
index 8ef8743..64d6556 100644
--- a/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMerger.java
@@ -28,7 +28,6 @@
import com.android.tools.r8.utils.ThreadUtils;
import com.android.tools.r8.utils.Timing;
import com.android.tools.r8.utils.Timing.TimingMerger;
-import com.google.common.collect.Sets;
import com.google.common.collect.Streams;
import java.util.ArrayList;
import java.util.Collection;
@@ -130,8 +129,7 @@
// can't build IR.
rewriteCodeWithLens(executorService, timing);
- // Remove force inlined constructors.
- removeFullyInlinedInstanceInitializers(executorService, timing);
+ // Remove merged classes from app now that the code is fully rewritten.
removeMergedClasses(verticalClassMergerResult.getVerticallyMergedClasses(), timing);
// Convert the (incomplete) synthesized bridges to CF or LIR.
@@ -278,38 +276,6 @@
timing.end();
}
- private void removeFullyInlinedInstanceInitializers(
- ExecutorService executorService, Timing timing) throws ExecutionException {
- if (mode.isInitial()) {
- return;
- }
- timing.begin("Remove fully inlined instance initializers");
- PrunedItems.Builder prunedItemsBuilder =
- PrunedItems.concurrentBuilder().setPrunedApp(appView.app());
- ThreadUtils.<DexProgramClass, Exception>processItems(
- consumer -> {
- for (DexProgramClass clazz : appView.appInfo().classes()) {
- if (!clazz.isInterface()) {
- consumer.accept(clazz);
- }
- }
- },
- clazz -> {
- Set<DexEncodedMethod> methodsToRemove = Sets.newIdentityHashSet();
- // TODO(b/321171043): Inline and remove instance initializers that are only called from
- // other instance initializers in the same class.
- clazz.getMethodCollection().removeMethods(methodsToRemove);
- methodsToRemove.forEach(
- removedMethod -> prunedItemsBuilder.addRemovedMethod(removedMethod.getReference()));
- },
- options.getThreadingModule(),
- executorService);
- PrunedItems prunedItems = prunedItemsBuilder.build();
- appView.pruneItems(prunedItems, executorService, Timing.empty());
- appView.appInfo().getMethodAccessInfoCollection().withoutPrunedItems(prunedItems);
- timing.end();
- }
-
private void removeMergedClasses(VerticallyMergedClasses verticallyMergedClasses, Timing timing) {
if (mode.isInitial()) {
return;
diff --git a/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerGraphLens.java b/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerGraphLens.java
index ead346d..783b3d2 100644
--- a/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerGraphLens.java
+++ b/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerGraphLens.java
@@ -513,7 +513,6 @@
}
if (implementation == null) {
- assert from.isAbstract();
return;
}
diff --git a/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerOptions.java b/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerOptions.java
index dbe28ca..55b3b41 100644
--- a/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerOptions.java
+++ b/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerOptions.java
@@ -12,6 +12,7 @@
private boolean enabled = true;
private boolean enableInitial = true;
+ private boolean enableBridgeAnalysis = true;
public VerticalClassMergerOptions(InternalOptions options) {
this.options = options;
@@ -35,7 +36,15 @@
return true;
}
+ public boolean isBridgeAnalysisEnabled() {
+ return enableBridgeAnalysis;
+ }
+
public void setEnabled(boolean enabled) {
this.enabled = enabled;
}
+
+ public void setEnableBridgeAnalysis(boolean enableBridgeAnalysis) {
+ this.enableBridgeAnalysis = enableBridgeAnalysis;
+ }
}
diff --git a/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergerTest.java b/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergerTest.java
index 50f9638..c5c75a8 100644
--- a/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergerTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergerTest.java
@@ -270,7 +270,10 @@
testForR8(parameters.getBackend())
.addKeepRules(getProguardConfig(EXAMPLE_KEEP))
.addOptionsModification(this::configure)
- .addOptionsModification(InlinerOptions::setOnlyForceInlining)
+ .addOptionsModification(
+ options ->
+ options.getVerticalClassMergerOptions().setEnableBridgeAnalysis(false))
+ .addOptionsModification(InlinerOptions::disableInlining)
.allowUnusedProguardConfigurationRules(),
main,
readProgramFiles(programFiles),
diff --git a/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergingWithMissingTypeArgsSubstitutionTest.java b/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergingWithMissingTypeArgsSubstitutionTest.java
index de57198..d79b35a 100644
--- a/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergingWithMissingTypeArgsSubstitutionTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergingWithMissingTypeArgsSubstitutionTest.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.classmerging.vertical;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentAndRenamed;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
@@ -13,10 +14,11 @@
import com.android.tools.r8.NoMethodStaticizing;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.BooleanUtils;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import java.lang.reflect.TypeVariable;
+import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -27,11 +29,15 @@
public class VerticalClassMergingWithMissingTypeArgsSubstitutionTest extends TestBase {
@Parameter(0)
+ public boolean enableBridgeAnalysis;
+
+ @Parameter(1)
public TestParameters parameters;
- @Parameters(name = "{0}")
- public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimesAndApiLevels().build();
+ @Parameters(name = "{1}, bridge analysis: {0}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ BooleanUtils.values(), getTestParameters().withAllRuntimesAndApiLevels().build());
}
@Test
@@ -41,8 +47,13 @@
.addKeepMainRule(Main.class)
.addKeepClassRules(A.class)
.addKeepAttributeSignature()
+ .addOptionsModification(
+ options ->
+ options
+ .getVerticalClassMergerOptions()
+ .setEnableBridgeAnalysis(enableBridgeAnalysis))
.addVerticallyMergedClassesInspector(
- inspector -> inspector.assertMergedIntoSubtype(B.class))
+ inspector -> inspector.assertMergedIntoSubtype(B.class).assertNoOtherClassesMerged())
.enableInliningAnnotations()
.enableNoMethodStaticizingAnnotations()
.enableConstantArgumentAnnotations()
@@ -56,9 +67,11 @@
assertEquals(
"<T:Ljava/lang/Object;>L" + binaryName(A.class) + "<Ljava/lang/Object;>;",
classSubject.getFinalSignatureAttribute());
+
MethodSubject bar = classSubject.uniqueMethodWithOriginalName("bar");
assertThat(bar, isPresentAndRenamed());
assertEquals("(TT;)V", bar.getFinalSignatureAttribute());
+
// The NeverInline is transferred to the private vertically merged method but also
// copied to the virtual bridge.
MethodSubject fooMovedFromB =
@@ -72,6 +85,7 @@
assertEquals(
"(Ljava/lang/Object;)Ljava/lang/Object;",
fooMovedFromB.getFinalSignatureAttribute());
+
MethodSubject fooBridge =
classSubject.uniqueMethodThatMatches(
method ->
@@ -79,9 +93,14 @@
&& method.isVirtual()
&& method.isSynthetic()
&& method.getOriginalName(false).equals("foo"));
- assertThat(fooBridge, isPresentAndRenamed());
- assertEquals(
- "(Ljava/lang/Object;)Ljava/lang/Object;", fooBridge.getFinalSignatureAttribute());
+ if (enableBridgeAnalysis) {
+ assertThat(fooBridge, isAbsent());
+ } else {
+ assertThat(fooBridge, isPresentAndRenamed());
+ assertEquals(
+ "(Ljava/lang/Object;)Ljava/lang/Object;",
+ fooBridge.getFinalSignatureAttribute());
+ }
});
}
diff --git a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterVerticalMergingMethodTest.java b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterVerticalMergingMethodTest.java
index 6271b08..52a8deb 100644
--- a/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterVerticalMergingMethodTest.java
+++ b/src/test/java/com/android/tools/r8/naming/applymapping/ApplyMappingAfterVerticalMergingMethodTest.java
@@ -113,14 +113,14 @@
assertThat(inspector.clazz(LibrarySubclass.class), isPresent());
List<FoundMethodSubject> methods =
inspector.clazz(LibrarySubclass.class).allMethods();
- assertEquals(backend.isCf() ? 5 : 4, methods.size());
+ assertEquals(backend.isCf() ? 4 : 3, methods.size());
assertEquals(
backend.isCf() ? 2 : 1,
methods.stream().filter(FoundMethodSubject::isInstanceInitializer).count());
assertEquals(
1, methods.stream().filter(m -> m.getFinalName().contains("main")).count());
assertEquals(
- 2, methods.stream().filter(m -> m.getOriginalName().contains("foo")).count());
+ 1, methods.stream().filter(m -> m.getOriginalName().contains("foo")).count());
});
}
diff --git a/src/test/java/com/android/tools/r8/naming/retrace/VerticalClassMergingRetraceTest.java b/src/test/java/com/android/tools/r8/naming/retrace/VerticalClassMergingRetraceTest.java
index 2fd2c50..cb94b5d 100644
--- a/src/test/java/com/android/tools/r8/naming/retrace/VerticalClassMergingRetraceTest.java
+++ b/src/test/java/com/android/tools/r8/naming/retrace/VerticalClassMergingRetraceTest.java
@@ -27,22 +27,34 @@
@RunWith(Parameterized.class)
public class VerticalClassMergingRetraceTest extends RetraceTestBase {
- @Parameters(name = "{0}, mode: {1}, compat: {2}")
+ @Parameters(name = "{0}, mode: {1}, compat: {2}, bridge analysis: {3}")
public static Collection<Object[]> data() {
return buildParameters(
getTestParameters().withAllRuntimesAndApiLevels().build(),
CompilationMode.values(),
+ BooleanUtils.values(),
BooleanUtils.values());
}
+ private final boolean enableBridgeAnalysis;
+
public VerticalClassMergingRetraceTest(
- TestParameters parameters, CompilationMode mode, boolean compat) {
+ TestParameters parameters,
+ CompilationMode mode,
+ boolean compat,
+ boolean enableBridgeAnalysis) {
super(parameters, mode, compat);
+ this.enableBridgeAnalysis = enableBridgeAnalysis;
}
@Override
- public void configure(R8TestBuilder builder) {
+ public void configure(R8TestBuilder<?> builder) {
builder
+ .addOptionsModification(
+ options ->
+ options
+ .getVerticalClassMergerOptions()
+ .setEnableBridgeAnalysis(enableBridgeAnalysis))
.enableInliningAnnotations()
.enableKeepUnusedReturnValueAnnotations()
.enableNeverClassInliningAnnotations();
@@ -59,8 +71,9 @@
}
private int expectedActualStackTraceHeight() {
- // In RELEASE mode, a synthetic bridge will be added by vertical class merger.
- return mode == CompilationMode.RELEASE ? 3 : 2;
+ // In RELEASE mode a synthetic bridge is added by the vertical class merger if the method is
+ // targeted by the invoke-super (which is modeled by setting enableBridgeAnalysis to false).
+ return mode == CompilationMode.DEBUG || enableBridgeAnalysis ? 2 : 3;
}
private boolean filterSynthesizedMethodWhenLineNumberAvailable(
diff --git a/src/test/java/com/android/tools/r8/profile/art/completeness/VerticalClassMergingBridgeProfileRewritingTest.java b/src/test/java/com/android/tools/r8/profile/art/completeness/VerticalClassMergingBridgeProfileRewritingTest.java
index 56ea0b9..4493de9 100644
--- a/src/test/java/com/android/tools/r8/profile/art/completeness/VerticalClassMergingBridgeProfileRewritingTest.java
+++ b/src/test/java/com/android/tools/r8/profile/art/completeness/VerticalClassMergingBridgeProfileRewritingTest.java
@@ -41,9 +41,12 @@
.addInnerClasses(getClass())
.addKeepMainRule(Main.class)
.addArtProfileForRewriting(getArtProfile())
- .addOptionsModification(InlinerOptions::setOnlyForceInlining)
+ .addOptionsModification(InlinerOptions::disableInlining)
.addOptionsModification(
- options -> options.callSiteOptimizationOptions().disableOptimization())
+ options -> {
+ options.callSiteOptimizationOptions().disableOptimization();
+ options.getVerticalClassMergerOptions().setEnableBridgeAnalysis(false);
+ })
.addVerticallyMergedClassesInspector(
inspector -> inspector.assertMergedIntoSubtype(A.class))
.enableNeverClassInliningAnnotations()
