Simplify deleting startup dex cache arrays.

- No need to be in a GC critical section.
- Make sure the dex cache is in the expected state before registering
  it.

Also remove now unused ScopedInterruptibleGCCriticalSection

Test: test.py
Change-Id: I27b1209c7a21b811f9e17677e5e2bf70456aad8c
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 4d1b9f4..4ef0061 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -1681,6 +1681,7 @@
   Runtime* const runtime = Runtime::Current();
   gc::Heap* const heap = runtime->GetHeap();
   const ImageHeader& header = space->GetImageHeader();
+  int32_t number_of_dex_cache_arrays_cleared = 0;
   {
     // Register dex caches with the class loader.
     WriterMutexLock mu(self, *Locks::classlinker_classes_lock_);
@@ -1689,10 +1690,24 @@
       {
         WriterMutexLock mu2(self, *Locks::dex_lock_);
         CHECK(class_linker->FindDexCacheDataLocked(*dex_file) == nullptr);
+        if (runtime->GetStartupCompleted()) {
+          number_of_dex_cache_arrays_cleared++;
+          // Free up dex cache arrays that we would only allocate at startup.
+          // We do this here before registering and within the lock to be
+          // consistent with `StartupCompletedTask`.
+          dex_cache->UnlinkStartupCaches();
+        }
         class_linker->RegisterDexFileLocked(*dex_file, dex_cache, class_loader.Get());
       }
     }
   }
+  if (number_of_dex_cache_arrays_cleared == dex_caches->GetLength()) {
+    // Free up dex cache arrays that we would only allocate at startup.
+    // If `number_of_dex_cache_arrays_cleared` isn't the number of dex caches in
+    // the image, then there is a race with the `StartupCompletedTask`, which
+    // will release the space instead.
+    space->ReleaseMetadata();
+  }
 
   if (ClassLinker::kAppImageMayContainStrings) {
     HandleAppImageStrings(space);
@@ -1710,14 +1725,6 @@
       }
     }, space->Begin(), kRuntimePointerSize);
   }
-
-  if (runtime->GetStartupCompleted()) {
-    // Free up dex cache arrays that we would only allocate at startup.
-    for (auto dex_cache : dex_caches.Iterate<mirror::DexCache>()) {
-      dex_cache->UnlinkStartupCaches();
-    }
-    space->ReleaseMetadata();
-  }
 }
 
 void AppImageLoadingHelper::HandleAppImageStrings(gc::space::ImageSpace* space) {
diff --git a/runtime/gc/gc_cause.cc b/runtime/gc/gc_cause.cc
index b197a99..02fe2f9 100644
--- a/runtime/gc/gc_cause.cc
+++ b/runtime/gc/gc_cause.cc
@@ -46,7 +46,7 @@
     case kGcCauseHprof: return "Hprof";
     case kGcCauseGetObjectsAllocated: return "ObjectsAllocated";
     case kGcCauseProfileSaver: return "ProfileSaver";
-    case kGcCauseRunEmptyCheckpoint: return "RunEmptyCheckpoint";
+    case kGcCauseDeletingDexCacheArrays: return "DeletingDexCacheArrays";
   }
   LOG(FATAL) << "Unreachable";
   UNREACHABLE();
diff --git a/runtime/gc/gc_cause.h b/runtime/gc/gc_cause.h
index 4dae585..5c039b3 100644
--- a/runtime/gc/gc_cause.h
+++ b/runtime/gc/gc_cause.h
@@ -62,8 +62,8 @@
   kGcCauseGetObjectsAllocated,
   // GC cause for the profile saver.
   kGcCauseProfileSaver,
-  // GC cause for running an empty checkpoint.
-  kGcCauseRunEmptyCheckpoint,
+  // GC cause for deleting dex cache arrays at startup.
+  kGcCauseDeletingDexCacheArrays,
 };
 
 const char* PrettyCause(GcCause cause);
diff --git a/runtime/gc/scoped_gc_critical_section.cc b/runtime/gc/scoped_gc_critical_section.cc
index eaede43..7a0a6e8 100644
--- a/runtime/gc/scoped_gc_critical_section.cc
+++ b/runtime/gc/scoped_gc_critical_section.cc
@@ -58,17 +58,5 @@
   critical_section_.Exit(old_no_suspend_reason_);
 }
 
-ScopedInterruptibleGCCriticalSection::ScopedInterruptibleGCCriticalSection(
-    Thread* self,
-    GcCause cause,
-    CollectorType type) : self_(self) {
-  DCHECK(self != nullptr);
-  Runtime::Current()->GetHeap()->StartGC(self_, cause, type);
-}
-
-ScopedInterruptibleGCCriticalSection::~ScopedInterruptibleGCCriticalSection() {
-  Runtime::Current()->GetHeap()->FinishGC(self_, collector::kGcTypeNone);
-}
-
 }  // namespace gc
 }  // namespace art
diff --git a/runtime/gc/scoped_gc_critical_section.h b/runtime/gc/scoped_gc_critical_section.h
index b3a897c..8ad0158 100644
--- a/runtime/gc/scoped_gc_critical_section.h
+++ b/runtime/gc/scoped_gc_critical_section.h
@@ -59,19 +59,6 @@
   const char* old_no_suspend_reason_;
 };
 
-// The use of ScopedGCCriticalSection should be preferred whenever possible.
-// This class allows thread suspension but should never be used with allocations because of the
-// deadlock risk. TODO: Add a new thread role for "no allocations" that still allows suspension.
-class ScopedInterruptibleGCCriticalSection {
- public:
-  ScopedInterruptibleGCCriticalSection(Thread* self, GcCause cause, CollectorType type);
-  ~ScopedInterruptibleGCCriticalSection();
-
- private:
-  Thread* const self_;
-};
-
-
 }  // namespace gc
 }  // namespace art
 
diff --git a/runtime/mirror/dex_cache-inl.h b/runtime/mirror/dex_cache-inl.h
index ddfc6b2..8b8eecc 100644
--- a/runtime/mirror/dex_cache-inl.h
+++ b/runtime/mirror/dex_cache-inl.h
@@ -322,6 +322,9 @@
     visitor.VisitRootIfNonNull(resolved_call_sites->GetGcRootAddress(i)->AddressWithoutBarrier());
   }
 
+  // Dex cache arrays can be reset and cleared during app startup. Assert we do not get
+  // suspended to ensure the arrays are not deallocated.
+  ScopedAssertNoThreadSuspension soants("dex caches");
   GcRootArray<mirror::Class>* resolved_types = GetResolvedTypesArray<kVerifyFlags>();
   size_t num_resolved_types = NumResolvedTypesArray<kVerifyFlags>();
   for (size_t i = 0; resolved_types != nullptr && i != num_resolved_types; ++i) {
diff --git a/runtime/startup_completed_task.cc b/runtime/startup_completed_task.cc
index 649124e..9358d48 100644
--- a/runtime/startup_completed_task.cc
+++ b/runtime/startup_completed_task.cc
@@ -34,28 +34,13 @@
 
 namespace art {
 
-class CollectStartupDexCacheVisitor : public DexCacheVisitor {
+class UnlinkStartupDexCacheVisitor : public DexCacheVisitor {
  public:
-  explicit CollectStartupDexCacheVisitor(VariableSizedHandleScope& handles) : handles_(handles) {}
+  UnlinkStartupDexCacheVisitor() {}
 
   void Visit(ObjPtr<mirror::DexCache> dex_cache)
       REQUIRES_SHARED(Locks::dex_lock_, Locks::mutator_lock_) override {
-    handles_.NewHandle(dex_cache);
-  }
-
- private:
-  VariableSizedHandleScope& handles_;
-};
-
-class UnlinkVisitor {
- public:
-  UnlinkVisitor() {}
-
-  void VisitRootIfNonNull(StackReference<mirror::Object>* ref)
-      REQUIRES_SHARED(Locks::mutator_lock_) {
-    if (!ref->IsNull()) {
-      ref->AsMirrorPtr()->AsDexCache()->UnlinkStartupCaches();
-    }
+    dex_cache->UnlinkStartupCaches();
   }
 };
 
@@ -83,43 +68,31 @@
     }
   }
 
-  // Fetch the startup linear alloc before the checkpoint to play nice with
-  // 1002-notify-startup test which resets the startup state.
-  std::unique_ptr<LinearAlloc> startup_linear_alloc(runtime->ReleaseStartupLinearAlloc());
   {
     ScopedTrace trace("Releasing dex caches and app image spaces metadata");
     ScopedObjectAccess soa(Thread::Current());
 
-    // Collect dex caches that were allocated with the startup linear alloc.
-    VariableSizedHandleScope handles(soa.Self());
     {
-      CollectStartupDexCacheVisitor visitor(handles);
+      UnlinkStartupDexCacheVisitor visitor;
       ReaderMutexLock mu(self, *Locks::dex_lock_);
       runtime->GetClassLinker()->VisitDexCaches(&visitor);
     }
 
-    // Request empty checkpoints to make sure no threads are:
+    // Request a checkpoint to make sure no threads are:
     // - accessing the image space metadata section when we madvise it
     // - accessing dex caches when we free them
-    //
-    // Use GC exclusion to prevent deadlocks that may happen if
-    // multiple threads are attempting to run empty checkpoints at the same time.
-    {
-      // Avoid using ScopedGCCriticalSection since that does not allow thread suspension. This is
-      // not allowed to prevent allocations, but it's still safe to suspend temporarily for the
-      // checkpoint.
-      gc::ScopedInterruptibleGCCriticalSection sigcs(self,
-                                                     gc::kGcCauseRunEmptyCheckpoint,
-                                                     gc::kCollectorTypeCriticalSection);
-      // Do the unlinking of dex cache arrays in the GC critical section to
-      // avoid GC not seeing these arrays. We do it before the checkpoint so
-      // we know after the checkpoint, no thread is holding onto the array.
-      UnlinkVisitor visitor;
-      handles.VisitRoots(visitor);
+    static struct EmptyClosure : Closure {
+      void Run(Thread* thread ATTRIBUTE_UNUSED) override {}
+    } closure;
 
-      runtime->GetThreadList()->RunEmptyCheckpoint();
-    }
+    runtime->GetThreadList()->RunCheckpoint(&closure);
 
+    // Now delete dex cache arrays from both images and startup linear alloc in
+    // a critical section. The critical section is to ensure there is no
+    // possibility the GC can temporarily see those arrays.
+    gc::ScopedGCCriticalSection sgcs(soa.Self(),
+                                     gc::kGcCauseDeletingDexCacheArrays,
+                                     gc::kCollectorTypeCriticalSection);
     for (gc::space::ContinuousSpace* space : runtime->GetHeap()->GetContinuousSpaces()) {
       if (space->IsImageSpace()) {
         gc::space::ImageSpace* image_space = space->AsImageSpace();
@@ -128,22 +101,19 @@
         }
       }
     }
+
+    std::unique_ptr<LinearAlloc> startup_linear_alloc(runtime->ReleaseStartupLinearAlloc());
+    if (startup_linear_alloc != nullptr) {
+      ScopedTrace trace2("Delete startup linear alloc");
+      ArenaPool* arena_pool = startup_linear_alloc->GetArenaPool();
+      startup_linear_alloc.reset();
+      arena_pool->TrimMaps();
+    }
   }
 
-  {
-    // Delete the thread pool used for app image loading since startup is assumed to be completed.
-    ScopedTrace trace2("Delete thread pool");
-    runtime->DeleteThreadPool();
-  }
-
-  if (startup_linear_alloc != nullptr) {
-    // We know that after the checkpoint, there is no thread that can hold
-    // the startup linear alloc, so it's safe to delete it now.
-    ScopedTrace trace2("Delete startup linear alloc");
-    ArenaPool* arena_pool = startup_linear_alloc->GetArenaPool();
-    startup_linear_alloc.reset();
-    arena_pool->TrimMaps();
-  }
+  // Delete the thread pool used for app image loading since startup is assumed to be completed.
+  ScopedTrace trace2("Delete thread pool");
+  runtime->DeleteThreadPool();
 }
 
 }  // namespace art