ResourcePathShortener deterministically handles hash collisions
ResourcePathShortener appends one digit at the end of colliding short
file names to disambiguate them. This cl sorts the list of file paths so
that colliding files always get the same disambiguating digit from one
run to the next rather than possibly alternating.
Test: make aapt2_tests
Change-Id: I983a1448c21f2c79d7cdb1de232e7758c04fc256
diff --git a/tools/aapt2/optimize/ResourcePathShortener.cpp b/tools/aapt2/optimize/ResourcePathShortener.cpp
index 7f5d104..7ff9bf5 100644
--- a/tools/aapt2/optimize/ResourcePathShortener.cpp
+++ b/tools/aapt2/optimize/ResourcePathShortener.cpp
@@ -16,6 +16,7 @@
#include "optimize/ResourcePathShortener.h"
+#include <set>
#include <unordered_set>
#include "androidfw/StringPiece.h"
@@ -71,10 +72,19 @@
return shortened_path;
}
+// implement custom comparator of FileReference pointers so as to use the
+// underlying filepath as key rather than the integer address. This is to ensure
+// determinism of output for colliding files.
+struct PathComparator {
+ bool operator() (const FileReference* lhs, const FileReference* rhs) const {
+ return lhs->path->compare(*rhs->path);
+ }
+};
+
bool ResourcePathShortener::Consume(IAaptContext* context, ResourceTable* table) {
// used to detect collisions
std::unordered_set<std::string> shortened_paths;
- std::unordered_set<FileReference*> file_refs;
+ std::set<FileReference*, PathComparator> file_refs;
for (auto& package : table->packages) {
for (auto& type : package->types) {
for (auto& entry : type->entries) {
diff --git a/tools/aapt2/optimize/ResourcePathShortener_test.cpp b/tools/aapt2/optimize/ResourcePathShortener_test.cpp
index 1f45694..f5a02be 100644
--- a/tools/aapt2/optimize/ResourcePathShortener_test.cpp
+++ b/tools/aapt2/optimize/ResourcePathShortener_test.cpp
@@ -29,6 +29,14 @@
return android::StringPiece(iter, path.end() - iter);
}
+void FillTable(aapt::test::ResourceTableBuilder& builder, int start, int end) {
+ for (int i=start; i<end; i++) {
+ builder.AddFileReference(
+ "android:drawable/xmlfile" + std::to_string(i),
+ "res/drawable/xmlfile" + std::to_string(i) + ".xml");
+ }
+}
+
namespace aapt {
TEST(ResourcePathShortenerTest, FileRefPathsChangedInResourceTable) {
@@ -114,4 +122,45 @@
EXPECT_THAT(GetExtension(path_map[original_png_path]), Eq(android::StringPiece(".png")));
}
+TEST(ResourcePathShortenerTest, DeterministicallyHandleCollisions) {
+ std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
+
+ // 4000 resources is the limit at which the hash space is expanded to 3
+ // letters to reduce collisions, we want as many collisions as possible thus
+ // N-1.
+ const auto kNumResources = 3999;
+ const auto kNumTries = 5;
+
+ test::ResourceTableBuilder builder1;
+ FillTable(builder1, 0, kNumResources);
+ std::unique_ptr<ResourceTable> table1 = builder1.Build();
+ std::map<std::string, std::string> expected_mapping;
+ ASSERT_TRUE(ResourcePathShortener(expected_mapping).Consume(context.get(), table1.get()));
+
+ // We are trying to ensure lack of non-determinism, it is not simple to prove
+ // a negative, thus we must try the test a few times so that the test itself
+ // is non-flaky. Basically create the pathmap 5 times from the same set of
+ // resources but a different order of addition and then ensure they are always
+ // mapped to the same short path.
+ for (int i=0; i<kNumTries; i++) {
+ test::ResourceTableBuilder builder2;
+ // This loop adds resources to the resource table in the range of
+ // [0:kNumResources). Adding the file references in different order makes
+ // non-determinism more likely to surface. Thus we add resources
+ // [start_index:kNumResources) first then [0:start_index). We also use a
+ // different start_index each run.
+ int start_index = (kNumResources/kNumTries)*i;
+ FillTable(builder2, start_index, kNumResources);
+ FillTable(builder2, 0, start_index);
+ std::unique_ptr<ResourceTable> table2 = builder2.Build();
+
+ std::map<std::string, std::string> actual_mapping;
+ ASSERT_TRUE(ResourcePathShortener(actual_mapping).Consume(context.get(), table2.get()));
+
+ for (auto& item : actual_mapping) {
+ ASSERT_THAT(expected_mapping[item.first], Eq(item.second));
+ }
+ }
+}
+
} // namespace aapt