Revert "Revert "Avoid adding region space bitmap to heap bitmap""
The issue was that hprof dumping could run in the middle of non CC
GC. This meant the allocation stack and live bitmap could both visit
the same object. The solution is to use a GC critical section.
Bug: 34967844
Test: test-art-host
This reverts commit 215835daf496f44b94b37eb89dd659f84e3ae44d.
Change-Id: I99e65ff31ece74aa94fc55cc7480e27c1e39661b
diff --git a/runtime/gc/collector_type.h b/runtime/gc/collector_type.h
index 7014357..eef4fba 100644
--- a/runtime/gc/collector_type.h
+++ b/runtime/gc/collector_type.h
@@ -55,6 +55,8 @@
kCollectorTypeClassLinker,
// JIT Code cache fake collector.
kCollectorTypeJitCodeCache,
+ // Hprof fake collector.
+ kCollectorTypeHprof,
// Fake collector for installing/removing a system-weak holder.
kCollectorTypeAddRemoveSystemWeakHolder,
};
diff --git a/runtime/gc/gc_cause.cc b/runtime/gc/gc_cause.cc
index 7ff845d..9e34346 100644
--- a/runtime/gc/gc_cause.cc
+++ b/runtime/gc/gc_cause.cc
@@ -39,6 +39,7 @@
case kGcCauseClassLinker: return "ClassLinker";
case kGcCauseJitCodeCache: return "JitCodeCache";
case kGcCauseAddRemoveSystemWeakHolder: return "SystemWeakHolder";
+ case kGcCauseHprof: return "Hprof";
}
LOG(FATAL) << "Unreachable";
UNREACHABLE();
diff --git a/runtime/gc/gc_cause.h b/runtime/gc/gc_cause.h
index f54f0e4..9b285b1 100644
--- a/runtime/gc/gc_cause.h
+++ b/runtime/gc/gc_cause.h
@@ -53,6 +53,8 @@
kGcCauseJitCodeCache,
// Not a real GC cause, used to add or remove system-weak holders.
kGcCauseAddRemoveSystemWeakHolder,
+ // Not a real GC cause, used to hprof running in the middle of GC.
+ kGcCauseHprof,
};
const char* PrettyCause(GcCause cause);
diff --git a/runtime/gc/heap.cc b/runtime/gc/heap.cc
index 0a45fce..a78de37 100644
--- a/runtime/gc/heap.cc
+++ b/runtime/gc/heap.cc
@@ -987,7 +987,9 @@
// Continuous spaces don't necessarily have bitmaps.
accounting::ContinuousSpaceBitmap* live_bitmap = continuous_space->GetLiveBitmap();
accounting::ContinuousSpaceBitmap* mark_bitmap = continuous_space->GetMarkBitmap();
- if (live_bitmap != nullptr) {
+ // The region space bitmap is not added since VisitObjects visits the region space objects with
+ // special handling.
+ if (live_bitmap != nullptr && !space->IsRegionSpace()) {
CHECK(mark_bitmap != nullptr);
live_bitmap_->AddContinuousSpaceBitmap(live_bitmap);
mark_bitmap_->AddContinuousSpaceBitmap(mark_bitmap);
@@ -1028,7 +1030,7 @@
// Continuous spaces don't necessarily have bitmaps.
accounting::ContinuousSpaceBitmap* live_bitmap = continuous_space->GetLiveBitmap();
accounting::ContinuousSpaceBitmap* mark_bitmap = continuous_space->GetMarkBitmap();
- if (live_bitmap != nullptr) {
+ if (live_bitmap != nullptr && !space->IsRegionSpace()) {
DCHECK(mark_bitmap != nullptr);
live_bitmap_->RemoveContinuousSpaceBitmap(live_bitmap);
mark_bitmap_->RemoveContinuousSpaceBitmap(mark_bitmap);
diff --git a/runtime/hprof/hprof.cc b/runtime/hprof/hprof.cc
index 133502e..e59c4bb 100644
--- a/runtime/hprof/hprof.cc
+++ b/runtime/hprof/hprof.cc
@@ -50,6 +50,7 @@
#include "gc_root.h"
#include "gc/accounting/heap_bitmap.h"
#include "gc/allocation_record.h"
+#include "gc/scoped_gc_critical_section.h"
#include "gc/heap.h"
#include "gc/space/space.h"
#include "globals.h"
@@ -463,6 +464,7 @@
}
bool okay;
+ visited_objects_.clear();
if (direct_to_ddms_) {
if (kDirectStream) {
okay = DumpToDdmsDirect(overall_size, max_length, CHUNK_TYPE("HPDS"));
@@ -911,6 +913,9 @@
// bits.
std::unordered_set<uint64_t> simple_roots_;
+ // To make sure we don't dump the same object multiple times. b/34967844
+ std::unordered_set<mirror::Object*> visited_objects_;
+
friend class GcRootVisitor;
DISALLOW_COPY_AND_ASSIGN(Hprof);
};
@@ -1093,6 +1098,7 @@
if (obj->IsClass() && obj->AsClass()->IsRetired()) {
return;
}
+ DCHECK(visited_objects_.insert(obj).second) << "Already visited " << obj;
++total_objects_;
@@ -1444,22 +1450,15 @@
// Otherwise, "filename" is used to create an output file.
void DumpHeap(const char* filename, int fd, bool direct_to_ddms) {
CHECK(filename != nullptr);
-
Thread* self = Thread::Current();
- gc::Heap* heap = Runtime::Current()->GetHeap();
- if (heap->IsGcConcurrentAndMoving()) {
- // Need to take a heap dump while GC isn't running. See the
- // comment in Heap::VisitObjects().
- heap->IncrementDisableMovingGC(self);
- }
- {
- ScopedSuspendAll ssa(__FUNCTION__, true /* long suspend */);
- Hprof hprof(filename, fd, direct_to_ddms);
- hprof.Dump();
- }
- if (heap->IsGcConcurrentAndMoving()) {
- heap->DecrementDisableMovingGC(self);
- }
+ // Need to take a heap dump while GC isn't running. See the comment in Heap::VisitObjects().
+ // Also we need the critical section to avoid visiting the same object twice. See b/34967844
+ gc::ScopedGCCriticalSection gcs(self,
+ gc::kGcCauseHprof,
+ gc::kCollectorTypeHprof);
+ ScopedSuspendAll ssa(__FUNCTION__, true /* long suspend */);
+ Hprof hprof(filename, fd, direct_to_ddms);
+ hprof.Dump();
}
} // namespace hprof
