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
diff --git a/build/Android.gtest.mk b/build/Android.gtest.mk
index 1691dbb..d59d8f6 100644
--- a/build/Android.gtest.mk
+++ b/build/Android.gtest.mk
@@ -86,6 +86,7 @@
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_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 08be5b2..32ebee2 100644
--- a/runtime/Android.bp
+++ b/runtime/Android.bp
@@ -517,6 +517,7 @@
"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 229cd47..dfe8949 100644
--- a/runtime/class_table-inl.h
+++ b/runtime/class_table-inl.h
@@ -71,6 +71,19 @@
return true;
}
+template <typename Visitor>
+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<ReadBarrierOption kReadBarrierOption>
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 ec33e5e..0f985c6 100644
--- a/runtime/class_table.cc
+++ b/runtime/class_table.cc
@@ -33,8 +33,9 @@
bool ClassTable::Contains(ObjPtr<mirror::Class> 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 @@
mirror::Class* ClassTable::LookupByDescriptor(ObjPtr<mirror::Class> 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 @@
}
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 @@
}
void ClassTable::Insert(ObjPtr<mirror::Class> 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<mirror::Class> klass) {
- classes_.back().Insert(TableSlot(klass));
+ const uint32_t hash = TableSlot::HashDescriptor(klass);
+ classes_.back().InsertWithHash(TableSlot(klass, hash), hash);
}
void ClassTable::InsertWithHash(ObjPtr<mirror::Class> klass, size_t hash) {
@@ -136,8 +140,8 @@
}
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 @@
strong_roots_.clear();
}
-ClassTable::TableSlot::TableSlot(ObjPtr<mirror::Class> klass) {
+ClassTable::TableSlot::TableSlot(ObjPtr<mirror::Class> klass)
+ : TableSlot(klass, HashDescriptor(klass)) {}
+
+uint32_t ClassTable::TableSlot::HashDescriptor(ObjPtr<mirror::Class> 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 104871f..f27d809 100644
--- a/runtime/class_table.h
+++ b/runtime/class_table.h
@@ -73,6 +73,9 @@
return MaskHash(other) == Hash();
}
+ static uint32_t HashDescriptor(ObjPtr<mirror::Class> klass)
+ REQUIRES_SHARED(Locks::mutator_lock_);
+
template<ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
mirror::Class* Read() const REQUIRES_SHARED(Locks::mutator_lock_);
@@ -174,6 +177,10 @@
bool Visit(Visitor& visitor)
REQUIRES(!lock_)
REQUIRES_SHARED(Locks::mutator_lock_);
+ template <typename Visitor>
+ 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 0000000..f1248eb
--- /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 <class MirrorType>
+ ALWAYS_INLINE void VisitRootIfNonNull(GcRoot<MirrorType>& root) const
+ REQUIRES_SHARED(Locks::mutator_lock_) {
+ if (!root.IsNull()) {
+ VisitRoot(root);
+ }
+ }
+
+ template <class MirrorType>
+ ALWAYS_INLINE void VisitRootIfNonNull(mirror::CompressedReference<MirrorType>* root) const
+ REQUIRES_SHARED(Locks::mutator_lock_) {
+ if (!root->IsNull()) {
+ VisitRoot(root);
+ }
+ }
+
+ template <class MirrorType>
+ void VisitRoot(GcRoot<MirrorType>& root) const REQUIRES_SHARED(Locks::mutator_lock_) {
+ VisitRoot(root.AddressWithoutBarrier());
+ }
+
+ template <class MirrorType>
+ void VisitRoot(mirror::CompressedReference<MirrorType>* root) const
+ REQUIRES_SHARED(Locks::mutator_lock_) {
+ roots_.insert(root->AsMirrorPtr());
+ }
+
+ mutable std::set<mirror::Object*> roots_;
+};
+
+
+class ClassTableTest : public CommonRuntimeTest {};
+
+TEST_F(ClassTableTest, ClassTable) {
+ ScopedObjectAccess soa(Thread::Current());
+ jobject jclass_loader = LoadDex("XandY");
+ VariableSizedHandleScope hs(soa.Self());
+ Handle<ClassLoader> class_loader(hs.NewHandle(soa.Decode<ClassLoader>(jclass_loader)));
+ const char* descriptor_x = "LX;";
+ const char* descriptor_y = "LY;";
+ Handle<mirror::Class> h_X(
+ hs.NewHandle(class_linker_->FindClass(soa.Self(), descriptor_x, class_loader)));
+ Handle<mirror::Class> h_Y(
+ hs.NewHandle(class_linker_->FindClass(soa.Self(), descriptor_y, class_loader)));
+ Handle<mirror::Object> 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<mirror::Class*> classes;
+ table.Visit([&classes](ObjPtr<mirror::Class> 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<mirror::Class> 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<uint8_t[]> 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 b54e416..a59bb7b 100644
--- a/runtime/mirror/dex_cache-inl.h
+++ b/runtime/mirror/dex_cache-inl.h
@@ -72,7 +72,7 @@
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();
}
