VisitClassesWithoutClassesLock isn't safe if classes move.
Which they do, so avoid by doing an array allocation.
Also, tidy member variables to the end of ClassLinker.
Remove unnecessary mutable. Tidy and fix a locks required/excluded.
Change-Id: I2404a9e7a1ea997d68ab1206f97d2a20dffbda06
(cherry picked from commit dbf3be0f133c0bdf454f637fee2452dbb5f7c027)
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 0d0c6ca..f2a208a 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -1747,6 +1747,7 @@
if (dex_cache_image_class_lookup_required_) {
MoveImageClassesToClassTable();
}
+ // TODO: why isn't this a ReaderMutexLock?
WriterMutexLock mu(Thread::Current(), *Locks::classlinker_classes_lock_);
for (std::pair<const size_t, GcRoot<mirror::Class> >& it : class_table_) {
mirror::Class* c = it.second.Read();
@@ -1756,18 +1757,75 @@
}
}
-static bool GetClassesVisitor(mirror::Class* c, void* arg) {
+static bool GetClassesVisitorSet(mirror::Class* c, void* arg) {
std::set<mirror::Class*>* classes = reinterpret_cast<std::set<mirror::Class*>*>(arg);
classes->insert(c);
return true;
}
+struct GetClassesVisitorArrayArg {
+ Handle<mirror::ObjectArray<mirror::Class>>* classes;
+ int32_t index;
+ bool success;
+};
+
+static bool GetClassesVisitorArray(mirror::Class* c, void* varg)
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+ GetClassesVisitorArrayArg* arg = reinterpret_cast<GetClassesVisitorArrayArg*>(varg);
+ if (arg->index < (*arg->classes)->GetLength()) {
+ (*arg->classes)->Set(arg->index, c);
+ arg->index++;
+ return true;
+ } else {
+ arg->success = false;
+ return false;
+ }
+}
+
void ClassLinker::VisitClassesWithoutClassesLock(ClassVisitor* visitor, void* arg) {
- std::set<mirror::Class*> classes;
- VisitClasses(GetClassesVisitor, &classes);
- for (mirror::Class* klass : classes) {
- if (!visitor(klass, arg)) {
- return;
+ // TODO: it may be possible to avoid secondary storage if we iterate over dex caches. The problem
+ // is avoiding duplicates.
+ if (!kMovingClasses) {
+ std::set<mirror::Class*> classes;
+ VisitClasses(GetClassesVisitorSet, &classes);
+ for (mirror::Class* klass : classes) {
+ if (!visitor(klass, arg)) {
+ return;
+ }
+ }
+ } else {
+ Thread* self = Thread::Current();
+ StackHandleScope<1> hs(self);
+ Handle<mirror::ObjectArray<mirror::Class>> classes =
+ hs.NewHandle<mirror::ObjectArray<mirror::Class>>(nullptr);
+ GetClassesVisitorArrayArg local_arg;
+ local_arg.classes = &classes;
+ local_arg.success = false;
+ // We size the array assuming classes won't be added to the class table during the visit.
+ // If this assumption fails we iterate again.
+ while (!local_arg.success) {
+ size_t class_table_size;
+ {
+ ReaderMutexLock mu(Thread::Current(), *Locks::classlinker_classes_lock_);
+ class_table_size = class_table_.size();
+ }
+ mirror::Class* class_type = mirror::Class::GetJavaLangClass();
+ mirror::Class* array_of_class = FindArrayClass(self, &class_type);
+ classes.Assign(
+ mirror::ObjectArray<mirror::Class>::Alloc(self, array_of_class, class_table_size));
+ CHECK(classes.Get() != nullptr); // OOME.
+ local_arg.index = 0;
+ local_arg.success = true;
+ VisitClasses(GetClassesVisitorArray, &local_arg);
+ }
+ for (int32_t i = 0; i < classes->GetLength(); ++i) {
+ // If the class table shrank during creation of the clases array we expect null elements. If
+ // the class table grew then the loop repeats. If classes are created after the loop has
+ // finished then we don't visit.
+ mirror::Class* klass = classes->Get(i);
+ if (klass != nullptr && !visitor(klass, arg)) {
+ return;
+ }
}
}
}
diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index 398ddd5..79cbb02 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -235,12 +235,14 @@
}
void VisitClasses(ClassVisitor* visitor, void* arg)
- LOCKS_EXCLUDED(dex_lock_)
+ LOCKS_EXCLUDED(Locks::classlinker_classes_lock_)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
- // Less efficient variant of VisitClasses that doesn't hold the classlinker_classes_lock_
- // when calling the visitor.
+
+ // Less efficient variant of VisitClasses that copies the class_table_ into secondary storage
+ // so that it can visit individual classes without holding the doesn't hold the
+ // Locks::classlinker_classes_lock_. As the Locks::classlinker_classes_lock_ isn't held this code
+ // can race with insertion and deletion of classes while the visitor is being called.
void VisitClassesWithoutClassesLock(ClassVisitor* visitor, void* arg)
- LOCKS_EXCLUDED(dex_lock_)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
void VisitClassRoots(RootCallback* callback, void* arg, VisitRootFlags flags)
@@ -624,6 +626,33 @@
Handle<mirror::ArtMethod> prototype)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+ mirror::Class* LookupClassFromTableLocked(const char* descriptor,
+ const mirror::ClassLoader* class_loader,
+ size_t hash)
+ SHARED_LOCKS_REQUIRED(Locks::classlinker_classes_lock_, Locks::mutator_lock_);
+
+ mirror::Class* UpdateClass(const char* descriptor, mirror::Class* klass, size_t hash)
+ LOCKS_EXCLUDED(Locks::classlinker_classes_lock_)
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+
+ void MoveImageClassesToClassTable() LOCKS_EXCLUDED(Locks::classlinker_classes_lock_)
+ LOCKS_EXCLUDED(Locks::classlinker_classes_lock_)
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+ mirror::Class* LookupClassFromImage(const char* descriptor)
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+
+ // EnsureResolved is called to make sure that a class in the class_table_ has been resolved
+ // before returning it to the caller. Its the responsibility of the thread that placed the class
+ // in the table to make it resolved. The thread doing resolution must notify on the class' lock
+ // when resolution has occurred. This happens in mirror::Class::SetStatus. As resolution may
+ // retire a class, the version of the class in the table is returned and this may differ from
+ // the class passed in.
+ mirror::Class* EnsureResolved(Thread* self, const char* descriptor, mirror::Class* klass)
+ WARN_UNUSED SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+
+ void FixupTemporaryDeclaringClass(mirror::Class* temp_class, mirror::Class* new_class)
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+
std::vector<const DexFile*> boot_class_path_;
mutable ReaderWriterMutex dex_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
@@ -647,32 +676,6 @@
// the classes into the class_table_ to avoid dex cache based searches.
Atomic<uint32_t> failed_dex_cache_class_lookups_;
- mirror::Class* LookupClassFromTableLocked(const char* descriptor,
- const mirror::ClassLoader* class_loader,
- size_t hash)
- SHARED_LOCKS_REQUIRED(Locks::classlinker_classes_lock_, Locks::mutator_lock_);
-
- mirror::Class* UpdateClass(const char* descriptor, mirror::Class* klass, size_t hash)
- LOCKS_EXCLUDED(Locks::classlinker_classes_lock_)
- SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-
- void MoveImageClassesToClassTable() LOCKS_EXCLUDED(Locks::classlinker_classes_lock_)
- SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
- mirror::Class* LookupClassFromImage(const char* descriptor)
- SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-
- // EnsureResolved is called to make sure that a class in the class_table_ has been resolved
- // before returning it to the caller. Its the responsibility of the thread that placed the class
- // in the table to make it resolved. The thread doing resolution must notify on the class' lock
- // when resolution has occurred. This happens in mirror::Class::SetStatus. As resolution may
- // retire a class, the version of the class in the table is returned and this may differ from
- // the class passed in.
- mirror::Class* EnsureResolved(Thread* self, const char* descriptor, mirror::Class* klass)
- WARN_UNUSED SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-
- void FixupTemporaryDeclaringClass(mirror::Class* temp_class, mirror::Class* new_class)
- SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-
// indexes into class_roots_.
// needs to be kept in sync with class_roots_descriptors_.
enum ClassRoot {
