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

2 files changed, 94 insertions, 33 deletions

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 0746e0c..f84600a 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<const size_t, GcRoot<mirror::Class> >& 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<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 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 {