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

1 files changed, 30 insertions, 27 deletions

diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index a7a68b7..7750c8e 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<mirror::ArtMethod> prototype)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
-  std::vector<const DexFile*> boot_class_path_;
-
-  mutable ReaderWriterMutex dex_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
-  std::vector<size_t> new_dex_cache_roots_ GUARDED_BY(dex_lock_);;
-  std::vector<GcRoot<mirror::DexCache>> dex_caches_ GUARDED_BY(dex_lock_);
-  std::vector<const OatFile*> 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<size_t, GcRoot<mirror::Class>, 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<std::pair<size_t, GcRoot<mirror::Class>>> 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<uint32_t> 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<const DexFile*> boot_class_path_;
+
+  mutable ReaderWriterMutex dex_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
+  std::vector<size_t> new_dex_cache_roots_ GUARDED_BY(dex_lock_);;
+  std::vector<GcRoot<mirror::DexCache>> dex_caches_ GUARDED_BY(dex_lock_);
+  std::vector<const OatFile*> 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<size_t, GcRoot<mirror::Class>, 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<std::pair<size_t, GcRoot<mirror::Class>>> 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<uint32_t> failed_dex_cache_class_lookups_;
+
   // indexes into class_roots_.
   // needs to be kept in sync with class_roots_descriptors_.
   enum ClassRoot {