Ensure that we keep resource table entries for overlapping res entries
This solves the issue that when we have
base: res/xml/foo.xml
and
feature: res/xml/foo.xml
we will keep both files when writing, even if only one of these are
reachable through a resource table entry (in either base or feature)
This ensures that we keep the resource table entry.
Bug: b/384905036
Change-Id: I7d16826ad522664e5d98c000e9e9df64378a0484
diff --git a/src/main/java/com/android/tools/r8/R8.java b/src/main/java/com/android/tools/r8/R8.java
index 3741cb5..6f99720 100644
--- a/src/main/java/com/android/tools/r8/R8.java
+++ b/src/main/java/com/android/tools/r8/R8.java
@@ -990,11 +990,9 @@
LegacyResourceShrinker shrinker = resourceShrinkerBuilder.build();
shrinkerResult = shrinker.run();
}
- Set<String> toKeep = shrinkerResult.getResFolderEntriesToKeep();
writeResourcesToConsumer(
reporter,
shrinkerResult,
- toKeep,
options.androidResourceProvider,
options.androidResourceConsumer,
FeatureSplit.BASE);
@@ -1004,7 +1002,6 @@
writeResourcesToConsumer(
reporter,
shrinkerResult,
- toKeep,
featureSplit.getAndroidResourceProvider(),
featureSplit.getAndroidResourceConsumer(),
featureSplit);
@@ -1019,7 +1016,6 @@
private static void writeResourcesToConsumer(
Reporter reporter,
ShrinkerResult shrinkerResult,
- Set<String> toKeep,
AndroidResourceProvider androidResourceProvider,
AndroidResourceConsumer androidResourceConsumer,
FeatureSplit featureSplit)
@@ -1044,7 +1040,9 @@
break;
case RES_FOLDER_FILE:
case XML_FILE:
- if (toKeep.contains(androidResource.getPath().location())) {
+ if (shrinkerResult
+ .getResFolderEntriesToKeep()
+ .contains(androidResource.getPath().location())) {
androidResourceConsumer.accept(
new R8PassThroughAndroidResource(androidResource, reporter), reporter);
}
diff --git a/src/resourceshrinker/java/com/android/build/shrinker/ResourceShrinkerImpl.kt b/src/resourceshrinker/java/com/android/build/shrinker/ResourceShrinkerImpl.kt
index d404e2e6..b13a28b 100644
--- a/src/resourceshrinker/java/com/android/build/shrinker/ResourceShrinkerImpl.kt
+++ b/src/resourceshrinker/java/com/android/build/shrinker/ResourceShrinkerImpl.kt
@@ -307,16 +307,12 @@
return isJarPathReachable(folder, name);
}
-private fun ResourceStore.getResourceId(
- folder: String,
- name: String
-): Int {
- val folderType = ResourceFolderType.getFolderType(folder) ?: return -1
+fun ResourceStore.getResourcesFor(path: String): List<Resource> {
+ val (_, folder, name) = path.split('/', limit = 3)
+ val folderType = ResourceFolderType.getFolderType(folder) ?: return emptyList()
val resourceName = name.substringBefore('.')
return FolderTypeRelationship.getRelatedResourceTypes(folderType)
.filterNot { it == ResourceType.ID }
.flatMap { getResources(it, resourceName) }
- .map { it.value }
- .getOrElse(0) { -1 }
-
+ .toList()
}
diff --git a/src/resourceshrinker/java/com/android/build/shrinker/r8integration/LegacyResourceShrinker.java b/src/resourceshrinker/java/com/android/build/shrinker/r8integration/LegacyResourceShrinker.java
index 6cbc8c9..a64a3fd 100644
--- a/src/resourceshrinker/java/com/android/build/shrinker/r8integration/LegacyResourceShrinker.java
+++ b/src/resourceshrinker/java/com/android/build/shrinker/r8integration/LegacyResourceShrinker.java
@@ -101,7 +101,7 @@
public Builder addResFolderInput(String path, byte[] bytes) {
PathAndBytes existing = resFolderInputs.get(path);
if (existing != null) {
- assert Arrays.equals(existing.getBytes(), bytes);
+ existing.setDuplicated(true);
} else {
resFolderInputs.put(path, new PathAndBytes(bytes, path));
}
@@ -111,7 +111,7 @@
public Builder addXmlInput(String path, byte[] bytes) {
PathAndBytes existing = xmlInputs.get(path);
if (existing != null) {
- assert Arrays.equals(existing.getBytes(), bytes);
+ existing.setDuplicated(true);
} else {
xmlInputs.put(path, new PathAndBytes(bytes, path));
}
@@ -217,14 +217,20 @@
debugReporter.debug(() -> "The root reachable resources are:");
roots.forEach(root -> debugReporter.debug(() -> " " + root));
});
- debugReporter.debug(() -> "Unused resources are: ");
- unusedResources.forEach(unused -> debugReporter.debug(() -> " " + unused));
- ImmutableSet.Builder<String> resEntriesToKeep = new ImmutableSet.Builder<>();
+ ImmutableSet.Builder<String> resEntriesToKeepBuilder = new ImmutableSet.Builder<>();
for (PathAndBytes xmlInput : Iterables.concat(xmlInputs, resFolderInputs)) {
if (ResourceShrinkerImplKt.isJarPathReachable(resourceStore, xmlInput.path.toString())) {
- resEntriesToKeep.add(xmlInput.path.toString());
+ resEntriesToKeepBuilder.add(xmlInput.path.toString());
+ if (xmlInput.duplicated) {
+ // Ensure that we don't remove references to duplicated res folder entries.
+ List<Resource> duplicatedResources =
+ ResourceShrinkerImplKt.getResourcesFor(resourceStore, xmlInput.path.toString());
+ unusedResources.removeAll(duplicatedResources);
+ }
}
}
+ debugReporter.debug(() -> "Unused resources are: ");
+ unusedResources.forEach(unused -> debugReporter.debug(() -> " " + unused));
List<Integer> resourceIdsToRemove = getResourceIdsToRemove(unusedResources);
Map<FeatureSplit, ResourceTable> shrunkenTables = new HashMap<>();
for (Entry<PathAndBytes, FeatureSplit> entry : resourceTables.entrySet()) {
@@ -233,7 +239,7 @@
ResourceTable.parseFrom(entry.getKey().bytes), resourceIdsToRemove);
shrunkenTables.put(entry.getValue(), shrunkenResourceTable);
}
- return new ShrinkerResult(resEntriesToKeep.build(), shrunkenTables);
+ return new ShrinkerResult(resEntriesToKeepBuilder.build(), shrunkenTables);
}
private static List<Integer> getResourceIdsToRemove(List<Resource> unusedResources) {
@@ -331,12 +337,17 @@
private static class PathAndBytes {
private final byte[] bytes;
private final String path;
+ private boolean duplicated;
private PathAndBytes(byte[] bytes, String path) {
this.bytes = bytes;
this.path = path;
}
+ public void setDuplicated(boolean duplicated) {
+ this.duplicated = duplicated;
+ }
+
public String getPathWithoutRes() {
assert path.startsWith("res/");
return path.substring(4);
diff --git a/src/resourceshrinker/java/com/android/build/shrinker/r8integration/R8ResourceShrinkerState.java b/src/resourceshrinker/java/com/android/build/shrinker/r8integration/R8ResourceShrinkerState.java
index 3dded16..4740a7c 100644
--- a/src/resourceshrinker/java/com/android/build/shrinker/r8integration/R8ResourceShrinkerState.java
+++ b/src/resourceshrinker/java/com/android/build/shrinker/r8integration/R8ResourceShrinkerState.java
@@ -54,6 +54,7 @@
private final Function<Exception, RuntimeException> errorHandler;
private final R8ResourceShrinkerModel r8ResourceShrinkerModel;
private final Map<String, Supplier<InputStream>> xmlFileProviders = new HashMap<>();
+ private final Set<String> duplicatedResFolderEntries = new HashSet<>();
private final List<Supplier<InputStream>> keepRuleFileProviders = new ArrayList<>();
private final List<Supplier<InputStream>> manifestProviders = new ArrayList<>();
@@ -62,7 +63,7 @@
private final ShrinkerDebugReporter shrinkerDebugReporter;
private ClassReferenceCallback enqueuerCallback;
private MethodReferenceCallback methodCallback;
- private Map<Integer, List<String>> resourceIdToXmlFiles;
+ private Map<Integer, Set<String>> resourceIdToXmlFiles;
private Set<String> packageNames;
private final Set<String> seenNoneClassValues = new HashSet<>();
private final Set<Integer> seenResourceIds = new HashSet<>();
@@ -172,7 +173,11 @@
}
public void addXmlFileProvider(Supplier<InputStream> inputStreamSupplier, String location) {
- this.xmlFileProviders.put(location, inputStreamSupplier);
+ if (this.xmlFileProviders.containsKey(location)) {
+ duplicatedResFolderEntries.add(location);
+ } else {
+ this.xmlFileProviders.put(location, inputStreamSupplier);
+ }
}
public void addKeepRuleRileProvider(Supplier<InputStream> inputStreamSupplier) {
@@ -180,6 +185,9 @@
}
public void addResFileProvider(Supplier<InputStream> inputStreamSupplier, String location) {
+ if (this.resfileProviders.containsKey(location)) {
+ duplicatedResFolderEntries.add(location);
+ }
this.resfileProviders.put(location, inputStreamSupplier);
}
@@ -253,11 +261,24 @@
}
private void traceXmlForResourceId(int id) {
- List<String> xmlFiles = getResourceIdToXmlFiles().get(id);
+ Set<String> xmlFiles = getResourceIdToXmlFiles().get(id);
if (xmlFiles != null) {
for (String xmlFile : xmlFiles) {
InputStream inputStream = xmlFileProviders.get(xmlFile).get();
traceXml(xmlFile, inputStream);
+ if (duplicatedResFolderEntries.contains(xmlFile)) {
+ traceDuplicatedXmlFileIds(id, xmlFile);
+ }
+ }
+ }
+ }
+
+ private void traceDuplicatedXmlFileIds(int currentId, String xmlFile) {
+ for (Map.Entry<Integer, Set<String>> entry : getResourceIdToXmlFiles().entrySet()) {
+ if (entry.getValue().contains(xmlFile)) {
+ if (entry.getKey() != currentId) {
+ trace(entry.getKey(), "Duplicated xmlfile " + xmlFile);
+ }
}
}
}
@@ -339,7 +360,7 @@
return packageName + "." + xmlAttribute.getValue();
}
- public Map<Integer, List<String>> getResourceIdToXmlFiles() {
+ public Map<Integer, Set<String>> getResourceIdToXmlFiles() {
if (resourceIdToXmlFiles == null) {
resourceIdToXmlFiles = new HashMap<>();
for (ResourceTable resourceTable : resourceTables.values()) {
@@ -356,7 +377,7 @@
if (file.getType() == FileReference.Type.PROTO_XML) {
int id = ResourceTableUtilKt.toIdentifier(packageEntry, type, entry);
resourceIdToXmlFiles
- .computeIfAbsent(id, unused -> new ArrayList<>())
+ .computeIfAbsent(id, unused -> new HashSet<>())
.add(file.getPath());
}
}
diff --git a/src/test/java/com/android/tools/r8/androidresources/ResourceShrinkingWithFeaturesAndDuplicatedResEntryTest.java b/src/test/java/com/android/tools/r8/androidresources/ResourceShrinkingWithFeaturesAndDuplicatedResEntryTest.java
new file mode 100644
index 0000000..ae4b83f
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/androidresources/ResourceShrinkingWithFeaturesAndDuplicatedResEntryTest.java
@@ -0,0 +1,144 @@
+// 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.androidresources;
+
+import com.android.tools.r8.R8FullTestBuilder;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.androidresources.AndroidResourceTestingUtils.AndroidTestResource;
+import com.android.tools.r8.androidresources.AndroidResourceTestingUtils.AndroidTestResourceBuilder;
+import com.android.tools.r8.utils.BooleanUtils;
+import java.io.IOException;
+import java.util.List;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class ResourceShrinkingWithFeaturesAndDuplicatedResEntryTest extends TestBase {
+
+ @Parameter(0)
+ public TestParameters parameters;
+
+ private static final String SMALL_XML = "<Z/>";
+
+ @Parameter(1)
+ public boolean optimized;
+
+ @Parameters(name = "{0}, optimized: {1}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ getTestParameters().withDefaultDexRuntime().withAllApiLevels().build(),
+ BooleanUtils.values());
+ }
+
+ public static AndroidTestResource getTestResources(TemporaryFolder temp) throws Exception {
+ return new AndroidTestResourceBuilder()
+ .withSimpleManifestAndAppNameString()
+ .addRClassInitializeWithDefaultValues(R.xml.class, R.string.class)
+ .addXml("base_used_xml.xml", SMALL_XML)
+ .addXml("base_unused_xml.xml", SMALL_XML)
+ .addXml("duplicated_xml.xml", SMALL_XML)
+ .build(temp);
+ }
+
+ public AndroidTestResource getFeatureSplitTestResources(TemporaryFolder temp) throws IOException {
+ return new AndroidTestResourceBuilder()
+ .withSimpleManifestAndAppNameString()
+ .setPackageId(0x7e)
+ .addRClassInitializeWithDefaultValues(FeatureSplit.R.xml.class, FeatureSplit.R.string.class)
+ .addXml("feature_used_xml.xml", SMALL_XML)
+ .addXml("feature_unused_xml.xml", SMALL_XML)
+ .addXml("duplicated_xml.xml", SMALL_XML)
+ .build(temp);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ TemporaryFolder featureSplitTemp = ToolHelper.getTemporaryFolderForTest();
+ featureSplitTemp.create();
+ testForR8(parameters.getBackend())
+ .setMinApi(parameters)
+ .addProgramClasses(Base.class)
+ .addFeatureSplit(FeatureSplit.FeatureSplitMain.class)
+ .addAndroidResources(getTestResources(temp))
+ .addFeatureSplitAndroidResources(
+ getFeatureSplitTestResources(featureSplitTemp), FeatureSplit.class.getName())
+ .applyIf(optimized, R8FullTestBuilder::enableOptimizedShrinking)
+ .addKeepMainRule(Base.class)
+ .addKeepMainRule(FeatureSplit.FeatureSplitMain.class)
+ .compile()
+ .inspectShrunkenResources(
+ resourceTableInspector -> {
+ resourceTableInspector.assertDoesNotContainResourceWithName("xml", "base_unused_xml");
+ resourceTableInspector.assertContainsResourceWithName("xml", "base_used_xml");
+ resourceTableInspector.assertContainsResourceWithName("xml", "duplicated_xml");
+ resourceTableInspector.assertDoesNotContainResourceWithName(
+ "string", "duplicated_xml");
+ })
+ .inspectShrunkenResourcesForFeature(
+ resourceTableInspector -> {
+ resourceTableInspector.assertDoesNotContainResourceWithName(
+ "xml", "feature_unused_xml");
+ resourceTableInspector.assertContainsResourceWithName("xml", "feature_used_xml");
+ resourceTableInspector.assertContainsResourceWithName("xml", "duplicated_xml");
+ resourceTableInspector.assertDoesNotContainResourceWithName(
+ "string", "duplicated_xml");
+ },
+ FeatureSplit.class.getName())
+ .run(parameters.getRuntime(), Base.class)
+ .assertSuccess();
+ }
+
+ public static class Base {
+
+ public static void main(String[] args) {
+ if (System.currentTimeMillis() == 0) {
+ System.out.println(R.xml.base_used_xml);
+ System.out.println(R.xml.duplicated_xml);
+ }
+ }
+ }
+
+ public static class R {
+ public static class string {
+ // We should still remove unreachable entries with overlapping name if the type is different
+ public static int duplicated_xml;
+ }
+
+ public static class xml {
+ public static int base_used_xml;
+ public static int base_unused_xml;
+ public static int duplicated_xml;
+ }
+ }
+
+ public static class FeatureSplit {
+ public static class FeatureSplitMain {
+ public static void main(String[] args) {
+ if (System.currentTimeMillis() == 0) {
+ System.out.println(R.xml.feature_used_xml);
+ }
+ }
+ }
+
+ public static class R {
+ public static class string {
+ // We should still remove unreachable entries with overlapping name if the type is different
+ public static int duplicated_xml;
+ }
+
+ public static class xml {
+ public static int feature_used_xml;
+ public static int feature_unused_xml;
+ // Unused in the feature
+ public static int duplicated_xml;
+ }
+ }
+ }
+}
