Version 2.1.29 Cherry pick: Fix bug in proto builder optimization CL: https://r8-review.googlesource.com/c/r8/+/51692 Cherry pick: Reproduce proto builder optimization bug CL: https://r8-review.googlesource.com/c/r8/+/51702 Bug: 155416893 Change-Id: I8fbf0a3224d32e65bfd2da7010a130e4ad5cdc04
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java index 8406706..622224b 100644 --- a/src/main/java/com/android/tools/r8/R8.java +++ b/src/main/java/com/android/tools/r8/R8.java
@@ -697,7 +697,7 @@ appView.withGeneratedMessageLiteBuilderShrinker( shrinker -> - shrinker.removeDeadBuilderReferencesFromDynamicMethods( + shrinker.rewriteDeadBuilderReferencesFromDynamicMethods( appViewWithLiveness, executorService, timing)); if (options.isShrinking()) { @@ -914,6 +914,10 @@ shrinker -> shrinker.setDeadProtoTypes(appViewWithLiveness.appInfo().getDeadProtoTypes())); } + appView.withGeneratedMessageLiteBuilderShrinker( + shrinker -> + shrinker.rewriteDeadBuilderReferencesFromDynamicMethods( + appViewWithLiveness, executorService, timing)); return appViewWithLiveness; }
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java index 283f647..47f1b12 100644 --- a/src/main/java/com/android/tools/r8/Version.java +++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@ // This field is accessed from release scripts using simple pattern matching. // Therefore, changing this field could break our release scripts. - public static final String LABEL = "2.1.28"; + public static final String LABEL = "2.1.29"; private Version() { }
diff --git a/src/main/java/com/android/tools/r8/graph/ClassAccessFlags.java b/src/main/java/com/android/tools/r8/graph/ClassAccessFlags.java index 030497d..e33792c 100644 --- a/src/main/java/com/android/tools/r8/graph/ClassAccessFlags.java +++ b/src/main/java/com/android/tools/r8/graph/ClassAccessFlags.java
@@ -167,6 +167,10 @@ return isSet(Constants.ACC_ABSTRACT); } + public void demoteFromAbstract() { + demote(Constants.ACC_ABSTRACT); + } + public void setAbstract() { set(Constants.ACC_ABSTRACT); }
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/fieldaccess/FieldAssignmentTracker.java b/src/main/java/com/android/tools/r8/ir/analysis/fieldaccess/FieldAssignmentTracker.java index fbb6443..1481061 100644 --- a/src/main/java/com/android/tools/r8/ir/analysis/fieldaccess/FieldAssignmentTracker.java +++ b/src/main/java/com/android/tools/r8/ir/analysis/fieldaccess/FieldAssignmentTracker.java
@@ -64,8 +64,13 @@ FieldAssignmentTracker(AppView<AppInfoWithLiveness> appView) { this.appView = appView; - this.fieldAccessGraph = new FieldAccessGraph(appView); - this.objectAllocationGraph = new ObjectAllocationGraph(appView); + this.fieldAccessGraph = new FieldAccessGraph(); + this.objectAllocationGraph = new ObjectAllocationGraph(); + } + + public void initialize() { + fieldAccessGraph.initialize(appView); + objectAllocationGraph.initialize(appView); initializeAbstractInstanceFieldValues(); } @@ -308,10 +313,11 @@ private final Reference2IntMap<DexEncodedField> pendingFieldWrites = new Reference2IntOpenHashMap<>(); - FieldAccessGraph(AppView<AppInfoWithLiveness> appView) { + FieldAccessGraph() {} + + public void initialize(AppView<AppInfoWithLiveness> appView) { FieldAccessInfoCollection<?> fieldAccessInfoCollection = appView.appInfo().getFieldAccessInfoCollection(); - fieldAccessInfoCollection.flattenAccessContexts(); fieldAccessInfoCollection.forEach( info -> { DexEncodedField field = @@ -356,7 +362,9 @@ private final Reference2IntMap<DexProgramClass> pendingObjectAllocations = new Reference2IntOpenHashMap<>(); - ObjectAllocationGraph(AppView<AppInfoWithLiveness> appView) { + ObjectAllocationGraph() {} + + public void initialize(AppView<AppInfoWithLiveness> appView) { ObjectAllocationInfoCollection objectAllocationInfos = appView.appInfo().getObjectAllocationInfoCollection(); objectAllocationInfos.forEachClassWithKnownAllocationSites(
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/proto/GeneratedMessageLiteBuilderShrinker.java b/src/main/java/com/android/tools/r8/ir/analysis/proto/GeneratedMessageLiteBuilderShrinker.java index deadfa0..c2cd2aa 100644 --- a/src/main/java/com/android/tools/r8/ir/analysis/proto/GeneratedMessageLiteBuilderShrinker.java +++ b/src/main/java/com/android/tools/r8/ir/analysis/proto/GeneratedMessageLiteBuilderShrinker.java
@@ -4,41 +4,48 @@ package com.android.tools.r8.ir.analysis.proto; +import static com.android.tools.r8.graph.DexProgramClass.asProgramClassOrNull; +import static com.android.tools.r8.ir.analysis.type.Nullability.definitelyNotNull; +import static com.android.tools.r8.ir.analysis.type.Nullability.maybeNull; + import com.android.tools.r8.graph.AppInfoWithClassHierarchy; import com.android.tools.r8.graph.AppView; import com.android.tools.r8.graph.DexEncodedMethod; +import com.android.tools.r8.graph.DexField; import com.android.tools.r8.graph.DexMethod; import com.android.tools.r8.graph.DexProgramClass; import com.android.tools.r8.graph.DexType; -import com.android.tools.r8.graph.EnumValueInfoMapCollection.EnumValueInfo; -import com.android.tools.r8.graph.EnumValueInfoMapCollection.EnumValueInfoMap; import com.android.tools.r8.graph.ProgramMethod; import com.android.tools.r8.graph.SubtypingInfo; +import com.android.tools.r8.graph.analysis.EnqueuerAnalysis; import com.android.tools.r8.ir.analysis.type.ClassTypeElement; import com.android.tools.r8.ir.analysis.type.TypeAnalysis; -import com.android.tools.r8.ir.analysis.type.TypeElement; import com.android.tools.r8.ir.code.CheckCast; import com.android.tools.r8.ir.code.IRCode; import com.android.tools.r8.ir.code.Instruction; import com.android.tools.r8.ir.code.InstructionListIterator; +import com.android.tools.r8.ir.code.InvokeDirect; import com.android.tools.r8.ir.code.InvokeVirtual; -import com.android.tools.r8.ir.code.Switch; +import com.android.tools.r8.ir.code.NewInstance; +import com.android.tools.r8.ir.code.StaticGet; import com.android.tools.r8.ir.code.Value; import com.android.tools.r8.ir.conversion.CallGraph.Node; import com.android.tools.r8.ir.conversion.IRConverter; import com.android.tools.r8.ir.conversion.MethodProcessor; -import com.android.tools.r8.ir.optimize.CodeRewriter; import com.android.tools.r8.ir.optimize.Inliner; import com.android.tools.r8.ir.optimize.Inliner.Reason; -import com.android.tools.r8.ir.optimize.controlflow.SwitchCaseAnalyzer; import com.android.tools.r8.ir.optimize.enums.EnumValueOptimizer; import com.android.tools.r8.ir.optimize.info.OptimizationFeedback; import com.android.tools.r8.ir.optimize.info.OptimizationFeedbackSimple; import com.android.tools.r8.ir.optimize.inliner.FixedInliningReasonStrategy; import com.android.tools.r8.shaking.AppInfoWithLiveness; +import com.android.tools.r8.shaking.Enqueuer; +import com.android.tools.r8.shaking.EnqueuerWorklist; +import com.android.tools.r8.utils.ObjectUtils; import com.android.tools.r8.utils.PredicateSet; import com.android.tools.r8.utils.ThreadUtils; import com.android.tools.r8.utils.Timing; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Sets; import java.util.ArrayList; import java.util.IdentityHashMap; @@ -53,6 +60,7 @@ private final AppView<? extends AppInfoWithClassHierarchy> appView; private final ProtoReferences references; + private final boolean enableAggressiveBuilderOptimization; private final Map<DexProgramClass, ProgramMethod> builders = new IdentityHashMap<>(); @@ -60,11 +68,98 @@ AppView<? extends AppInfoWithClassHierarchy> appView, ProtoReferences references) { this.appView = appView; this.references = references; + this.enableAggressiveBuilderOptimization = computeEnableAggressiveBuilderOptimization(); + // If this fails it is likely an unsupported version of the protobuf library. + assert enableAggressiveBuilderOptimization; + } + + private boolean computeEnableAggressiveBuilderOptimization() { + boolean unexpectedGeneratedMessageLiteBuilder = + ObjectUtils.getBooleanOrElse( + appView + .appInfo() + .definitionForWithoutExistenceAssert(references.generatedMessageLiteBuilderType), + clazz -> clazz.getMethodCollection().hasMethods(DexEncodedMethod::isAbstract), + true); + boolean unexpectedGeneratedMessageLiteExtendableBuilder = + ObjectUtils.getBooleanOrElse( + appView + .appInfo() + .definitionForWithoutExistenceAssert( + references.generatedMessageLiteExtendableBuilderType), + clazz -> clazz.getMethodCollection().hasMethods(DexEncodedMethod::isAbstract), + true); + if (unexpectedGeneratedMessageLiteBuilder) { + appView + .options() + .reporter + .warning( + "Unexpected implementation of `" + + references.generatedMessageLiteBuilderType.toSourceString() + + "`: disabling aggressive protobuf builder optimization."); + return false; + } + if (unexpectedGeneratedMessageLiteExtendableBuilder) { + appView + .options() + .reporter + .warning( + "Unexpected implementation of `" + + references.generatedMessageLiteExtendableBuilderType.toSourceString() + + "`: disabling aggressive protobuf builder optimization."); + return false; + } + return true; + } + + public EnqueuerAnalysis createEnqueuerAnalysis() { + Set<DexProgramClass> seen = Sets.newIdentityHashSet(); + return new EnqueuerAnalysis() { + @Override + public void notifyFixpoint(Enqueuer enqueuer, EnqueuerWorklist worklist, Timing timing) { + builders.forEach( + (builder, dynamicMethod) -> { + if (seen.add(builder)) { + // This builder class is never used in the program except from dynamicMethod(), + // which creates an instance of the builder. Instead of creating an instance of the + // builder class, we just instantiate the parent builder class. For this to work, + // we make the parent builder non-abstract. + DexProgramClass superClass = + asProgramClassOrNull(appView.definitionFor(builder.superType)); + assert superClass != null; + superClass.accessFlags.demoteFromAbstract(); + if (superClass.type == references.generatedMessageLiteBuilderType) { + // Manually trace `new GeneratedMessageLite.Builder(DEFAULT_INSTANCE)` since we + // haven't rewritten the code yet. + worklist.enqueueTraceNewInstanceAction( + references.generatedMessageLiteBuilderType, dynamicMethod); + worklist.enqueueTraceInvokeDirectAction( + references.generatedMessageLiteBuilderMethods.constructorMethod, + dynamicMethod); + } else { + assert superClass.type == references.generatedMessageLiteExtendableBuilderType; + // Manually trace `new GeneratedMessageLite.ExtendableBuilder(DEFAULT_INSTANCE)` + // since we haven't rewritten the code yet. + worklist.enqueueTraceNewInstanceAction( + references.generatedMessageLiteExtendableBuilderType, dynamicMethod); + worklist.enqueueTraceInvokeDirectAction( + references.generatedMessageLiteExtendableBuilderMethods.constructorMethod, + dynamicMethod); + } + worklist.enqueueTraceStaticFieldRead( + references.getDefaultInstanceField(dynamicMethod.getHolder()), dynamicMethod); + } + }); + } + }; } /** Returns true if an action was deferred. */ public boolean deferDeadProtoBuilders( DexProgramClass clazz, ProgramMethod method, BooleanSupplier register) { + if (!enableAggressiveBuilderOptimization) { + return false; + } DexEncodedMethod definition = method.getDefinition(); if (references.isDynamicMethod(definition) && references.isGeneratedMessageLiteBuilder(clazz)) { if (register.getAsBoolean()) { @@ -77,40 +172,79 @@ } /** - * Reprocesses each dynamicMethod() that references a dead builder to remove the dead builder + * Reprocesses each dynamicMethod() that references a dead builder to rewrite the dead builder * references. */ - public void removeDeadBuilderReferencesFromDynamicMethods( + public void rewriteDeadBuilderReferencesFromDynamicMethods( AppView<AppInfoWithLiveness> appView, ExecutorService executorService, Timing timing) throws ExecutionException { + if (builders.isEmpty()) { + return; + } timing.begin("Remove dead builder references"); AppInfoWithLiveness appInfo = appView.appInfo(); IRConverter converter = new IRConverter(appView, Timing.empty()); - CodeRewriter codeRewriter = new CodeRewriter(appView, converter); - MethodToInvokeSwitchCaseAnalyzer switchCaseAnalyzer = new MethodToInvokeSwitchCaseAnalyzer(); - if (switchCaseAnalyzer.isInitialized()) { - ThreadUtils.processItems( - builders.entrySet(), - entry -> { - if (!appInfo.isLiveProgramClass(entry.getKey())) { - removeDeadBuilderReferencesFromDynamicMethod( - appView, entry.getValue(), converter, codeRewriter, switchCaseAnalyzer); - } - }, - executorService); - } + ThreadUtils.processMap( + builders, + (builder, dynamicMethod) -> { + if (!appInfo.isLiveProgramClass(builder)) { + rewriteDeadBuilderReferencesFromDynamicMethod( + appView, builder, dynamicMethod, converter); + } + }, + executorService); builders.clear(); timing.end(); // Remove dead builder references } - private void removeDeadBuilderReferencesFromDynamicMethod( + private void rewriteDeadBuilderReferencesFromDynamicMethod( AppView<AppInfoWithLiveness> appView, + DexProgramClass builder, ProgramMethod dynamicMethod, - IRConverter converter, - CodeRewriter codeRewriter, - SwitchCaseAnalyzer switchCaseAnalyzer) { + IRConverter converter) { IRCode code = dynamicMethod.buildIR(appView); - codeRewriter.rewriteSwitch(code, switchCaseAnalyzer); + InstructionListIterator instructionIterator = code.instructionListIterator(); + + assert builder.superType == references.generatedMessageLiteBuilderType + || builder.superType == references.generatedMessageLiteExtendableBuilderType; + + DexField defaultInstanceField = references.getDefaultInstanceField(dynamicMethod.getHolder()); + Value builderValue = + code.createValue(ClassTypeElement.create(builder.superType, definitelyNotNull(), appView)); + Value defaultInstanceValue = + code.createValue(ClassTypeElement.create(defaultInstanceField.type, maybeNull(), appView)); + + // Replace `new Message.Builder()` by `new GeneratedMessageLite.Builder()` + // (or `new GeneratedMessageLite.ExtendableBuilder()`). + NewInstance newInstance = + instructionIterator.nextUntil( + instruction -> + instruction.isNewInstance() && instruction.asNewInstance().clazz == builder.type); + assert newInstance != null; + instructionIterator.replaceCurrentInstruction(new NewInstance(builder.superType, builderValue)); + + // Replace `builder.<init>()` by `builder.<init>(Message.DEFAULT_INSTANCE)`. + // + // We may also see an accessibility bridge constructor, because the Builder constructor is + // private. The accessibility bridge takes null as an argument. + InvokeDirect constructorInvoke = + instructionIterator.nextUntil( + instruction -> { + assert instruction.isInvokeDirect() || instruction.isConstNumber(); + return instruction.isInvokeDirect(); + }); + assert constructorInvoke != null; + instructionIterator.replaceCurrentInstruction( + new StaticGet(defaultInstanceValue, defaultInstanceField)); + instructionIterator.setInsertionPosition(constructorInvoke.getPosition()); + instructionIterator.add( + new InvokeDirect( + builder.superType == references.generatedMessageLiteBuilderType + ? references.generatedMessageLiteBuilderMethods.constructorMethod + : references.generatedMessageLiteExtendableBuilderMethods.constructorMethod, + null, + ImmutableList.of(builderValue, defaultInstanceValue))); + converter.removeDeadCodeAndFinalizeIR( dynamicMethod, code, OptimizationFeedbackSimple.getInstance(), Timing.empty()); } @@ -232,51 +366,6 @@ } } - private class MethodToInvokeSwitchCaseAnalyzer extends SwitchCaseAnalyzer { - - private final int newBuilderOrdinal; - - private MethodToInvokeSwitchCaseAnalyzer() { - EnumValueInfoMap enumValueInfoMap = - appView.appInfo().withLiveness().getEnumValueInfoMap(references.methodToInvokeType); - if (enumValueInfoMap != null) { - EnumValueInfo newBuilderValueInfo = - enumValueInfoMap.getEnumValueInfo(references.methodToInvokeMembers.newBuilderField); - if (newBuilderValueInfo != null) { - newBuilderOrdinal = newBuilderValueInfo.ordinal; - return; - } - } - newBuilderOrdinal = -1; - } - - public boolean isInitialized() { - return newBuilderOrdinal >= 0; - } - - @Override - public boolean switchCaseIsUnreachable(Switch theSwitch, int index) { - if (theSwitch.isStringSwitch()) { - assert false : "Unexpected string-switch instruction in dynamicMethod()"; - return false; - } - if (index != newBuilderOrdinal) { - return false; - } - Value switchValue = theSwitch.value(); - if (!switchValue.isDefinedByInstructionSatisfying(Instruction::isInvokeVirtual)) { - return false; - } - InvokeVirtual definition = switchValue.definition.asInvokeVirtual(); - if (definition.getInvokedMethod() != appView.dexItemFactory().enumMethods.ordinal) { - return false; - } - TypeElement enumType = definition.getReceiver().getType(); - return enumType.isClassType() - && enumType.asClassType().getClassType() == references.methodToInvokeType; - } - } - private static class RootSetExtension { private final AppView<? extends AppInfoWithClassHierarchy> appView;
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/proto/ProtoInliningReasonStrategy.java b/src/main/java/com/android/tools/r8/ir/analysis/proto/ProtoInliningReasonStrategy.java index ca4ca1e..e8770c7 100644 --- a/src/main/java/com/android/tools/r8/ir/analysis/proto/ProtoInliningReasonStrategy.java +++ b/src/main/java/com/android/tools/r8/ir/analysis/proto/ProtoInliningReasonStrategy.java
@@ -43,11 +43,19 @@ return Reason.ALWAYS; } return references.isDynamicMethod(target) || references.isDynamicMethodBridge(target) - ? computeInliningReasonForDynamicMethod(invoke) + ? computeInliningReasonForDynamicMethod(invoke, target, context) : parent.computeInliningReason(invoke, target, context); } - private Reason computeInliningReasonForDynamicMethod(InvokeMethod invoke) { + private Reason computeInliningReasonForDynamicMethod( + InvokeMethod invoke, ProgramMethod target, ProgramMethod context) { + // Do not allow inlining of dynamicMethod() into a proto library class. This should only happen + // if there is exactly one proto message in the program, since we would otherwise not be able + // to conclude a single target. + if (references.isDynamicMethod(target) && references.isProtoLibraryClass(context.getHolder())) { + return Reason.NEVER; + } + Value methodToInvokeValue = invoke .inValues()
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/proto/ProtoReferences.java b/src/main/java/com/android/tools/r8/ir/analysis/proto/ProtoReferences.java index 861c0cd..8fd43ee 100644 --- a/src/main/java/com/android/tools/r8/ir/analysis/proto/ProtoReferences.java +++ b/src/main/java/com/android/tools/r8/ir/analysis/proto/ProtoReferences.java
@@ -17,6 +17,8 @@ public class ProtoReferences { + private final DexItemFactory dexItemFactory; + public final DexType enumLiteMapType; public final DexType extendableMessageType; public final DexType extensionDescriptorType; @@ -25,6 +27,7 @@ public final DexType generatedMessageLiteType; public final DexType generatedMessageLiteBuilderType; public final DexType generatedMessageLiteExtendableBuilderType; + public final DexType generatedMessageLiteExtendableMessageType; public final DexType rawMessageInfoType; public final DexType messageLiteType; public final DexType methodToInvokeType; @@ -37,10 +40,13 @@ generatedMessageLiteExtendableBuilderMethods; public final MethodToInvokeMembers methodToInvokeMembers; + public final DexString defaultInstanceFieldName; public final DexString dynamicMethodName; public final DexString findLiteExtensionByNumberName; public final DexString newBuilderMethodName; + public final DexString protobufPackageDescriptorPrefix; + public final DexProto dynamicMethodProto; public final DexProto findLiteExtensionByNumberProto; @@ -49,6 +55,8 @@ public final DexMethod rawMessageInfoConstructor; public ProtoReferences(DexItemFactory factory) { + dexItemFactory = factory; + // Types. enumLiteMapType = factory.createType("Lcom/google/protobuf/Internal$EnumLiteMap;"); extendableMessageType = @@ -63,6 +71,8 @@ factory.createType("Lcom/google/protobuf/GeneratedMessageLite$Builder;"); generatedMessageLiteExtendableBuilderType = factory.createType("Lcom/google/protobuf/GeneratedMessageLite$ExtendableBuilder;"); + generatedMessageLiteExtendableMessageType = + factory.createType("Lcom/google/protobuf/GeneratedMessageLite$ExtendableMessage;"); rawMessageInfoType = factory.createType("Lcom/google/protobuf/RawMessageInfo;"); messageLiteType = factory.createType("Lcom/google/protobuf/MessageLite;"); methodToInvokeType = @@ -70,10 +80,14 @@ wireFormatFieldType = factory.createType("Lcom/google/protobuf/WireFormat$FieldType;"); // Names. + defaultInstanceFieldName = factory.createString("DEFAULT_INSTANCE"); dynamicMethodName = factory.createString("dynamicMethod"); findLiteExtensionByNumberName = factory.createString("findLiteExtensionByNumber"); newBuilderMethodName = factory.createString("newBuilder"); + // Other names. + protobufPackageDescriptorPrefix = factory.createString("Lcom/google/protobuf/"); + // Protos. dynamicMethodProto = factory.createProto( @@ -105,6 +119,10 @@ methodToInvokeMembers = new MethodToInvokeMembers(factory); } + public DexField getDefaultInstanceField(DexProgramClass holder) { + return dexItemFactory.createField(holder.type, holder.type, defaultInstanceFieldName); + } + public boolean isAbstractGeneratedMessageLiteBuilder(DexProgramClass clazz) { return clazz.type == generatedMessageLiteBuilderType || clazz.type == generatedMessageLiteExtendableBuilderType; @@ -123,7 +141,8 @@ } public boolean isDynamicMethodBridge(DexMethod method) { - return method == generatedMessageLiteMethods.dynamicMethodBridgeMethod; + return method == generatedMessageLiteMethods.dynamicMethodBridgeMethod + || method == generatedMessageLiteMethods.dynamicMethodBridgeMethodWithObject; } public boolean isDynamicMethodBridge(DexEncodedMethod method) { @@ -154,6 +173,10 @@ return method.match(newMessageInfoMethod) || method == rawMessageInfoConstructor; } + public boolean isProtoLibraryClass(DexProgramClass clazz) { + return clazz.type.descriptor.startsWith(protobufPackageDescriptorPrefix); + } + public class GeneratedExtensionMethods { public final DexMethod constructor; @@ -192,6 +215,7 @@ public final DexMethod createBuilderMethod; public final DexMethod dynamicMethodBridgeMethod; + public final DexMethod dynamicMethodBridgeMethodWithObject; public final DexMethod isInitializedMethod; public final DexMethod newRepeatedGeneratedExtension; public final DexMethod newSingularGeneratedExtension; @@ -207,6 +231,12 @@ generatedMessageLiteType, dexItemFactory.createProto(dexItemFactory.objectType, methodToInvokeType), "dynamicMethod"); + dynamicMethodBridgeMethodWithObject = + dexItemFactory.createMethod( + generatedMessageLiteType, + dexItemFactory.createProto( + dexItemFactory.objectType, methodToInvokeType, dexItemFactory.objectType), + "dynamicMethod"); isInitializedMethod = dexItemFactory.createMethod( generatedMessageLiteType, @@ -241,7 +271,7 @@ } } - class GeneratedMessageLiteBuilderMethods { + public class GeneratedMessageLiteBuilderMethods { public final DexMethod buildPartialMethod; public final DexMethod constructorMethod; @@ -260,9 +290,10 @@ } } - class GeneratedMessageLiteExtendableBuilderMethods { + public class GeneratedMessageLiteExtendableBuilderMethods { public final DexMethod buildPartialMethod; + public final DexMethod constructorMethod; private GeneratedMessageLiteExtendableBuilderMethods(DexItemFactory dexItemFactory) { buildPartialMethod = @@ -270,6 +301,12 @@ generatedMessageLiteExtendableBuilderType, dexItemFactory.createProto(extendableMessageType), "buildPartial"); + constructorMethod = + dexItemFactory.createMethod( + generatedMessageLiteExtendableBuilderType, + dexItemFactory.createProto( + dexItemFactory.voidType, generatedMessageLiteExtendableMessageType), + dexItemFactory.constructorMethodName); } }
diff --git a/src/main/java/com/android/tools/r8/ir/code/IRCodeInstructionListIterator.java b/src/main/java/com/android/tools/r8/ir/code/IRCodeInstructionListIterator.java index b9cc832..b4860aa 100644 --- a/src/main/java/com/android/tools/r8/ir/code/IRCodeInstructionListIterator.java +++ b/src/main/java/com/android/tools/r8/ir/code/IRCodeInstructionListIterator.java
@@ -151,6 +151,11 @@ } @Override + public void setInsertionPosition(Position position) { + instructionIterator.setInsertionPosition(position); + } + + @Override public void replaceCurrentInstruction(Instruction newInstruction, Set<Value> affectedValues) { instructionIterator.replaceCurrentInstruction(newInstruction, affectedValues); }
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 844ccce..c0f75e9 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
@@ -675,6 +675,9 @@ // 2) Revisit DexEncodedMethods for the collected candidates. printPhase("Primary optimization pass"); + if (fieldAccessAnalysis != null) { + fieldAccessAnalysis.fieldAssignmentTracker().initialize(); + } // Process the application identifying outlining candidates. GraphLense graphLenseForIR = appView.graphLense();
diff --git a/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java b/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java index b992f8d..1c1de57 100644 --- a/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java +++ b/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java
@@ -347,9 +347,9 @@ computeMethodRebinding(appInfo.directInvokes, this::anyLookup, Type.DIRECT); // Likewise static invokes. computeMethodRebinding(appInfo.staticInvokes, this::anyLookup, Type.STATIC); - computeFieldRebinding(); - - return builder.build(lense); + GraphLense lens = builder.build(lense); + appInfo.getFieldAccessInfoCollection().flattenAccessContexts(); + return lens; } }
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 8a54cca..1cc091d 100644 --- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java +++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -374,10 +374,15 @@ this.useRegistryFactory = createUseRegistryFactory(); this.workList = EnqueuerWorklist.createWorklist(appView); - if (options.protoShrinking().enableGeneratedMessageLiteShrinking - && mode.isInitialOrFinalTreeShaking()) { - registerAnalysis(new ProtoEnqueuerExtension(appView)); + if (mode.isInitialOrFinalTreeShaking()) { + if (options.protoShrinking().enableGeneratedMessageLiteShrinking) { + registerAnalysis(new ProtoEnqueuerExtension(appView)); + } + appView.withGeneratedMessageLiteBuilderShrinker( + shrinker -> registerAnalysis(shrinker.createEnqueuerAnalysis())); } + + liveTypes = new SetWithReportedReason<>(); initializedTypes = new SetWithReportedReason<>(); targetedMethods = new SetWithReason<>(graphReporter::registerMethod);
diff --git a/src/test/examplesProto/proto2/BuilderOnlyReferencedFromDynamicMethodTestClass.java b/src/test/examplesProto/proto2/BuilderOnlyReferencedFromDynamicMethodTestClass.java new file mode 100644 index 0000000..e3707dc --- /dev/null +++ b/src/test/examplesProto/proto2/BuilderOnlyReferencedFromDynamicMethodTestClass.java
@@ -0,0 +1,24 @@ +// Copyright (c) 2020, 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 proto2; + +import com.android.tools.r8.proto2.TestProto.Primitives; +import com.google.protobuf.GeneratedMessageLite; +import com.google.protobuf.InvalidProtocolBufferException; + +public class BuilderOnlyReferencedFromDynamicMethodTestClass { + + public static void main(String[] args) { + GeneratedMessageLite<?, ?> primitivesInDisguise; + try { + primitivesInDisguise = Primitives.parseFrom(new byte[0]); + } catch (InvalidProtocolBufferException e) { + System.out.println("Unexpected exception: " + e); + throw new RuntimeException(e); + } + Primitives primitives = (Primitives) primitivesInDisguise.toBuilder().build(); + Printer.print(primitives); + } +}
diff --git a/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderOnlyReferencedFromDynamicMethodTest.java b/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderOnlyReferencedFromDynamicMethodTest.java new file mode 100644 index 0000000..6405047 --- /dev/null +++ b/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderOnlyReferencedFromDynamicMethodTest.java
@@ -0,0 +1,78 @@ +// Copyright (c) 2020, 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.internal.proto; + +import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertFalse; + +import com.android.tools.r8.TestParameters; +import com.android.tools.r8.TestParametersCollection; +import com.android.tools.r8.utils.codeinspector.ClassSubject; +import com.android.tools.r8.utils.codeinspector.CodeInspector; +import com.google.common.collect.ImmutableList; +import java.nio.file.Path; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class Proto2BuilderOnlyReferencedFromDynamicMethodTest extends ProtoShrinkingTestBase { + + private static final String MAIN = "proto2.BuilderOnlyReferencedFromDynamicMethodTestClass"; + + private static List<Path> PROGRAM_FILES = + ImmutableList.of(PROTO2_EXAMPLES_JAR, PROTO2_PROTO_JAR, PROTOBUF_LITE_JAR); + + private final TestParameters parameters; + + @Parameterized.Parameters(name = "{0}") + public static TestParametersCollection data() { + return getTestParameters().withAllRuntimesAndApiLevels().build(); + } + + public Proto2BuilderOnlyReferencedFromDynamicMethodTest(TestParameters parameters) { + this.parameters = parameters; + } + + @Test + public void test() throws Exception { + testForR8(parameters.getBackend()) + .addProgramFiles(PROGRAM_FILES) + .addKeepMainRule(MAIN) + .addKeepRuleFiles(PROTOBUF_LITE_PROGUARD_RULES) + .allowAccessModification() + .allowDiagnosticMessages() + .allowUnusedProguardConfigurationRules() + .enableProtoShrinking() + .setMinApi(parameters.getApiLevel()) + .compile() + .assertAllInfoMessagesMatch( + containsString("Proguard configuration rule does not match anything")) + .assertAllWarningMessagesMatch(equalTo("Resource 'META-INF/MANIFEST.MF' already exists.")) + .inspect(this::inspect) + .run(parameters.getRuntime(), MAIN) + .assertSuccessWithOutputLines( + "false", "0", "false", "", "false", "0", "false", "0", "false", ""); + } + + private void inspect(CodeInspector outputInspector) { + verifyBuilderIsAbsent(outputInspector); + } + + private void verifyBuilderIsAbsent(CodeInspector outputInspector) { + ClassSubject generatedMessageLiteBuilder = + outputInspector.clazz("com.google.protobuf.GeneratedMessageLite$Builder"); + assertThat(generatedMessageLiteBuilder, isPresent()); + assertFalse(generatedMessageLiteBuilder.isAbstract()); + assertThat( + outputInspector.clazz("com.android.tools.r8.proto2.TestProto$Primitives$Builder"), + not(isPresent())); + } +}
diff --git a/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderShrinkingTest.java b/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderShrinkingTest.java index 8c98b46..dc2b296 100644 --- a/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderShrinkingTest.java +++ b/src/test/java/com/android/tools/r8/internal/proto/Proto2BuilderShrinkingTest.java
@@ -171,9 +171,7 @@ assertThat( outputInspector.clazz( "com.android.tools.r8.proto2.Shrinking$HasFlaggedOffExtension$Builder"), - mains.equals(ImmutableList.of("proto2.HasFlaggedOffExtensionBuilderTestClass")) - ? isPresent() - : not(isPresent())); + not(isPresent())); assertThat( outputInspector.clazz("com.android.tools.r8.proto2.TestProto$Primitives$Builder"), not(isPresent()));