[mycpp/runtime] Make marking non-recursive.

Use a "gray stack". This worked on the first try! I just typed it and it worked. (It passes all tests I ran, but let's see what the CI says.)
oils-for-unix · Jan 3, 2023 · 4a08648 · 4a08648
1 parent ea63c59
commit 4a08648
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 56 deletions.
diff --git a/mycpp/gc_obj.h b/mycpp/gc_obj.h
@@ -55,10 +55,14 @@ struct ObjHeader {
 #endif
 };
 
+// TODO: we could determine the max statically!
+const int kFieldMaskBits = 16;
+
 #if defined(MARK_SWEEP) || defined(BUMP_LEAK)
   #define FIELD_MASK(header) (header).u_mask_npointers_strlen
   #define STR_LEN(header) (header).u_mask_npointers_strlen
   #define NUM_POINTERS(header) (header).u_mask_npointers_strlen
+
 #else
   #define FIELD_MASK(header) (header).field_mask
                              // TODO: derive from obj_len

diff --git a/mycpp/mark_sweep_heap.cc b/mycpp/mark_sweep_heap.cc
@@ -81,66 +81,84 @@ void* MarkSweepHeap::Reallocate(void* p, size_t num_bytes) {
   // return realloc(p, num_bytes);
 }
 
-void MarkSweepHeap::MarkObjects(RawObject* obj) {
+// "Leaf" for marking / TraceChildren
+//
+// - Abort if nullptr
+// - Find the header (get rid of this when remove ObjHeader member)
+// - Tag::{Opaque,FixedSized,Scanned} have their mark bits set
+// - Tag::{FixedSize,Scanned} are also pushed on the gray stack
+
+void MarkSweepHeap::MaybeMarkAndPush(RawObject* obj) {
+  if (obj == nullptr) {
+    return;
+  }
+
   ObjHeader* header = FindObjHeader(obj);
   int obj_id = header->obj_id;
-  bool is_marked = mark_set_.IsMarked(obj_id);
 
-  if (is_marked) {
+  // Don't re-mark or re-push
+  if (mark_set_.IsMarked(obj_id)) {
     return;
   }
 
   switch (header->heap_tag) {
-  case HeapTag::Opaque:
+  case HeapTag::Opaque:  // e.g. strings have no children
     mark_set_.Mark(obj_id);
     break;
 
-  case HeapTag::FixedSize: {
+  case HeapTag::Scanned:  // these 2 types have children
+  case HeapTag::FixedSize:
     mark_set_.Mark(obj_id);
+    gray_stack_.push_back(header);  // Push the header, not the object!
+    break;
+
+  case HeapTag::Global:
+    // don't mark or push
+    break;
+
+  default:
+    FAIL(kShouldNotGetHere);
+  }
+}
+
+void MarkSweepHeap::TraceChildren() {
+  while (!gray_stack_.empty()) {
+    ObjHeader* header = gray_stack_.back();
+    gray_stack_.pop_back();
 
-    auto fixed = reinterpret_cast<LayoutFixed*>(header);
-    int mask = FIELD_MASK(fixed->header_);
+    switch (header->heap_tag) {
+    case HeapTag::FixedSize: {
+      auto fixed = reinterpret_cast<LayoutFixed*>(header);
+      int mask = FIELD_MASK(fixed->header_);
 
-    // TODO(Jesse): Put the 16 in a #define
-    for (int i = 0; i < 16; ++i) {
-      if (mask & (1 << i)) {
-        RawObject* child = fixed->children_[i];
-        if (child) {
-          MarkObjects(child);
+      for (int i = 0; i < kFieldMaskBits; ++i) {
+        if (mask & (1 << i)) {
+          MaybeMarkAndPush(fixed->children_[i]);
         }
       }
+      break;
     }
-    break;
-  }
-
-  case HeapTag::Scanned: {
-    mark_set_.Mark(obj_id);
 
-    // no vtable
-    assert(reinterpret_cast<void*>(header) == reinterpret_cast<void*>(obj));
+    case HeapTag::Scanned: {
+      // no vtable
+      // assert(reinterpret_cast<void*>(header) ==
+      // reinterpret_cast<void*>(obj));
 
-    auto slab = reinterpret_cast<Slab<RawObject*>*>(header);
+      auto slab = reinterpret_cast<Slab<RawObject*>*>(header);
 
-    // TODO: mark and sweep should store number of pointers directly
-    // int n = (slab->header_.obj_len - kSlabHeaderSize) / sizeof(void*);
-    int n = NUM_POINTERS(slab->header_);
+      // TODO: mark and sweep should store number of pointers directly
+      // int n = (slab->header_.obj_len - kSlabHeaderSize) / sizeof(void*);
+      int n = NUM_POINTERS(slab->header_);
 
-    for (int i = 0; i < n; ++i) {
-      RawObject* child = slab->items_[i];
-      if (child) {
-        MarkObjects(child);
+      for (int i = 0; i < n; ++i) {
+        MaybeMarkAndPush(slab->items_[i]);
       }
+      break;
+    }
+    default:
+      // Only FixedSize and Scanned are pushed
+      FAIL(kShouldNotGetHere);
     }
-    break;
-  }
-
-  case HeapTag::Global:
-    // Not marked
-    break;
-
-  default:
-    // Invalid tag
-    assert(0);
   }
 }
 
@@ -186,32 +204,20 @@ int MarkSweepHeap::Collect() {
   // Resize it
   mark_set_.ReInit(current_obj_id_);
 
-  // Note: Can we get rid of double pointers?
-
+  // Note: It might be nice to get rid of double pointers
   for (int i = 0; i < num_roots; ++i) {
     RawObject* root = *(roots_[i]);
-    if (root) {
-      MarkObjects(root);
-    }
+    MaybeMarkAndPush(root);
   }
 
   for (int i = 0; i < num_globals; ++i) {
-    RawObject* root = global_roots_[i];
-    if (root) {
-      MarkObjects(root);
-    }
+    MaybeMarkAndPush(global_roots_[i]);
   }
 
-  #if 0
-  log("Collect(): num marked %d", marked_.size());
-
-  for (auto marked_obj : marked_ ) {
-    auto m = reinterpret_cast<RawObject*>(marked_obj);
-    assert(m->heap_tag != HeapTag::Global);  // BUG FIX
-  }
-  #endif
+  TraceChildren();
 
   Sweep();
+
   if (gc_verbose_) {
     log("    %d live after sweep", num_live_);
   }

diff --git a/mycpp/mark_sweep_heap.h b/mycpp/mark_sweep_heap.h
@@ -104,7 +104,10 @@ class MarkSweepHeap {
   void* Reallocate(void* p, size_t num_bytes);
   int MaybeCollect();
   int Collect();
-  void MarkObjects(RawObject* obj);
+
+  void MaybeMarkAndPush(RawObject* obj);
+  void TraceChildren();
+
   void Sweep();
 
   void PrintStats(int fd);  // public for testing
@@ -142,6 +145,8 @@ class MarkSweepHeap {
   std::vector<RawObject*> global_roots_;
 
   std::vector<void*> live_objs_;
+
+  std::vector<ObjHeader*> gray_stack_;
   MarkSet mark_set_;
 
   int current_obj_id_ = 0;