From ffbbdb5a10a73d598db54823bcc1ce3feda59d07 Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Tue, 21 May 2024 21:29:49 +0000 Subject: [PATCH] [vm, service] Treat the object id ring as strong during major GC. The id ring was treated as a strong root during minor GC and a weak root during major GC. Before mark-through-new-space, new-space objects could not be collected during a major GC, so in practice the policy was that the id ring was strong for all new-space objects. After mark-through-new-space, new-space object can be collected during a major GC, so service ids could expire very quickly, especially for object just created using the service protocol and not otherwise reachable from the Dart program. Given that service id zones have still not been implemented, this makes it impossible to reliably interact with such an object. TEST=ci Bug: https://github.com/flutter/flutter/issues/148704 Change-Id: I0f15c00414f996fad49bcb137c7f1c15bb4955c4 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/367440 Reviewed-by: Derek Xu Reviewed-by: Alexander Aprelev Commit-Queue: Ryan Macnak --- runtime/vm/heap/marker.cc | 46 ++++++------------------------- runtime/vm/heap/marker.h | 1 - runtime/vm/object_id_ring_test.cc | 16 +++++------ 3 files changed, 16 insertions(+), 47 deletions(-) diff --git a/runtime/vm/heap/marker.cc b/runtime/vm/heap/marker.cc index aa841c4008e..623ad6fb797 100644 --- a/runtime/vm/heap/marker.cc +++ b/runtime/vm/heap/marker.cc @@ -699,7 +699,8 @@ void GCMarker::Epilogue() {} enum RootSlices { kIsolate = 0, - kNumFixedRootSlices = 1, + kObjectIdRing = 1, + kNumFixedRootSlices = 2, }; void GCMarker::ResetSlices() { @@ -727,6 +728,12 @@ void GCMarker::IterateRoots(ObjectPointerVisitor* visitor) { visitor, ValidationPolicy::kDontValidateFrames); break; } + case kObjectIdRing: { + TIMELINE_FUNCTION_GC_DURATION(Thread::Current(), + "ProcessObjectIdTable"); + isolate_group_->VisitObjectIdRingPointers(visitor); + break; + } } MonitorLocker ml(&root_slices_monitor_); @@ -740,7 +747,6 @@ void GCMarker::IterateRoots(ObjectPointerVisitor* visitor) { enum WeakSlices { kWeakHandles = 0, kWeakTables, - kObjectIdRing, kRememberedSet, kNumWeakSlices, }; @@ -759,9 +765,6 @@ void GCMarker::IterateWeakRoots(Thread* thread) { case kWeakTables: ProcessWeakTables(thread); break; - case kObjectIdRing: - ProcessObjectIdTable(thread); - break; case kRememberedSet: ProcessRememberedSet(thread); break; @@ -851,39 +854,6 @@ void GCMarker::ProcessRememberedSet(Thread* thread) { store_buffer->PushBlock(writing, StoreBuffer::kIgnoreThreshold); } -class ObjectIdRingClearPointerVisitor : public ObjectPointerVisitor { - public: - explicit ObjectIdRingClearPointerVisitor(IsolateGroup* isolate_group) - : ObjectPointerVisitor(isolate_group) {} - - void VisitPointers(ObjectPtr* first, ObjectPtr* last) override { - for (ObjectPtr* current = first; current <= last; current++) { - ObjectPtr obj = *current; - ASSERT(obj->IsHeapObject()); - if (!obj->untag()->IsMarked()) { - // Object has become garbage. Replace it will null. - *current = Object::null(); - } - } - } - -#if defined(DART_COMPRESSED_POINTERS) - void VisitCompressedPointers(uword heap_base, - CompressedObjectPtr* first, - CompressedObjectPtr* last) override { - UNREACHABLE(); // ObjectIdRing is not compressed. - } -#endif -}; - -void GCMarker::ProcessObjectIdTable(Thread* thread) { -#ifndef PRODUCT - TIMELINE_FUNCTION_GC_DURATION(thread, "ProcessObjectIdTable"); - ObjectIdRingClearPointerVisitor visitor(isolate_group_); - isolate_group_->VisitObjectIdRingPointers(&visitor); -#endif // !PRODUCT -} - class ParallelMarkTask : public ThreadPool::Task { public: ParallelMarkTask(GCMarker* marker, diff --git a/runtime/vm/heap/marker.h b/runtime/vm/heap/marker.h index b9755bb35ae..d799b2b56b7 100644 --- a/runtime/vm/heap/marker.h +++ b/runtime/vm/heap/marker.h @@ -62,7 +62,6 @@ class GCMarker { void ProcessWeakHandles(Thread* thread); void ProcessWeakTables(Thread* thread); void ProcessRememberedSet(Thread* thread); - void ProcessObjectIdTable(Thread* thread); // Called by anyone: finalize and accumulate stats from 'visitor'. template diff --git a/runtime/vm/object_id_ring_test.cc b/runtime/vm/object_id_ring_test.cc index 8ece9933a05..d391e891831 100644 --- a/runtime/vm/object_id_ring_test.cc +++ b/runtime/vm/object_id_ring_test.cc @@ -195,7 +195,7 @@ TEST_CASE(ObjectIdRingScavengeMoveTest) { EXPECT_EQ(3, list_length); } -// Test that the ring table is updated with nulls when the old GC collects. +// Test that the ring table is updated when major GC runs. ISOLATE_UNIT_TEST_CASE(ObjectIdRingOldGCTest) { Isolate* isolate = thread->isolate(); ObjectIdRing* ring = isolate->EnsureObjectIdRing(); @@ -229,16 +229,16 @@ ISOLATE_UNIT_TEST_CASE(ObjectIdRingOldGCTest) { UntaggedObject::ToAddr(raw_obj2)); // Exit scope. Freeing String handle. } - // Force a GC. No reference exist to the old string anymore. It should be - // collected and the object id ring will now return the null object for - // those ids. + // Force a GC. No other reference to the old string exists, but the service id + // should keep it alive. GCTestHelper::CollectOldSpace(); ObjectPtr raw_object_moved1 = ring->GetObjectForId(raw_obj_id1, &kind); - EXPECT_EQ(ObjectIdRing::kCollected, kind); - EXPECT_EQ(Object::null(), raw_object_moved1); + EXPECT_EQ(ObjectIdRing::kValid, kind); + EXPECT(raw_object_moved1->IsOneByteString()); ObjectPtr raw_object_moved2 = ring->GetObjectForId(raw_obj_id2, &kind); - EXPECT_EQ(ObjectIdRing::kCollected, kind); - EXPECT_EQ(Object::null(), raw_object_moved2); + EXPECT_EQ(ObjectIdRing::kValid, kind); + EXPECT(raw_object_moved2->IsOneByteString()); + EXPECT_EQ(raw_object_moved1, raw_object_moved2); } // Test that the ring table correctly reports an entry as expired when it is