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