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