[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 <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
index aa841c4..623ad6f 100644
--- a/runtime/vm/heap/marker.cc
+++ b/runtime/vm/heap/marker.cc
@@ -699,7 +699,8 @@
enum RootSlices {
kIsolate = 0,
- kNumFixedRootSlices = 1,
+ kObjectIdRing = 1,
+ kNumFixedRootSlices = 2,
};
void GCMarker::ResetSlices() {
@@ -727,6 +728,12 @@
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 @@
enum WeakSlices {
kWeakHandles = 0,
kWeakTables,
- kObjectIdRing,
kRememberedSet,
kNumWeakSlices,
};
@@ -759,9 +765,6 @@
case kWeakTables:
ProcessWeakTables(thread);
break;
- case kObjectIdRing:
- ProcessObjectIdTable(thread);
- break;
case kRememberedSet:
ProcessRememberedSet(thread);
break;
@@ -851,39 +854,6 @@
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 b9755bb..d799b2b 100644
--- a/runtime/vm/heap/marker.h
+++ b/runtime/vm/heap/marker.h
@@ -62,7 +62,6 @@
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
index 8ece993..d391e89 100644
--- a/runtime/vm/object_id_ring_test.cc
+++ b/runtime/vm/object_id_ring_test.cc
@@ -195,7 +195,7 @@
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 @@
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
