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());
