[vm, service] Treat the object id ring as strong during major GC.

rmacnak-google · Commit Queue · commit ffbbdb5a10a7 · 2024-05-21T21:29:49.000Z
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: flutter/flutter#148704 Change-Id: I0f15c00414f996fad49bcb137c7f1c15bb4955c4 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/367440 Reviewed-by: Derek Xu <derekx@google.com> Reviewed-by: Alexander Aprelev <aam@google.com> Commit-Queue: Ryan Macnak <rmacnak@google.com>
diff --git 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
@@ -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 <class MarkingVisitorType>
diff --git 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
