From 8d4ee973fa631c13ad458a3b906d8c1319fe0913 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Mon, 27 Aug 2018 11:24:04 -0700 Subject: AAPT2: Fix resource table load time regression A previous change (deee395) caused duplicate entries to be created for entries eith entry ids greater than 0x0ff. This is because the wrong data type was used (uint8_t instead of uint16_t). This made loading in resources slower as well since more entries had to be iterated over. Bug: 36051266 Test: Dumping all resources in 700 apks found in the android tree took 1 minute instead of 5 minutes. Created a test in aapt2_tests. Change-Id: I1c3d830da517a56ac3496221dbe605c72e0c6014 --- tools/aapt2/ResourceTable.cpp | 10 +++++----- tools/aapt2/ResourceTable.h | 5 +++-- tools/aapt2/ResourceTable_test.cpp | 40 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp index 58b5e8f0212c..23322ab277bf 100644 --- a/tools/aapt2/ResourceTable.cpp +++ b/tools/aapt2/ResourceTable.cpp @@ -51,7 +51,7 @@ static bool less_than_struct_with_name(const std::unique_ptr& lhs, const Stri template static bool less_than_struct_with_name_and_id(const std::unique_ptr& lhs, - const std::pair>& rhs) { + const std::pair>& rhs) { int name_cmp = lhs->name.compare(0, lhs->name.size(), rhs.first.data(), rhs.first.size()); return name_cmp < 0 || (name_cmp == 0 && rhs.second && lhs->id < rhs.second); } @@ -141,7 +141,7 @@ ResourceTableType* ResourceTablePackage::FindOrCreateType(ResourceType type, return types.emplace(iter, std::move(new_type))->get(); } -ResourceEntry* ResourceTableType::FindEntry(const StringPiece& name, const Maybe id) { +ResourceEntry* ResourceTableType::FindEntry(const StringPiece& name, const Maybe id) { const auto last = entries.end(); auto iter = std::lower_bound(entries.begin(), last, std::make_pair(name, id), less_than_struct_with_name_and_id); @@ -152,7 +152,7 @@ ResourceEntry* ResourceTableType::FindEntry(const StringPiece& name, const Maybe } ResourceEntry* ResourceTableType::FindOrCreateEntry(const StringPiece& name, - const Maybe id) { + const Maybe id) { auto last = entries.end(); auto iter = std::lower_bound(entries.begin(), last, std::make_pair(name, id), less_than_struct_with_name_and_id); @@ -450,7 +450,7 @@ bool ResourceTable::AddResourceImpl(const ResourceNameRef& name, const ResourceI } ResourceEntry* entry = type->FindOrCreateEntry(name.entry, use_id ? res_id.entry_id() - : Maybe()); + : Maybe()); // Check for entries appearing twice with two different entry ids if (check_id && entry->id && entry->id.value() != res_id.entry_id()) { @@ -561,7 +561,7 @@ bool ResourceTable::SetVisibilityImpl(const ResourceNameRef& name, const Visibil } ResourceEntry* entry = type->FindOrCreateEntry(name.entry, use_id ? res_id.entry_id() - : Maybe()); + : Maybe()); // Check for entries appearing twice with two different entry ids if (check_id && entry->id && entry->id.value() != res_id.entry_id()) { diff --git a/tools/aapt2/ResourceTable.h b/tools/aapt2/ResourceTable.h index c40323c34f48..5a43a2d86cfe 100644 --- a/tools/aapt2/ResourceTable.h +++ b/tools/aapt2/ResourceTable.h @@ -146,9 +146,10 @@ class ResourceTableType { explicit ResourceTableType(const ResourceType type) : type(type) {} - ResourceEntry* FindEntry(const android::StringPiece& name, Maybe id = Maybe()); + ResourceEntry* FindEntry(const android::StringPiece& name, + Maybe id = Maybe()); ResourceEntry* FindOrCreateEntry(const android::StringPiece& name, - Maybe id = Maybe()); + Maybe id = Maybe()); private: DISALLOW_COPY_AND_ASSIGN(ResourceTableType); diff --git a/tools/aapt2/ResourceTable_test.cpp b/tools/aapt2/ResourceTable_test.cpp index 7fa8ea2f7f94..1aa97511dd37 100644 --- a/tools/aapt2/ResourceTable_test.cpp +++ b/tools/aapt2/ResourceTable_test.cpp @@ -258,4 +258,44 @@ TEST(ResourceTableTest, SetOverlayable) { ASSERT_FALSE(table.SetOverlayable(name, overlayable, test::GetDiagnostics())); } +TEST(ResourceTableTest, AllowDuplictaeResourcesNames) { + ResourceTable table(/* validate_resources */ false); + + const ResourceName foo_name = test::ParseNameOrDie("android:bool/foo"); + ASSERT_TRUE(table.AddResourceWithId(foo_name, ResourceId(0x7f0100ff), ConfigDescription{} , "", + test::BuildPrimitive(android::Res_value::TYPE_INT_BOOLEAN, 0), + test::GetDiagnostics())); + ASSERT_TRUE(table.AddResourceWithId(foo_name, ResourceId(0x7f010100), ConfigDescription{} , "", + test::BuildPrimitive(android::Res_value::TYPE_INT_BOOLEAN, 1), + test::GetDiagnostics())); + + ASSERT_TRUE(table.SetVisibilityWithId(foo_name, Visibility{Visibility::Level::kPublic}, + ResourceId(0x7f0100ff), test::GetDiagnostics())); + ASSERT_TRUE(table.SetVisibilityWithId(foo_name, Visibility{Visibility::Level::kPrivate}, + ResourceId(0x7f010100), test::GetDiagnostics())); + + auto package = table.FindPackageById(0x7f); + ASSERT_THAT(package, NotNull()); + auto type = package->FindType(ResourceType::kBool); + ASSERT_THAT(type, NotNull()); + + auto entry1 = type->FindEntry("foo", 0x00ff); + ASSERT_THAT(entry1, NotNull()); + ASSERT_THAT(entry1->id, Eq(0x00ff)); + ASSERT_THAT(entry1->values[0], NotNull()); + ASSERT_THAT(entry1->values[0]->value, NotNull()); + ASSERT_THAT(ValueCast(entry1->values[0]->value.get()), NotNull()); + ASSERT_THAT(ValueCast(entry1->values[0]->value.get())->value.data, Eq(0u)); + ASSERT_THAT(entry1->visibility.level, Visibility::Level::kPublic); + + auto entry2 = type->FindEntry("foo", 0x0100); + ASSERT_THAT(entry2, NotNull()); + ASSERT_THAT(entry2->id, Eq(0x0100)); + ASSERT_THAT(entry2->values[0], NotNull()); + ASSERT_THAT(entry1->values[0]->value, NotNull()); + ASSERT_THAT(ValueCast(entry2->values[0]->value.get()), NotNull()); + ASSERT_THAT(ValueCast(entry2->values[0]->value.get())->value.data, Eq(1u)); + ASSERT_THAT(entry2->visibility.level, Visibility::Level::kPrivate); +} + } // namespace aapt -- cgit v1.2.3-59-g8ed1b