From d8117e1b08f1d034d07d7e450eba00a83f4698c6 Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Mon, 7 Jul 2025 17:02:17 -0700 Subject: [PATCH] [vm] Finer labeling of roots in heap snapshots. Add missing object id zone roots, which should have been part of ffbbdb5a10a73d598db54823bcc1ce3feda59d07 when they switched from weak to strong. TEST=examine snapshot after using inspect Bug: https://github.com/dart-lang/sdk/issues/61036 Change-Id: I3bea765e4ae487babfd86eccbaa87bab80320dcf Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/439160 Reviewed-by: Alexander Aprelev Commit-Queue: Ryan Macnak --- runtime/vm/heap/become.cc | 9 -- runtime/vm/heap/compactor.cc | 13 --- runtime/vm/heap/incremental_compactor.cc | 13 --- runtime/vm/heap/marker.cc | 10 +- runtime/vm/heap/scavenger.cc | 17 +-- runtime/vm/heap/scavenger.h | 1 - runtime/vm/isolate.cc | 135 ++++++++++++++--------- runtime/vm/isolate.h | 59 +++++++++- runtime/vm/object_graph.cc | 52 ++++++--- runtime/vm/object_id_ring.cc | 5 +- runtime/vm/service.cc | 4 +- runtime/vm/service.h | 4 +- runtime/vm/visitor.h | 1 + 13 files changed, 193 insertions(+), 130 deletions(-) diff --git a/runtime/vm/heap/become.cc b/runtime/vm/heap/become.cc index 0c0a187fe15..6c685e5355b 100644 --- a/runtime/vm/heap/become.cc +++ b/runtime/vm/heap/become.cc @@ -365,15 +365,6 @@ void Become::FollowForwardingPointers(Thread* thread) { // C++ pointers. isolate_group->VisitObjectPointers(&pointer_visitor, ValidationPolicy::kValidateFrames); -#ifndef PRODUCT - isolate_group->ForEachIsolate( - [&](Isolate* isolate) { - for (intptr_t i = 0; i < isolate->NumServiceIdZones(); ++i) { - isolate->GetServiceIdZone(i)->VisitPointers(pointer_visitor); - } - }, - /*at_safepoint=*/true); -#endif // !PRODUCT // Weak persistent handles. ForwardHeapPointersHandleVisitor handle_visitor; diff --git a/runtime/vm/heap/compactor.cc b/runtime/vm/heap/compactor.cc index be5af61b60d..89400538dfd 100644 --- a/runtime/vm/heap/compactor.cc +++ b/runtime/vm/heap/compactor.cc @@ -469,19 +469,6 @@ void CompactorTask::RunEnteredIsolateGroup() { isolate_group_->VisitWeakPersistentHandles(compactor_); break; } -#ifndef PRODUCT - case 4: { - TIMELINE_FUNCTION_GC_DURATION(thread, "ForwardObjectIdRing"); - isolate_group_->ForEachIsolate( - [&](Isolate* isolate) { - for (intptr_t i = 0; i < isolate->NumServiceIdZones(); ++i) { - isolate->GetServiceIdZone(i)->VisitPointers(*compactor_); - } - }, - /*at_safepoint=*/true); - break; - } -#endif // !PRODUCT default: more_forwarding_tasks = false; } diff --git a/runtime/vm/heap/incremental_compactor.cc b/runtime/vm/heap/incremental_compactor.cc index 88df923157e..621ca9b39c7 100644 --- a/runtime/vm/heap/incremental_compactor.cc +++ b/runtime/vm/heap/incremental_compactor.cc @@ -574,7 +574,6 @@ class EpilogueState { bool TakeOOM() { return oom_slice_.exchange(false); } bool TakeWeakHandles() { return weak_handles_slice_.exchange(false); } bool TakeWeakTables() { return weak_tables_slice_.exchange(false); } - bool TakeIdRing() { return id_ring_slice_.exchange(false); } bool TakeRoots() { return roots_slice_.exchange(false); } bool TakeResetProgressBars() { return reset_progress_bars_slice_.exchange(false); @@ -634,18 +633,6 @@ class EpilogueTask : public SafepointTask { TIMELINE_FUNCTION_GC_DURATION(thread, "WeakTables"); isolate_group_->heap()->ForwardWeakTables(&visitor); } -#ifndef PRODUCT - if (state_->TakeIdRing()) { - TIMELINE_FUNCTION_GC_DURATION(thread, "IdRing"); - isolate_group_->ForEachIsolate( - [&](Isolate* isolate) { - for (intptr_t i = 0; i < isolate->NumServiceIdZones(); ++i) { - isolate->GetServiceIdZone(i)->VisitPointers(visitor); - } - }, - /*at_safepoint=*/true); - } -#endif // !PRODUCT barrier_->Sync(); diff --git a/runtime/vm/heap/marker.cc b/runtime/vm/heap/marker.cc index f9c9ebf33c5..0c99d6ae9aa 100644 --- a/runtime/vm/heap/marker.cc +++ b/runtime/vm/heap/marker.cc @@ -751,8 +751,7 @@ void GCMarker::Epilogue() {} enum RootSlices { kIsolate = 0, - kObjectIdRing = 1, - kNumFixedRootSlices = 2, + kNumFixedRootSlices = 1, }; void GCMarker::ResetSlices() { @@ -774,18 +773,13 @@ void GCMarker::IterateRoots(ObjectPointerVisitor* visitor) { switch (slice) { case kIsolate: { + // TODO(gc): Split this by isolate? TIMELINE_FUNCTION_GC_DURATION(Thread::Current(), "ProcessIsolateGroupRoots"); isolate_group_->VisitObjectPointers( visitor, ValidationPolicy::kDontValidateFrames); break; } - case kObjectIdRing: { - TIMELINE_FUNCTION_GC_DURATION(Thread::Current(), - "ProcessObjectIdTable"); - isolate_group_->VisitPointersInAllServiceIdZones(*visitor); - break; - } } MonitorLocker ml(&root_slices_monitor_); diff --git a/runtime/vm/heap/scavenger.cc b/runtime/vm/heap/scavenger.cc index e92dde88d2a..7636cd73b65 100644 --- a/runtime/vm/heap/scavenger.cc +++ b/runtime/vm/heap/scavenger.cc @@ -1192,33 +1192,22 @@ void Scavenger::IterateRememberedCards(ScavengerVisitor* visitor) { heap_->old_space()->VisitRememberedCards(visitor); } -void Scavenger::IterateObjectIdTable(ObjectPointerVisitor* visitor) { -#ifndef PRODUCT - TIMELINE_FUNCTION_GC_DURATION(Thread::Current(), "IterateObjectIdTable"); - heap_->isolate_group()->VisitPointersInAllServiceIdZones(*visitor); -#endif // !PRODUCT -} - enum RootSlices { kIsolate = 0, - kObjectIdRing, - kNumRootSlices, + kNumFixedRootSlices = 1, }; void Scavenger::IterateRoots(ScavengerVisitor* visitor) { for (;;) { intptr_t slice = root_slices_started_.fetch_add(1); - if (slice >= kNumRootSlices) { + if (slice >= kNumFixedRootSlices) { break; // No more slices. } - switch (slice) { case kIsolate: + // TODO(gc): Split this by isolate? IterateIsolateRoots(visitor); break; - case kObjectIdRing: - IterateObjectIdTable(visitor); - break; default: UNREACHABLE(); } diff --git a/runtime/vm/heap/scavenger.h b/runtime/vm/heap/scavenger.h index 2dc6a31f2a4..33a8fb49273 100644 --- a/runtime/vm/heap/scavenger.h +++ b/runtime/vm/heap/scavenger.h @@ -281,7 +281,6 @@ class Scavenger { void IterateIsolateRoots(ObjectPointerVisitor* visitor); void IterateStoreBuffers(ScavengerVisitor* visitor); void IterateRememberedCards(ScavengerVisitor* visitor); - void IterateObjectIdTable(ObjectPointerVisitor* visitor); void IterateRoots(ScavengerVisitor* visitor); void IterateWeak(); void MournWeakHandles(); diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index a5281a69358..35a913b57c8 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -2935,53 +2935,95 @@ void IsolateGroup::VisitObjectPointers(ObjectPointerVisitor* visitor, VisitStackPointers(visitor, validate_frames); } -void IsolateGroup::VisitSharedPointers(ObjectPointerVisitor* visitor) { - // Visit objects in the class table. - class_table()->VisitObjectPointers(visitor); - if (heap_walk_class_table() != class_table()) { - heap_walk_class_table()->VisitObjectPointers(visitor); - } - api_state()->VisitObjectPointersUnlocked(visitor); - // Visit objects in the object store. - if (object_store() != nullptr) { - object_store()->VisitObjectPointers(visitor); - } - visitor->VisitPointer(reinterpret_cast(&saved_unlinked_calls_)); - initial_field_table()->VisitObjectPointers(visitor); - sentinel_field_table()->VisitObjectPointers(visitor); - shared_initial_field_table()->VisitObjectPointers(visitor); - shared_field_table()->VisitObjectPointers(visitor); - - // Visit the boxed_field_list_. - // 'boxed_field_list_' access via mutator and background compilation threads - // is guarded with a monitor. This means that we can visit it only - // when at safepoint or the field_list_mutex_ lock has been taken. - visitor->VisitPointer(reinterpret_cast(&boxed_field_list_)); - - NOT_IN_PRECOMPILED(background_compiler()->VisitPointers(visitor)); - +void IsolateGroup::VisitSharedPointers(ObjectPointerVisitor* visitor, + intptr_t slice) { + switch (slice) { + case kClassTable: + class_table()->VisitObjectPointers(visitor); + if (heap_walk_class_table() != class_table()) { + heap_walk_class_table()->VisitObjectPointers(visitor); + } + break; + case kApiState: + api_state()->VisitObjectPointersUnlocked(visitor); + break; + case kObjectStore: + if (object_store() != nullptr) { + object_store()->VisitObjectPointers(visitor); + } + break; + case kSavedUnlinkedCalls: + visitor->VisitPointer( + reinterpret_cast(&saved_unlinked_calls_)); + break; + case kInitialFieldTable: + initial_field_table()->VisitObjectPointers(visitor); + break; + case kSentinelFieldTable: + sentinel_field_table()->VisitObjectPointers(visitor); + break; + case kSharedInitialFieldTable: + shared_initial_field_table()->VisitObjectPointers(visitor); + break; + case kSharedFieldTable: + shared_field_table()->VisitObjectPointers(visitor); + break; + case kBoxedFieldList: + // Visit the boxed_field_list_. + // 'boxed_field_list_' access via mutator and background compilation + // threads is guarded with a monitor. This means that we can visit it only + // when at safepoint or the field_list_mutex_ lock has been taken. + visitor->VisitPointer(reinterpret_cast(&boxed_field_list_)); + break; + case kBackgroundCompiler: + NOT_IN_PRECOMPILED(background_compiler()->VisitPointers(visitor)); + break; + case kDebugger: #if !defined(PRODUCT) - if (debugger() != nullptr) { - debugger()->VisitObjectPointers(visitor); - } + if (debugger() != nullptr) { + debugger()->VisitObjectPointers(visitor); + } #endif - + break; + case kReloadContext: #if !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME) - // Visit objects that are being used for isolate reload. - if (program_reload_context() != nullptr) { - program_reload_context()->VisitObjectPointers(visitor); - program_reload_context()->group_reload_context()->VisitObjectPointers( - visitor); - } + if (program_reload_context() != nullptr) { + program_reload_context()->VisitObjectPointers(visitor); + program_reload_context()->group_reload_context()->VisitObjectPointers( + visitor); + } #endif // !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME) - - if (source()->loaded_blobs_ != nullptr) { - visitor->VisitPointer( - reinterpret_cast(&(source()->loaded_blobs_))); + break; + case kLoadedBlobs: + if (source()->loaded_blobs_ != nullptr) { + visitor->VisitPointer( + reinterpret_cast(&(source()->loaded_blobs_))); + } + break; + case kBecome: + if (become() != nullptr) { + become()->VisitObjectPointers(visitor); + } + break; + case kObjectIdZones: +#if !defined(PRODUCT) + if (visitor->trace_object_id_rings()) { + for (Isolate* isolate : isolates_) { + for (intptr_t i = 0; i < isolate->NumServiceIdZones(); ++i) { + isolate->GetServiceIdZone(i)->VisitPointers(visitor); + } + } + } +#endif // !defined(PRODUCT) + break; + default: + UNREACHABLE(); } +} - if (become() != nullptr) { - become()->VisitObjectPointers(visitor); +void IsolateGroup::VisitSharedPointers(ObjectPointerVisitor* visitor) { + for (intptr_t i = 0; i < kNumRootSlices; i++) { + VisitSharedPointers(visitor, static_cast(i)); } } @@ -3002,17 +3044,6 @@ void IsolateGroup::VisitStackPointers(ObjectPointerVisitor* visitor, visitor->clear_gc_root_type(); } -void IsolateGroup::VisitPointersInAllServiceIdZones( - ObjectPointerVisitor& visitor) { -#if !defined(PRODUCT) - for (Isolate* isolate : isolates_) { - for (intptr_t i = 0; i < isolate->NumServiceIdZones(); ++i) { - isolate->GetServiceIdZone(i)->VisitPointers(visitor); - } - } -#endif // !defined(PRODUCT) -} - void IsolateGroup::VisitWeakPersistentHandles(HandleVisitor* visitor) { api_state()->VisitWeakHandlesUnlocked(visitor); } diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index c3842274907..2df15a07321 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -262,6 +262,63 @@ class MutatorThreadPool : public ThreadPool { IsolateGroup* isolate_group_ = nullptr; }; +enum RootSlice : intptr_t { + kClassTable, + kApiState, + kObjectStore, + kSavedUnlinkedCalls, + kInitialFieldTable, + kSentinelFieldTable, + kSharedInitialFieldTable, + kSharedFieldTable, + kBoxedFieldList, + kBackgroundCompiler, + kDebugger, + kReloadContext, + kLoadedBlobs, + kBecome, + kObjectIdZones, + + kNumRootSlices, +}; + +inline const char* RootSliceToCString(intptr_t slice) { + switch (slice) { + case kClassTable: + return "class table"; + case kApiState: + return "api state"; + case kObjectStore: + return "group object store"; + case kSavedUnlinkedCalls: + return "saved unlinked calls"; + case kInitialFieldTable: + return "initial field table"; + case kSentinelFieldTable: + return "sentinel field table"; + case kSharedInitialFieldTable: + return "shared initial field table"; + case kSharedFieldTable: + return "shared field table"; + case kBoxedFieldList: + return "boxed field list"; + case kBackgroundCompiler: + return "background compiler"; + case kDebugger: + return "debugger"; + case kReloadContext: + return "reload context"; + case kLoadedBlobs: + return "loaded blobs"; + case kBecome: + return "become"; + case kObjectIdZones: + return "object id zones"; + default: + return "?"; + } +} + // Represents an isolate group and is shared among all isolates within a group. class IsolateGroup : public IntrusiveDListEntry { public: @@ -692,9 +749,9 @@ class IsolateGroup : public IntrusiveDListEntry { void VisitObjectPointers(ObjectPointerVisitor* visitor, ValidationPolicy validate_frames); void VisitSharedPointers(ObjectPointerVisitor* visitor); + void VisitSharedPointers(ObjectPointerVisitor* visitor, intptr_t slice); void VisitStackPointers(ObjectPointerVisitor* visitor, ValidationPolicy validate_frames); - void VisitPointersInAllServiceIdZones(ObjectPointerVisitor& visitor); void VisitWeakPersistentHandles(HandleVisitor* visitor); // In precompilation we finalize all regular classes before compiling. diff --git a/runtime/vm/object_graph.cc b/runtime/vm/object_graph.cc index 283cc867540..2175579b7cd 100644 --- a/runtime/vm/object_graph.cc +++ b/runtime/vm/object_graph.cc @@ -184,6 +184,7 @@ class ObjectGraph::Stack : public ObjectPointerVisitor { } bool trace_values_through_fields() const override { return true; } + bool trace_object_id_rings() const override { return false; } // Marks and pushes. Used to initialize this stack with roots. // We can use ObjectIdTable normally used by serializers because it @@ -660,6 +661,7 @@ class InboundReferencesVisitor : public ObjectVisitor, } bool trace_values_through_fields() const override { return true; } + bool trace_object_id_rings() const override { return false; } intptr_t length() const { return length_; } @@ -1106,9 +1108,10 @@ static constexpr intptr_t kMaxStringElements = 128; enum ExtraCids { kRootExtraCid = 1, // 1-origin kImagePageExtraCid = 2, - kIsolateExtraCid = 3, + kRootSliceExtraCid = 3, + kIsolateExtraCid = 4, - kNumExtraCids = 3, + kNumExtraCids = 4, }; class Pass2Visitor : public ObjectVisitor, @@ -1552,7 +1555,16 @@ void HeapSnapshotWriter::Write() { WriteUnsigned(0); // Field count } { - ASSERT(kIsolateExtraCid == 3); + ASSERT(kRootSliceExtraCid == 3); + WriteUnsigned(0); // Flags + WriteUtf8("Root slice"); // Name + WriteUtf8(""); // Library name + WriteUtf8(""); // Library uri + WriteUtf8(""); // Reserved + WriteUnsigned(0); // Field count + } + { + ASSERT(kIsolateExtraCid == 4); WriteUnsigned(0); // Flags WriteUtf8("Isolate"); // Name WriteUtf8(""); // Library name @@ -1570,7 +1582,6 @@ void HeapSnapshotWriter::Write() { } } - ASSERT(kNumExtraCids == 3); for (intptr_t cid = 1; cid <= class_count_; cid++) { if (!class_table->HasValidClassAt(cid)) { WriteUnsigned(0); // Flags @@ -1626,7 +1637,6 @@ void HeapSnapshotWriter::Write() { // Root "objects". { ++object_count_; - isolate_group()->VisitSharedPointers(&visitor); } { ++object_count_; @@ -1635,6 +1645,10 @@ void HeapSnapshotWriter::Write() { num_image_objects = visitor.count(); CountReferences(num_image_objects); } + for (intptr_t i = 0; i < kNumRootSlices; i++) { + ++object_count_; + isolate_group()->VisitSharedPointers(&visitor, static_cast(i)); + } { isolate_group()->ForEachIsolate( [&](Isolate* isolate) { @@ -1647,8 +1661,9 @@ void HeapSnapshotWriter::Write() { }, /*at_safepoint=*/true); } - CountReferences(1); // Root -> Image Pages - CountReferences(num_isolates); // Root -> Isolate + CountReferences(1); // Root -> Image Pages + CountReferences(kNumRootSlices); // Root -> Root slices + CountReferences(num_isolates); // Root -> Isolate // Heap objects. iteration.IterateVMIsolateObjects(&visitor); @@ -1675,14 +1690,12 @@ void HeapSnapshotWriter::Write() { WriteUnsigned(0); // shallowSize WriteUnsigned(kNoData); visitor.DoCount(); - isolate_group()->VisitSharedPointers(&visitor); - visitor.CountExtraRefs(num_isolates + 1); + visitor.CountExtraRefs(1 + kNumRootSlices + num_isolates); visitor.DoWrite(); - isolate_group()->VisitSharedPointers(&visitor); visitor.WriteExtraRef(2); // Root -> Image Pages - for (intptr_t i = 0; i < num_isolates; i++) { - // 0 = sentinel, 1 = root, 2 = image pages, 2+ = isolates - visitor.WriteExtraRef(i + 3); + for (intptr_t i = 0; i < num_isolates + kNumRootSlices; i++) { + // 0 = sentinel, 1 = root, 2 = image pages, 3+ = slices/isolates + visitor.WriteExtraRef(3 + i); } } { @@ -1694,6 +1707,16 @@ void HeapSnapshotWriter::Write() { H->old_space()->VisitObjectsImagePages(&visitor); DEBUG_ASSERT(visitor.count() == num_image_objects); } + for (intptr_t i = 0; i < kNumRootSlices; i++) { + WriteUnsigned(kRootSliceExtraCid); + WriteUnsigned(0); // shallowSize + WriteUnsigned(kNameData); + WriteUtf8(RootSliceToCString(i)); + visitor.DoCount(); + isolate_group()->VisitSharedPointers(&visitor, i); + visitor.DoWrite(); + isolate_group()->VisitSharedPointers(&visitor, i); + } isolate_group()->ForEachIsolate( [&](Isolate* isolate) { WriteUnsigned(kIsolateExtraCid); @@ -1741,6 +1764,9 @@ void HeapSnapshotWriter::Write() { WriteUnsigned(0); // Root fake object. WriteUnsigned(0); // Image pages fake object. + for (intptr_t i = 0; i < kNumRootSlices; i++) { + WriteUnsigned(0); // Root slice fake object. + } isolate_group()->ForEachIsolate( [&](Isolate* isolate) { WriteUnsigned(0); // Isolate fake object. diff --git a/runtime/vm/object_id_ring.cc b/runtime/vm/object_id_ring.cc index 5f08aa4693b..c7c79182de5 100644 --- a/runtime/vm/object_id_ring.cc +++ b/runtime/vm/object_id_ring.cc @@ -68,8 +68,9 @@ ObjectPtr ObjectIdRing::GetObjectForId(int32_t id, LookupResult* kind) { } void ObjectIdRing::VisitPointers(ObjectPointerVisitor* visitor) const { - ASSERT(table_ != nullptr); - visitor->VisitPointers(table_, capacity_); + if (table_ != nullptr) { + visitor->VisitPointers(table_, capacity_); + } } void ObjectIdRing::PrintJSON(JSONStream* js) { diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc index 1276b5daee1..8a819d66177 100644 --- a/runtime/vm/service.cc +++ b/runtime/vm/service.cc @@ -446,8 +446,8 @@ void RingServiceIdZone::Invalidate() { ring_.Invalidate(); } -void RingServiceIdZone::VisitPointers(ObjectPointerVisitor& visitor) const { - ring_.VisitPointers(&visitor); +void RingServiceIdZone::VisitPointers(ObjectPointerVisitor* visitor) const { + ring_.VisitPointers(visitor); } void RingServiceIdZone::PrintJSON(JSONStream& js) const { diff --git a/runtime/vm/service.h b/runtime/vm/service.h index beffbd026d7..a7428bf9377 100644 --- a/runtime/vm/service.h +++ b/runtime/vm/service.h @@ -57,7 +57,7 @@ class ServiceIdZone { virtual char* GetServiceId(const Object& obj) = 0; // Invalidate all the Service IDs currently living in this zone. virtual void Invalidate() = 0; - virtual void VisitPointers(ObjectPointerVisitor& visitor) const = 0; + virtual void VisitPointers(ObjectPointerVisitor* visitor) const = 0; virtual void PrintJSON(JSONStream& js) const = 0; @@ -91,7 +91,7 @@ class RingServiceIdZone final : public ServiceIdZone { // Returned string will be zone allocated. char* GetServiceId(const Object& obj) final; void Invalidate() final; - void VisitPointers(ObjectPointerVisitor& visitor) const final; + void VisitPointers(ObjectPointerVisitor* visitor) const final; void PrintJSON(JSONStream& js) const final; diff --git a/runtime/vm/visitor.h b/runtime/vm/visitor.h index e306164bb05..9f9b2085fd1 100644 --- a/runtime/vm/visitor.h +++ b/runtime/vm/visitor.h @@ -67,6 +67,7 @@ class ObjectPointerVisitor { // through fields. // Otherwise trace field values through isolate's field_table. virtual bool trace_values_through_fields() const { return false; } + virtual bool trace_object_id_rings() const { return true; } const ClassTable* class_table() const { return class_table_; }