Revert "Adjust accessibility for desugared lambdas prior to IR processing in R8"
This reverts commit 8330106ac3dfaf98aeea2730278613b2a915af6f.
Revert "Remove special handling for desugared lambdas in class inliner"
This reverts commit 90993db1b0b6b791959b6f5ad802bf7f075a6990.
Reason for revert: R8CompiledThroughDexTest.testR8CompiledWithR8Dex fails.
Change-Id: Ia26cf13262c532b7debdb05a4614d08a5fb94bfc
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
index 541bb0e..dbd455b 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
@@ -796,6 +796,8 @@
assert (dexCode.getDebugInfo() == null)
|| (arity == dexCode.getDebugInfo().parameters.length);
} else {
+ assert appView.options().isDesugaredLibraryCompilation()
+ || appView.options().enableCfInterfaceMethodDesugaring;
assert code.isCfCode();
CfCode cfCode = code.asCfCode();
cfCode.addFakeThisParameter(appView.dexItemFactory());
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 35f7041..b3a6d66 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -274,7 +274,9 @@
assert appView.rootSet() != null;
AppView<AppInfoWithLiveness> appViewWithLiveness = appView.withLiveness();
this.classInliner =
- options.enableClassInlining && options.enableInlining ? new ClassInliner() : null;
+ options.enableClassInlining && options.enableInlining
+ ? new ClassInliner(lambdaRewriter)
+ : null;
this.classStaticizer =
options.enableClassStaticizer ? new ClassStaticizer(appViewWithLiveness, this) : null;
this.dynamicTypeOptimization =
@@ -400,11 +402,8 @@
private void synthesizeLambdaClasses(Builder<?> builder, ExecutorService executorService)
throws ExecutionException {
if (lambdaRewriter != null) {
- if (appView.enableWholeProgramOptimizations()) {
- lambdaRewriter.finalizeLambdaDesugaringForR8(builder);
- } else {
- lambdaRewriter.finalizeLambdaDesugaringForD8(builder, executorService);
- }
+ lambdaRewriter.adjustAccessibility();
+ lambdaRewriter.synthesizeLambdaClasses(builder, executorService);
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/LambdaClass.java b/src/main/java/com/android/tools/r8/ir/desugar/LambdaClass.java
index 8169472..ce77849 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/LambdaClass.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/LambdaClass.java
@@ -484,7 +484,7 @@
}
// Ensure access of the referenced symbol(s).
- abstract DexEncodedMethod ensureAccessibility();
+ abstract void ensureAccessibility();
DexClass definitionFor(DexType type) {
return rewriter.converter.appView.appInfo().app().definitionFor(type);
@@ -527,9 +527,7 @@
}
@Override
- DexEncodedMethod ensureAccessibility() {
- return null;
- }
+ void ensureAccessibility() {}
}
// Used for static private lambda$ methods. Only needs access relaxation.
@@ -540,7 +538,7 @@
}
@Override
- DexEncodedMethod ensureAccessibility() {
+ void ensureAccessibility() {
// We already found the static method to be called, just relax its accessibility.
assert descriptor.getAccessibility() != null;
descriptor.getAccessibility().unsetPrivate();
@@ -548,7 +546,6 @@
if (implMethodHolder.isInterface()) {
descriptor.getAccessibility().setPublic();
}
- return null;
}
}
@@ -561,7 +558,7 @@
}
@Override
- DexEncodedMethod ensureAccessibility() {
+ void ensureAccessibility() {
// For all instantiation points for which the compiler creates lambda$
// methods, it creates these methods in the same class/interface.
DexMethod implMethod = descriptor.implHandle.asMethod();
@@ -592,13 +589,11 @@
DexEncodedMethod.setDebugInfoWithFakeThisParameter(
newMethod.getCode(), callTarget.getArity(), rewriter.converter.appView);
implMethodHolder.setDirectMethod(i, newMethod);
-
- return newMethod;
+ return;
}
}
assert false
: "Unexpected failure to find direct lambda target for: " + implMethod.qualifiedName();
- return null;
}
}
@@ -610,7 +605,7 @@
}
@Override
- DexEncodedMethod ensureAccessibility() {
+ void ensureAccessibility() {
// For all instantiation points for which the compiler creates lambda$
// methods, it creates these methods in the same class/interface.
DexMethod implMethod = descriptor.implHandle.asMethod();
@@ -637,11 +632,9 @@
// Move the method from the direct methods to the virtual methods set.
implMethodHolder.removeDirectMethod(i);
implMethodHolder.appendVirtualMethod(newMethod);
-
- return newMethod;
+ return;
}
}
- return null;
}
}
@@ -654,7 +647,7 @@
}
@Override
- DexEncodedMethod ensureAccessibility() {
+ void ensureAccessibility() {
// Create a static accessor with proper accessibility.
DexProgramClass accessorClass = programDefinitionFor(callTarget.holder);
assert accessorClass != null;
@@ -680,7 +673,7 @@
accessorClass.appendDirectMethod(accessorEncodedMethod);
}
- return accessorEncodedMethod;
+ rewriter.converter.optimizeSynthesizedMethod(accessorEncodedMethod);
}
}
}
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java b/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
index 216a9d7..ed49900 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
@@ -112,28 +112,19 @@
OptimizationFeedbackDelayed feedback,
LensCodeRewriter lensCodeRewriter)
throws ExecutionException {
- Set<LambdaClass> synthesizedLambdaClasses = Sets.newIdentityHashSet();
- Set<DexProgramClass> synthesizedLambdaProgramClasses = Sets.newIdentityHashSet();
+ Set<DexProgramClass> synthesizedLambdaClasses = Sets.newIdentityHashSet();
for (DexEncodedMethod method : wave) {
- synthesizeLambdaClassesForMethod(
- method,
- lambdaClass -> {
- synthesizedLambdaClasses.add(lambdaClass);
- synthesizedLambdaProgramClasses.add(lambdaClass.getOrCreateLambdaClass());
- },
- lensCodeRewriter);
+ synthesizeLambdaClassesForMethod(method, synthesizedLambdaClasses::add, lensCodeRewriter);
}
if (synthesizedLambdaClasses.isEmpty()) {
return;
}
- synthesizeAccessibilityBridgesForLambdaClasses(synthesizedLambdaClasses, executorService);
-
// Record that the static fields on each lambda class are only written inside the static
// initializer of the lambdas.
Map<DexEncodedField, Set<DexEncodedMethod>> writesWithContexts = new IdentityHashMap<>();
- for (DexProgramClass synthesizedLambdaClass : synthesizedLambdaProgramClasses) {
+ for (DexProgramClass synthesizedLambdaClass : synthesizedLambdaClasses) {
DexEncodedMethod clinit = synthesizedLambdaClass.getClassInitializer();
if (clinit != null) {
for (DexEncodedField field : synthesizedLambdaClass.staticFields()) {
@@ -146,28 +137,14 @@
appViewWithLiveness.setAppInfo(
appViewWithLiveness.appInfo().withStaticFieldWrites(writesWithContexts));
- converter.optimizeSynthesizedLambdaClasses(synthesizedLambdaProgramClasses, executorService);
+ converter.optimizeSynthesizedLambdaClasses(synthesizedLambdaClasses, executorService);
feedback.updateVisibleOptimizationInfo();
}
- private void synthesizeAccessibilityBridgesForLambdaClasses(
- Collection<LambdaClass> lambdaClasses, ExecutorService executorService)
- throws ExecutionException {
- Set<DexEncodedMethod> nonDexAccessibilityBridges = Sets.newIdentityHashSet();
- for (LambdaClass lambdaClass : lambdaClasses) {
- // This call may cause methodMapping to be updated.
- DexEncodedMethod accessibilityBridge = lambdaClass.target.ensureAccessibility();
- if (accessibilityBridge != null && !accessibilityBridge.getCode().isDexCode()) {
- nonDexAccessibilityBridges.add(accessibilityBridge);
- }
- }
- if (!nonDexAccessibilityBridges.isEmpty()) {
- converter.processMethodsConcurrently(nonDexAccessibilityBridges, executorService);
- }
- }
-
public void synthesizeLambdaClassesForMethod(
- DexEncodedMethod method, Consumer<LambdaClass> consumer, LensCodeRewriter lensCodeRewriter) {
+ DexEncodedMethod method,
+ Consumer<DexProgramClass> consumer,
+ LensCodeRewriter lensCodeRewriter) {
if (!method.hasCode() || method.isProcessed()) {
// Nothing to desugar.
return;
@@ -190,7 +167,9 @@
LambdaDescriptor descriptor =
inferLambdaDescriptor(lensCodeRewriter.rewriteCallSite(callSite, method));
if (descriptor != LambdaDescriptor.MATCH_FAILED) {
- consumer.accept(getOrCreateLambdaClass(descriptor, method.method.holder));
+ consumer.accept(
+ getOrCreateLambdaClass(descriptor, method.method.holder)
+ .getOrCreateLambdaClass());
}
}
});
@@ -262,10 +241,33 @@
return false;
}
+ /** Adjust accessibility of referenced application symbols or creates necessary accessors. */
+ public void adjustAccessibility() {
+ // For each lambda class perform necessary adjustment of the
+ // referenced symbols to make them accessible. This can result in
+ // method access relaxation or creation of accessor method.
+ for (LambdaClass lambdaClass : knownLambdaClasses.values()) {
+ // This call may cause methodMapping to be updated.
+ lambdaClass.target.ensureAccessibility();
+ }
+ if (appView.enableWholeProgramOptimizations() && !methodMapping.isEmpty()) {
+ appView.setGraphLense(
+ new LambdaRewriterGraphLense(methodMapping, appView.graphLense(), factory));
+ }
+ }
+
+ /**
+ * Returns a synthetic class for desugared lambda or `null` if the `type` does not represent one.
+ * Method can be called concurrently.
+ */
+ public DexProgramClass getLambdaClass(DexType type) {
+ LambdaClass lambdaClass = getKnown(knownLambdaClasses, type);
+ return lambdaClass == null ? null : lambdaClass.getOrCreateLambdaClass();
+ }
+
/** Generates lambda classes and adds them to the builder. */
- public void finalizeLambdaDesugaringForD8(Builder<?> builder, ExecutorService executorService)
+ public void synthesizeLambdaClasses(Builder<?> builder, ExecutorService executorService)
throws ExecutionException {
- synthesizeAccessibilityBridgesForLambdaClasses(knownLambdaClasses.values(), executorService);
AppInfo appInfo = appView.appInfo();
for (LambdaClass lambdaClass : knownLambdaClasses.values()) {
DexProgramClass synthesizedClass = lambdaClass.getOrCreateLambdaClass();
@@ -279,20 +281,6 @@
executorService);
}
- /** Generates lambda classes and adds them to the builder. */
- public void finalizeLambdaDesugaringForR8(Builder<?> builder) {
- if (!methodMapping.isEmpty()) {
- appView.setGraphLense(
- new LambdaRewriterGraphLense(methodMapping, appView.graphLense(), factory));
- }
- AppInfo appInfo = appView.appInfo();
- for (LambdaClass lambdaClass : knownLambdaClasses.values()) {
- DexProgramClass synthesizedClass = lambdaClass.getOrCreateLambdaClass();
- appInfo.addSynthesizedClass(synthesizedClass);
- builder.addSynthesizedClass(synthesizedClass, lambdaClass.addToMainDexList.get());
- }
- }
-
public Set<DexCallSite> getDesugaredCallSites() {
synchronized (knownCallSites) {
return knownCallSites.keySet();
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
index 538cba6..36fd914 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
@@ -13,6 +13,7 @@
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.InstructionOrPhi;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.ir.desugar.LambdaRewriter;
import com.android.tools.r8.ir.optimize.CodeRewriter;
import com.android.tools.r8.ir.optimize.Inliner;
import com.android.tools.r8.ir.optimize.InliningOracle;
@@ -59,9 +60,14 @@
ELIGIBLE
}
+ private final LambdaRewriter lambdaRewriter;
private final ConcurrentHashMap<DexClass, EligibilityStatus> knownClasses =
new ConcurrentHashMap<>();
+ public ClassInliner(LambdaRewriter lambdaRewriter) {
+ this.lambdaRewriter = lambdaRewriter;
+ }
+
private void logEligibilityStatus(
DexEncodedMethod context, Instruction root, EligibilityStatus status) {
if (Log.ENABLED && Log.isLoggingEnabledFor(ClassInliner.class)) {
@@ -186,6 +192,7 @@
InlineCandidateProcessor processor =
new InlineCandidateProcessor(
appView,
+ lambdaRewriter,
inliner,
clazz -> isClassEligible(appView, clazz),
isProcessedConcurrently,
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
index 35e84ef..c522ff3 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
@@ -35,6 +35,7 @@
import com.android.tools.r8.ir.code.InvokeMethodWithReceiver;
import com.android.tools.r8.ir.code.StaticGet;
import com.android.tools.r8.ir.code.Value;
+import com.android.tools.r8.ir.desugar.LambdaRewriter;
import com.android.tools.r8.ir.optimize.Inliner;
import com.android.tools.r8.ir.optimize.Inliner.InlineAction;
import com.android.tools.r8.ir.optimize.Inliner.InliningInfo;
@@ -77,6 +78,7 @@
ImmutableSet.of(If.Type.EQ, If.Type.NE);
private final AppView<AppInfoWithLiveness> appView;
+ private final LambdaRewriter lambdaRewriter;
private final Inliner inliner;
private final Function<DexClass, EligibilityStatus> isClassEligible;
private final Predicate<DexEncodedMethod> isProcessedConcurrently;
@@ -86,6 +88,7 @@
private Value eligibleInstance;
private DexType eligibleClass;
private DexProgramClass eligibleClassDefinition;
+ private boolean isDesugaredLambda;
private final Map<InvokeMethodWithReceiver, InliningInfo> methodCallsOnInstance =
new IdentityHashMap<>();
@@ -103,12 +106,14 @@
InlineCandidateProcessor(
AppView<AppInfoWithLiveness> appView,
+ LambdaRewriter lambdaRewriter,
Inliner inliner,
Function<DexClass, EligibilityStatus> isClassEligible,
Predicate<DexEncodedMethod> isProcessedConcurrently,
DexEncodedMethod method,
Instruction root) {
this.appView = appView;
+ this.lambdaRewriter = lambdaRewriter;
this.inliner = inliner;
this.isClassEligible = isClassEligible;
this.method = method;
@@ -160,6 +165,11 @@
if (!eligibleClass.isClassType()) {
return EligibilityStatus.NON_CLASS_TYPE;
}
+ if (lambdaRewriter != null) {
+ // Check if the class is synthesized for a desugared lambda
+ eligibleClassDefinition = lambdaRewriter.getLambdaClass(eligibleClass);
+ isDesugaredLambda = eligibleClassDefinition != null;
+ }
if (eligibleClassDefinition == null) {
eligibleClassDefinition = asProgramClassOrNull(appView.definitionFor(eligibleClass));
}
@@ -203,6 +213,65 @@
}
assert root.isStaticGet();
+
+ // We know that desugared lambda classes satisfy eligibility requirements.
+ if (isDesugaredLambda) {
+ return EligibilityStatus.ELIGIBLE;
+ }
+
+ // Checking if we can safely inline class implemented following singleton-like
+ // pattern, by which we assume a static final field holding on to the reference
+ // initialized in class constructor.
+ //
+ // In general we are targeting cases when the class is defined as:
+ //
+ // class X {
+ // static final X F;
+ // static {
+ // F = new X();
+ // }
+ // }
+ //
+ // and being used as follows:
+ //
+ // void foo() {
+ // f = X.F;
+ // f.bar();
+ // }
+ //
+ // The main difference from the similar case of class inliner with 'new-instance'
+ // instruction is that in this case the instance we inline is not just leaked, but
+ // is actually published via X.F field. There are several risks we need to address
+ // in this case:
+ //
+ // Risk: instance stored in field X.F has changed after it was initialized in
+ // class initializer
+ // Solution: we assume that final field X.F is not modified outside the class
+ // initializer. In rare cases when it is (e.g. via reflections) it should
+ // be marked with keep rules
+ //
+ // Risk: instance stored in field X.F is not initialized yet
+ // Solution: not initialized instance can only be visible if X.<clinit>
+ // triggers other class initialization which references X.F. This
+ // situation should never happen if we:
+ // -- don't allow any superclasses to have static initializer,
+ // -- don't allow any subclasses,
+ // -- guarantee the class has trivial class initializer
+ // (see CodeRewriter::computeClassInitializerInfo), and
+ // -- guarantee the instance is initialized with trivial instance
+ // initializer (see CodeRewriter::computeInstanceInitializerInfo)
+ //
+ // Risk: instance stored in field X.F was mutated
+ // Solution: we require that class X does not have any instance fields, and
+ // if any of its superclasses has instance fields, accessing them will make
+ // this instance not eligible for inlining. I.e. even though the instance is
+ // publicized and its state has been mutated, it will not effect the logic
+ // of class inlining
+ //
+
+ if (!eligibleClassDefinition.instanceFields().isEmpty()) {
+ return EligibilityStatus.HAS_INSTANCE_FIELDS;
+ }
return EligibilityStatus.ELIGIBLE;
}
@@ -236,24 +305,10 @@
indirectUsers.addAll(alias.uniqueUsers());
continue;
}
-
- if (user.isInstanceGet()) {
- if (root.isStaticGet()) {
- // We don't have a replacement for this field read.
- return user; // Not eligible.
- }
- DexEncodedField field =
- appView.appInfo().resolveField(user.asFieldInstruction().getField());
- if (field == null || field.isStatic()) {
- return user; // Not eligible.
- }
- continue;
- }
-
- if (user.isInstancePut()) {
- if (!receivers.addIllegalReceiverAlias(user.asInstancePut().value())) {
- return user; // Not eligible.
- }
+ // Field read/write.
+ if (user.isInstanceGet()
+ || (user.isInstancePut()
+ && receivers.addIllegalReceiverAlias(user.asInstancePut().value()))) {
DexEncodedField field =
appView.appInfo().resolveField(user.asFieldInstruction().getField());
if (field == null || field.isStatic()) {
@@ -652,6 +707,12 @@
return null;
}
+ if (isDesugaredLambda) {
+ // Lambda desugaring synthesizes eligible constructors.
+ markSizeForInlining(invoke, singleTarget);
+ return new InliningInfo(singleTarget, eligibleClass);
+ }
+
// Check that the entire constructor chain can be inlined into the current context.
DexItemFactory dexItemFactory = appView.dexItemFactory();
DexMethod parent = instanceInitializerInfo.getParent();
@@ -817,15 +878,6 @@
return null;
}
- if (root.isStaticGet()) {
- // If we are class inlining a singleton instance from a static-get, then we don't the value of
- // the fields.
- ParameterUsage receiverUsage = optimizationInfo.getParameterUsages(0);
- if (receiverUsage == null || receiverUsage.hasFieldRead) {
- return null;
- }
- }
-
// If the method returns receiver and the return value is actually
// used in the code we need to make some additional checks.
if (!eligibilityAcceptanceCheck.test(eligibility)) {
@@ -961,14 +1013,6 @@
return false;
}
- if (root.isStaticGet()) {
- // If we are class inlining a singleton instance from a static-get, then we don't the value of
- // the fields.
- if (parameterUsage.hasFieldRead) {
- return false;
- }
- }
-
if (parameterUsage.isReturned) {
if (invoke.outValue() != null && invoke.outValue().hasAnyUsers()) {
// Used as return value which is not ignored.
@@ -1036,6 +1080,12 @@
private boolean exemptFromInstructionLimit(DexEncodedMethod inlinee) {
DexType inlineeHolder = inlinee.method.holder;
+ if (isDesugaredLambda && inlineeHolder == eligibleClass) {
+ return true;
+ }
+ if (appView.appInfo().isPinned(inlineeHolder)) {
+ return false;
+ }
DexClass inlineeClass = appView.definitionFor(inlineeHolder);
assert inlineeClass != null;
@@ -1066,6 +1116,15 @@
if (isProcessedConcurrently.test(singleTarget)) {
return false;
}
+ if (isDesugaredLambda && !singleTarget.accessFlags.isBridge()) {
+ // OK if this is the call to the main method of a desugared lambda (for both direct and
+ // indirect calls).
+ //
+ // Note: This is needed because lambda methods are generally processed in the same batch as
+ // they are class inlined, which means that the call to isInliningCandidate() below will
+ // return false.
+ return true;
+ }
if (!singleTarget.isInliningCandidate(
method, Reason.SIMPLE, appView.appInfo(), NopWhyAreYouNotInliningReporter.getInstance())) {
// If `singleTarget` is not an inlining candidate, we won't be able to inline it here.
diff --git a/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java b/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java
index 92d02ee..8154dfc 100644
--- a/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java
+++ b/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java
@@ -174,7 +174,7 @@
.withBuilderTransformation(ToolHelper::allowTestProguardOptions)
.withBuilderTransformation(
b -> b.addProguardConfiguration(PROGUARD_OPTIONS_N_PLUS, Origin.unknown()))
- .withDexCheck(inspector -> checkLambdaCount(inspector, 7, "lambdadesugaringnplus"))
+ .withDexCheck(inspector -> checkLambdaCount(inspector, 5, "lambdadesugaringnplus"))
.run();
}
