From db70ce5e788404f36cb5dbb137c6a8f79f34a2a0 Mon Sep 17 00:00:00 2001 From: Mathieu Chartier Date: Mon, 12 Dec 2016 11:06:59 -0800 Subject: [PATCH] Address some review comments Addressed comments in dex cache and class table. Added class table test. Test: mm test-art-host-gtest-class_table_test -j20 Change-Id: I3ec0282247187acb1ec7af25b309501f001a1c3e --- build/Android.gtest.mk | 2 + runtime/Android.bp | 1 + runtime/class_table-inl.h | 13 ++++ runtime/class_table.cc | 24 +++--- runtime/class_table.h | 7 ++ runtime/class_table_test.cc | 163 +++++++++++++++++++++++++++++++++++++++++ runtime/mirror/dex_cache-inl.h | 2 +- 7 files changed, 202 insertions(+), 10 deletions(-) create mode 100644 runtime/class_table_test.cc diff --git a/build/Android.gtest.mk b/build/Android.gtest.mk index 1691dbb3b..d59d8f690 100644 --- a/build/Android.gtest.mk +++ b/build/Android.gtest.mk @@ -86,6 +86,7 @@ ART_GTEST_dex2oat_environment_tests_DEX_DEPS := Main MainStripped MultiDex Multi ART_GTEST_atomic_method_ref_map_test_DEX_DEPS := Interfaces ART_GTEST_class_linker_test_DEX_DEPS := Interfaces MethodTypes MultiDex MyClass Nested Statics StaticsFromCode +ART_GTEST_class_table_test_DEX_DEPS := XandY ART_GTEST_compiler_driver_test_DEX_DEPS := AbstractMethod StaticLeafMethods ProfileTestMultiDex ART_GTEST_dex_cache_test_DEX_DEPS := Main Packages MethodTypes ART_GTEST_dex_file_test_DEX_DEPS := GetMethodSignature Main Nested @@ -598,6 +599,7 @@ ART_TEST_TARGET_VALGRIND_GTEST$(2ND_ART_PHONY_TEST_TARGET_SUFFIX)_RULES := ART_TEST_TARGET_VALGRIND_GTEST_RULES := ART_GTEST_TARGET_ANDROID_ROOT := ART_GTEST_class_linker_test_DEX_DEPS := +ART_GTEST_class_table_test_DEX_DEPS := ART_GTEST_compiler_driver_test_DEX_DEPS := ART_GTEST_dex_file_test_DEX_DEPS := ART_GTEST_exception_test_DEX_DEPS := diff --git a/runtime/Android.bp b/runtime/Android.bp index 08be5b297..32ebee24c 100644 --- a/runtime/Android.bp +++ b/runtime/Android.bp @@ -517,6 +517,7 @@ art_cc_test { "base/unix_file/fd_file_test.cc", "cha_test.cc", "class_linker_test.cc", + "class_table_test.cc", "compiler_filter_test.cc", "dex_file_test.cc", "dex_file_verifier_test.cc", diff --git a/runtime/class_table-inl.h b/runtime/class_table-inl.h index 229cd477d..dfe894913 100644 --- a/runtime/class_table-inl.h +++ b/runtime/class_table-inl.h @@ -71,6 +71,19 @@ bool ClassTable::Visit(Visitor& visitor) { return true; } +template +bool ClassTable::Visit(const Visitor& visitor) { + ReaderMutexLock mu(Thread::Current(), lock_); + for (ClassSet& class_set : classes_) { + for (TableSlot& table_slot : class_set) { + if (!visitor(table_slot.Read())) { + return false; + } + } + } + return true; +} + template inline mirror::Class* ClassTable::TableSlot::Read() const { const uint32_t before = data_.LoadRelaxed(); diff --git a/runtime/class_table.cc b/runtime/class_table.cc index ec33e5ef8..0f985c642 100644 --- a/runtime/class_table.cc +++ b/runtime/class_table.cc @@ -33,8 +33,9 @@ void ClassTable::FreezeSnapshot() { bool ClassTable::Contains(ObjPtr klass) { ReaderMutexLock mu(Thread::Current(), lock_); + TableSlot slot(klass); for (ClassSet& class_set : classes_) { - auto it = class_set.Find(TableSlot(klass)); + auto it = class_set.Find(slot); if (it != class_set.end()) { return it->Read() == klass; } @@ -44,8 +45,9 @@ bool ClassTable::Contains(ObjPtr klass) { mirror::Class* ClassTable::LookupByDescriptor(ObjPtr klass) { ReaderMutexLock mu(Thread::Current(), lock_); + TableSlot slot(klass); for (ClassSet& class_set : classes_) { - auto it = class_set.Find(TableSlot(klass)); + auto it = class_set.Find(slot); if (it != class_set.end()) { return it->Read(); } @@ -110,8 +112,8 @@ size_t ClassTable::NumNonZygoteClasses(ObjPtr defining_load } mirror::Class* ClassTable::Lookup(const char* descriptor, size_t hash) { - ReaderMutexLock mu(Thread::Current(), lock_); DescriptorHashPair pair(descriptor, hash); + ReaderMutexLock mu(Thread::Current(), lock_); for (ClassSet& class_set : classes_) { auto it = class_set.FindWithHash(pair, hash); if (it != class_set.end()) { @@ -122,12 +124,14 @@ mirror::Class* ClassTable::Lookup(const char* descriptor, size_t hash) { } void ClassTable::Insert(ObjPtr klass) { + const uint32_t hash = TableSlot::HashDescriptor(klass); WriterMutexLock mu(Thread::Current(), lock_); - classes_.back().Insert(TableSlot(klass)); + classes_.back().InsertWithHash(TableSlot(klass, hash), hash); } void ClassTable::InsertWithoutLocks(ObjPtr klass) { - classes_.back().Insert(TableSlot(klass)); + const uint32_t hash = TableSlot::HashDescriptor(klass); + classes_.back().InsertWithHash(TableSlot(klass, hash), hash); } void ClassTable::InsertWithHash(ObjPtr klass, size_t hash) { @@ -136,8 +140,8 @@ void ClassTable::InsertWithHash(ObjPtr klass, size_t hash) { } bool ClassTable::Remove(const char* descriptor) { - WriterMutexLock mu(Thread::Current(), lock_); DescriptorHashPair pair(descriptor, ComputeModifiedUtf8Hash(descriptor)); + WriterMutexLock mu(Thread::Current(), lock_); for (ClassSet& class_set : classes_) { auto it = class_set.Find(pair); if (it != class_set.end()) { @@ -250,10 +254,12 @@ void ClassTable::ClearStrongRoots() { strong_roots_.clear(); } -ClassTable::TableSlot::TableSlot(ObjPtr klass) { +ClassTable::TableSlot::TableSlot(ObjPtr klass) + : TableSlot(klass, HashDescriptor(klass)) {} + +uint32_t ClassTable::TableSlot::HashDescriptor(ObjPtr klass) { std::string temp; - data_.StoreRelaxed(Encode(klass.Ptr(), - MaskHash(ComputeModifiedUtf8Hash(klass->GetDescriptor(&temp))))); + return ComputeModifiedUtf8Hash(klass->GetDescriptor(&temp)); } } // namespace art diff --git a/runtime/class_table.h b/runtime/class_table.h index 104871ff2..f27d8093c 100644 --- a/runtime/class_table.h +++ b/runtime/class_table.h @@ -73,6 +73,9 @@ class ClassTable { return MaskHash(other) == Hash(); } + static uint32_t HashDescriptor(ObjPtr klass) + REQUIRES_SHARED(Locks::mutator_lock_); + template mirror::Class* Read() const REQUIRES_SHARED(Locks::mutator_lock_); @@ -174,6 +177,10 @@ class ClassTable { bool Visit(Visitor& visitor) REQUIRES(!lock_) REQUIRES_SHARED(Locks::mutator_lock_); + template + bool Visit(const Visitor& visitor) + REQUIRES(!lock_) + REQUIRES_SHARED(Locks::mutator_lock_); // Return the first class that matches the descriptor. Returns null if there are none. mirror::Class* Lookup(const char* descriptor, size_t hash) diff --git a/runtime/class_table_test.cc b/runtime/class_table_test.cc new file mode 100644 index 000000000..f1248eb00 --- /dev/null +++ b/runtime/class_table_test.cc @@ -0,0 +1,163 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "class_table-inl.h" + +#include "art_field-inl.h" +#include "art_method-inl.h" +#include "class_linker-inl.h" +#include "common_runtime_test.h" +#include "dex_file.h" +#include "gc/accounting/card_table-inl.h" +#include "gc/heap.h" +#include "handle_scope-inl.h" +#include "mirror/class-inl.h" +#include "obj_ptr.h" +#include "scoped_thread_state_change-inl.h" + +namespace art { +namespace mirror { + +class CollectRootVisitor { + public: + CollectRootVisitor() {} + + template + ALWAYS_INLINE void VisitRootIfNonNull(GcRoot& root) const + REQUIRES_SHARED(Locks::mutator_lock_) { + if (!root.IsNull()) { + VisitRoot(root); + } + } + + template + ALWAYS_INLINE void VisitRootIfNonNull(mirror::CompressedReference* root) const + REQUIRES_SHARED(Locks::mutator_lock_) { + if (!root->IsNull()) { + VisitRoot(root); + } + } + + template + void VisitRoot(GcRoot& root) const REQUIRES_SHARED(Locks::mutator_lock_) { + VisitRoot(root.AddressWithoutBarrier()); + } + + template + void VisitRoot(mirror::CompressedReference* root) const + REQUIRES_SHARED(Locks::mutator_lock_) { + roots_.insert(root->AsMirrorPtr()); + } + + mutable std::set roots_; +}; + + +class ClassTableTest : public CommonRuntimeTest {}; + +TEST_F(ClassTableTest, ClassTable) { + ScopedObjectAccess soa(Thread::Current()); + jobject jclass_loader = LoadDex("XandY"); + VariableSizedHandleScope hs(soa.Self()); + Handle class_loader(hs.NewHandle(soa.Decode(jclass_loader))); + const char* descriptor_x = "LX;"; + const char* descriptor_y = "LY;"; + Handle h_X( + hs.NewHandle(class_linker_->FindClass(soa.Self(), descriptor_x, class_loader))); + Handle h_Y( + hs.NewHandle(class_linker_->FindClass(soa.Self(), descriptor_y, class_loader))); + Handle obj_X = hs.NewHandle(h_X->AllocObject(soa.Self())); + ASSERT_TRUE(obj_X.Get() != nullptr); + ClassTable table; + EXPECT_EQ(table.NumZygoteClasses(class_loader.Get()), 0u); + EXPECT_EQ(table.NumNonZygoteClasses(class_loader.Get()), 0u); + + // Add h_X to the class table. + table.Insert(h_X.Get()); + EXPECT_EQ(table.LookupByDescriptor(h_X.Get()), h_X.Get()); + EXPECT_EQ(table.Lookup(descriptor_x, ComputeModifiedUtf8Hash(descriptor_x)), h_X.Get()); + EXPECT_EQ(table.Lookup("NOT_THERE", ComputeModifiedUtf8Hash("NOT_THERE")), nullptr); + EXPECT_EQ(table.NumZygoteClasses(class_loader.Get()), 0u); + EXPECT_EQ(table.NumNonZygoteClasses(class_loader.Get()), 1u); + + // Create the zygote snapshot and ensure the accounting is correct. + table.FreezeSnapshot(); + EXPECT_EQ(table.NumZygoteClasses(class_loader.Get()), 1u); + EXPECT_EQ(table.NumNonZygoteClasses(class_loader.Get()), 0u); + + // Test inserting and related lookup functions. + EXPECT_EQ(table.LookupByDescriptor(h_Y.Get()), nullptr); + EXPECT_FALSE(table.Contains(h_Y.Get())); + table.Insert(h_Y.Get()); + EXPECT_EQ(table.LookupByDescriptor(h_X.Get()), h_X.Get()); + EXPECT_EQ(table.LookupByDescriptor(h_Y.Get()), h_Y.Get()); + EXPECT_TRUE(table.Contains(h_X.Get())); + EXPECT_TRUE(table.Contains(h_Y.Get())); + + EXPECT_EQ(table.NumZygoteClasses(class_loader.Get()), 1u); + EXPECT_EQ(table.NumNonZygoteClasses(class_loader.Get()), 1u); + + // Test adding / clearing strong roots. + EXPECT_TRUE(table.InsertStrongRoot(obj_X.Get())); + EXPECT_FALSE(table.InsertStrongRoot(obj_X.Get())); + table.ClearStrongRoots(); + EXPECT_TRUE(table.InsertStrongRoot(obj_X.Get())); + + // Collect all the roots and make sure there is nothing missing. + CollectRootVisitor roots; + table.VisitRoots(roots); + EXPECT_TRUE(roots.roots_.find(h_X.Get()) != roots.roots_.end()); + EXPECT_TRUE(roots.roots_.find(h_Y.Get()) != roots.roots_.end()); + EXPECT_TRUE(roots.roots_.find(obj_X.Get()) != roots.roots_.end()); + + // Checks that vising only classes works. + std::set classes; + table.Visit([&classes](ObjPtr klass) REQUIRES_SHARED(Locks::mutator_lock_) { + classes.insert(klass.Ptr()); + return true; + }); + EXPECT_TRUE(classes.find(h_X.Get()) != classes.end()); + EXPECT_TRUE(classes.find(h_Y.Get()) != classes.end()); + EXPECT_EQ(classes.size(), 2u); + classes.clear(); + table.Visit([&classes](ObjPtr klass) REQUIRES_SHARED(Locks::mutator_lock_) { + classes.insert(klass.Ptr()); + // Return false to exit the Visit early. + return false; + }); + EXPECT_EQ(classes.size(), 1u); + + // Test remove. + table.Remove(descriptor_x); + EXPECT_FALSE(table.Contains(h_X.Get())); + + // Test that WriteToMemory and ReadFromMemory work. + table.Insert(h_X.Get()); + const size_t count = table.WriteToMemory(nullptr); + std::unique_ptr buffer(new uint8_t[count]()); + ASSERT_EQ(table.WriteToMemory(&buffer[0]), count); + ClassTable table2; + size_t count2 = table2.ReadFromMemory(&buffer[0]); + EXPECT_EQ(count, count2); + // Strong roots are not serialized, only classes. + EXPECT_TRUE(table2.Contains(h_X.Get())); + EXPECT_TRUE(table2.Contains(h_Y.Get())); + + // TODO: Add tests for UpdateClass, InsertOatFile. +} + +} // namespace mirror +} // namespace art diff --git a/runtime/mirror/dex_cache-inl.h b/runtime/mirror/dex_cache-inl.h index b54e416bc..a59bb7b88 100644 --- a/runtime/mirror/dex_cache-inl.h +++ b/runtime/mirror/dex_cache-inl.h @@ -72,7 +72,7 @@ inline void DexCache::ClearString(dex::StringIndex string_idx) { inline Class* DexCache::GetResolvedType(dex::TypeIndex type_idx) { // It is theorized that a load acquire is not required since obtaining the resolved class will - // always have an address depedency or a lock. + // always have an address dependency or a lock. DCHECK_LT(type_idx.index_, NumResolvedTypes()); return GetResolvedTypes()[type_idx.index_].Read(); } -- 2.11.0