From: Ian Rogers Date: Fri, 29 Aug 2014 22:40:08 +0000 (-0700) Subject: VisitClassesWithoutClassesLock isn't safe if classes move. X-Git-Tag: android-x86-7.1-r1~889^2~3171^2 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=dbf3be0f133c0bdf454f637fee2452dbb5f7c027;p=android-x86%2Fart.git 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 --- diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc index 0746e0ca0..f84600afa 100644 --- a/runtime/class_linker.cc +++ b/runtime/class_linker.cc @@ -1804,6 +1804,7 @@ void ClassLinker::VisitClasses(ClassVisitor* visitor, void* arg) { 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 >& it : class_table_) { mirror::Class* c = it.second.Read(); @@ -1813,18 +1814,75 @@ void ClassLinker::VisitClasses(ClassVisitor* visitor, void* arg) { } } -static bool GetClassesVisitor(mirror::Class* c, void* arg) { +static bool GetClassesVisitorSet(mirror::Class* c, void* arg) { std::set* classes = reinterpret_cast*>(arg); classes->insert(c); return true; } +struct GetClassesVisitorArrayArg { + Handle>* classes; + int32_t index; + bool success; +}; + +static bool GetClassesVisitorArray(mirror::Class* c, void* varg) + SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { + GetClassesVisitorArrayArg* arg = reinterpret_cast(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 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 classes; + VisitClasses(GetClassesVisitorSet, &classes); + for (mirror::Class* klass : classes) { + if (!visitor(klass, arg)) { + return; + } + } + } else { + Thread* self = Thread::Current(); + StackHandleScope<1> hs(self); + Handle> classes = + hs.NewHandle>(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::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 a7a68b754..7750c8ec4 100644 --- a/runtime/class_linker.h +++ b/runtime/class_linker.h @@ -237,12 +237,14 @@ class ClassLinker { } 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) @@ -623,29 +625,6 @@ class ClassLinker { ConstHandle prototype) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_); - std::vector boot_class_path_; - - mutable ReaderWriterMutex dex_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER; - std::vector new_dex_cache_roots_ GUARDED_BY(dex_lock_);; - std::vector> dex_caches_ GUARDED_BY(dex_lock_); - std::vector oat_files_ GUARDED_BY(dex_lock_); - - - // multimap from a string hash code of a class descriptor to - // mirror::Class* instances. Results should be compared for a matching - // Class::descriptor_ and Class::class_loader_. - typedef AllocationTrackingMultiMap, kAllocatorTagClassTable> Table; - // This contains strong roots. To enable concurrent root scanning of - // the class table, be careful to use a read barrier when accessing this. - Table class_table_ GUARDED_BY(Locks::classlinker_classes_lock_); - std::vector>> new_class_roots_; - - // Do we need to search dex caches to find image classes? - bool dex_cache_image_class_lookup_required_; - // Number of times we've searched dex caches for a class. After a certain number of misses we move - // the classes into the class_table_ to avoid dex cache based searches. - Atomic failed_dex_cache_class_lookups_; - mirror::Class* LookupClassFromTableLocked(const char* descriptor, const mirror::ClassLoader* class_loader, size_t hash) @@ -656,6 +635,7 @@ class ClassLinker { 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_); @@ -672,6 +652,29 @@ class ClassLinker { void FixupTemporaryDeclaringClass(mirror::Class* temp_class, mirror::Class* new_class) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_); + std::vector boot_class_path_; + + mutable ReaderWriterMutex dex_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER; + std::vector new_dex_cache_roots_ GUARDED_BY(dex_lock_);; + std::vector> dex_caches_ GUARDED_BY(dex_lock_); + std::vector oat_files_ GUARDED_BY(dex_lock_); + + + // multimap from a string hash code of a class descriptor to + // mirror::Class* instances. Results should be compared for a matching + // Class::descriptor_ and Class::class_loader_. + typedef AllocationTrackingMultiMap, kAllocatorTagClassTable> Table; + // This contains strong roots. To enable concurrent root scanning of + // the class table, be careful to use a read barrier when accessing this. + Table class_table_ GUARDED_BY(Locks::classlinker_classes_lock_); + std::vector>> new_class_roots_; + + // Do we need to search dex caches to find image classes? + bool dex_cache_image_class_lookup_required_; + // Number of times we've searched dex caches for a class. After a certain number of misses we move + // the classes into the class_table_ to avoid dex cache based searches. + Atomic failed_dex_cache_class_lookups_; + // indexes into class_roots_. // needs to be kept in sync with class_roots_descriptors_. enum ClassRoot {