Require instance fields are kept when instantiating classes using Unsafe This effectively reverts 6250eeeafa351b13ef371e8a67f5b8c641459e1b. The reason for the revert is that this causes +1% regressions in dex size. When sun.misc.Unsafe is used to instantiate a class without an instance initializer, the previous behavior can be restored by keeping the instance fields of the instantiated class. See also DefaultFieldValueAnalysisWithKeptSubclassTest. Bug: b/379034741 Change-Id: Ie3795040413e888691ae1316e1878da9f77f6707
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java index 4c9489a..bd2fdb3 100644 --- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java +++ b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java
@@ -56,6 +56,9 @@ public class DefaultFieldValueJoiner { + private static final boolean includeDefaultValueForAllFieldsWhereHolderIsKeptOrHasKeptSubclass = + false; + private final AppView<AppInfoWithLiveness> appView; private final Set<DexProgramClass> classesWithSingleCallerInlinedInstanceInitializers; private final FieldStateCollection fieldStates; @@ -88,26 +91,28 @@ // Classes that are kept or have a kept subclass can be instantiated in a way that does not use // any instance initializers. For all fields on such classes, we therefore include the default // field value. - Map<DexProgramClass, Boolean> classesWithKeptSubclasses = - computeClassesWithKeptSubclasses(fieldsOfInterest); ProgramFieldSet fieldsWithLiveDefaultValue = ProgramFieldSet.createConcurrent(); - MapUtils.removeIf( - fieldsOfInterest, - (clazz, fields) -> { - Boolean isKeptOrHasKeptSubclass = classesWithKeptSubclasses.get(clazz); - assert isKeptOrHasKeptSubclass != null; - if (isKeptOrHasKeptSubclass) { - fields.removeIf( - field -> { - if (field.getDefinition().isInstance()) { - fieldsWithLiveDefaultValue.add(field); - return true; - } - return false; - }); - } - return fields.isEmpty(); - }); + if (includeDefaultValueForAllFieldsWhereHolderIsKeptOrHasKeptSubclass) { + Map<DexProgramClass, Boolean> classesWithKeptSubclasses = + computeClassesWithKeptSubclasses(fieldsOfInterest); + MapUtils.removeIf( + fieldsOfInterest, + (clazz, fields) -> { + Boolean isKeptOrHasKeptSubclass = classesWithKeptSubclasses.get(clazz); + assert isKeptOrHasKeptSubclass != null; + if (isKeptOrHasKeptSubclass) { + fields.removeIf( + field -> { + if (field.getDefinition().isInstance()) { + fieldsWithLiveDefaultValue.add(field); + return true; + } + return false; + }); + } + return fields.isEmpty(); + }); + } // If constructor inlining is disabled, then we focus on whether each instance initializer // definitely assigns the given field before it is read. We do the same for final and static @@ -330,6 +335,12 @@ (holderType, fields) -> { assert !fields.isEmpty(); DexProgramClass holder = fields.iterator().next().getHolder(); + // If the class is kept it could be instantiated directly, in which case all default field + // values could be live. + if (appView.getKeepInfo(holder).isPinned(appView.options())) { + fields.forEach(liveDefaultValueConsumer); + return true; + } if (holder.isFinal() || !appView.appInfo().isInstantiatedIndirectly(holder)) { // When the class is not explicitly marked final, the class could in principle have // injected subclasses if it is pinned. However, none of the fields are pinned, so we
diff --git a/src/test/examplesJava17/records/RecordBlogTest.java b/src/test/examplesJava17/records/RecordBlogTest.java index c28b92b..1128f07 100644 --- a/src/test/examplesJava17/records/RecordBlogTest.java +++ b/src/test/examplesJava17/records/RecordBlogTest.java
@@ -33,7 +33,7 @@ ImmutableMap.<String, String>builder() .put("-dontobfuscate\n-dontoptimize", "RecordBlog$Person[name=%s, age=42]") .put("", "a[a=%s]") - .put("-keep,allowshrinking class " + CLASS, "RecordBlog$Person[a=%s, b=42]") + .put("-keep,allowshrinking class " + CLASS, "RecordBlog$Person[a=%s]") .put( "-keepclassmembers,allowshrinking,allowoptimization class " + CLASS
diff --git a/src/test/java/com/android/tools/r8/optimize/argumentpropagation/DefaultFieldValueAnalysisWithKeptSubclassTest.java b/src/test/java/com/android/tools/r8/optimize/argumentpropagation/DefaultFieldValueAnalysisWithKeptSubclassTest.java index dd9a582..9987908 100644 --- a/src/test/java/com/android/tools/r8/optimize/argumentpropagation/DefaultFieldValueAnalysisWithKeptSubclassTest.java +++ b/src/test/java/com/android/tools/r8/optimize/argumentpropagation/DefaultFieldValueAnalysisWithKeptSubclassTest.java
@@ -4,6 +4,7 @@ package com.android.tools.r8.optimize.argumentpropagation; import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; +import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentIf; import static org.hamcrest.MatcherAssert.assertThat; import com.android.tools.r8.NeverClassInline; @@ -11,10 +12,12 @@ import com.android.tools.r8.NoVerticalClassMerging; import com.android.tools.r8.TestBase; import com.android.tools.r8.TestParameters; -import com.android.tools.r8.TestParametersCollection; +import com.android.tools.r8.TestRunResult; import com.android.tools.r8.ToolHelper.DexVm.Version; +import com.android.tools.r8.utils.BooleanUtils; import com.android.tools.r8.utils.codeinspector.ClassSubject; import java.lang.reflect.Field; +import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -26,15 +29,20 @@ public class DefaultFieldValueAnalysisWithKeptSubclassTest extends TestBase { @Parameter(0) + public boolean keepFields; + + @Parameter(1) public TestParameters parameters; @Parameters(name = "{0}") - public static TestParametersCollection data() { - return getTestParameters() - .withCfRuntimes() - .withDexRuntimesStartingFromIncluding(Version.V5_1_1) - .withAllApiLevels() - .build(); + public static List<Object[]> data() { + return buildParameters( + BooleanUtils.values(), + getTestParameters() + .withCfRuntimes() + .withDexRuntimesStartingFromIncluding(Version.V5_1_1) + .withAllApiLevels() + .build()); } @Test @@ -55,6 +63,13 @@ .addInnerClasses(getClass()) .addKeepClassAndMembersRules(Main.class) .addKeepClassRules(B.class) + .applyIf( + keepFields, + b -> + b.addKeepRules( + "-keepclassmembers class " + A.class.getTypeName() + " {", + " !static <fields>;", + "}")) .enableInliningAnnotations() .enableNeverClassInliningAnnotations() .enableNoVerticalClassMergingAnnotations() @@ -64,10 +79,13 @@ inspector -> { ClassSubject aClassSubject = inspector.clazz(A.class); assertThat(aClassSubject, isPresent()); - assertThat(aClassSubject.uniqueFieldWithOriginalName("f"), isPresent()); + assertThat(aClassSubject.uniqueFieldWithOriginalName("f"), isPresentIf(keepFields)); }) .run(parameters.getRuntime(), Main.class, B.class.getTypeName()) - .assertSuccessWithOutputLines("Hello, world!"); + .applyIf( + keepFields, + rr -> rr.assertSuccessWithOutputLines("Hello, world!"), + TestRunResult::assertSuccessWithEmptyOutput); } static class Main {
diff --git a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedFieldTypeTest.java b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedFieldTypeTest.java index 181a85a..693a077 100644 --- a/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedFieldTypeTest.java +++ b/src/test/java/com/android/tools/r8/shaking/ifrule/verticalclassmerging/MergedFieldTypeTest.java
@@ -118,7 +118,7 @@ ClassSubject testClassSubject = inspector.clazz(TestClass.class); assertThat(testClassSubject, isPresent()); - if (enableVerticalClassMerging) { + if (enableVerticalClassMerging && parameters.canInitNewInstanceUsingSuperclassConstructor()) { // Verify that TestClass.field has been removed. assertEquals(1, testClassSubject.allFields().size());