More special handling of the symbol table during garbage collection....

src/mark-compact.cc

Index: src/mark-compact.cc
===================================================================
--- src/mark-compact.cc	(revision 1845)
+++ src/mark-compact.cc	(working copy)
@@ -562,19 +562,48 @@
 }
 
 
+// Helper class to unmark marked objects in a range of pointers but
+// not recursively.
+class UnmarkingVisitor : public ObjectVisitor {
+ public:
+  void VisitPointers(Object** start, Object** end) {
+    for (Object** p = start; p < end; p++) {
+      if ((*p)->IsHeapObject() && HeapObject::cast(*p)->IsMarked()) {
+        MarkCompactCollector::ClearMark(HeapObject::cast(*p));
+      }
+    }
+  }
+};
+
+
 void MarkCompactCollector::ProcessRoots(RootMarkingVisitor* visitor) {
-  // Mark the heap roots gray, including global variables, stack variables,
-  // etc.
-  Heap::IterateStrongRoots(visitor);
-
-  // Take care of the symbol table specially.
+  // Handle the symbol table specially.  Mark the prefix and the
+  // symbol table itself.  Do not mark the symbol table entries, but
+  // do explicitly mark all other objects reachable from them.
+  //
+  // Objects reachable from symbols are marked as live so as to ensure
+  // that if the symbol itself remains alive after GC for any reason,
+  // and if it is a sliced string or a cons string backed by an
+  // external string (even indirectly), then the external string does
+  // not receive a weak reference callback.

[iposva, 2009/05/04 17:54:47]

In retrospect was that not always a problem even with regular non-external
strings only being referred to by a sliced string in the symbol table?

[Kevin Millikin (Chromium), 2009/05/04 18:34:52]

I don't think so.  The problem was that the weak reference callback was getting
called for the external string.  With no callback, there should not be any (err,
this) problem.

   SymbolTable* symbol_table = SymbolTable::cast(Heap::symbol_table());
-  // 1. Mark the prefix of the symbol table gray.
-  symbol_table->IteratePrefix(visitor);
-  // 2. Mark the symbol table black (ie, do not push it on the marking stack
-  // or mark it overflowed).
+  // First mark everything reachable from the symbol table, then
+  // unmark just the elements themselves.
+  symbol_table->Iterate(visitor);
+  // There may be overflowed objects in the heap.  Visit them now.
+  while (marking_stack.overflowed()) {
+    RefillMarkingStack();
+    EmptyMarkingStack(visitor->stack_visitor());
+  }
+  UnmarkingVisitor unmarking_visitor;
+  symbol_table->IterateElements(&unmarking_visitor);
+  // Mark the symbol table itself.
   SetMark(symbol_table);

[iposva, 2009/05/04 17:54:47]

The flipping of mark bits to marked and back to unmarked seems fragile to me.
Currently this only works because of the order (symbol table walking being
first.)

Since we need to handle the symbol table in a special way. The direct references
are not marked but any depending ones. Can we add a special marker instead? That
way we would not be depending on resetting mark bits during a GC, which is an
unneeded break of the assertions of the GC algorithm and adds unknown ordering
dependencies in the marking steps making the code more complex.

[Kevin Millikin (Chromium), 2009/05/04 18:34:52]

This was just the simplest implementation to get a fix out asap.  It leverages
all the existing infrastructure for recursive marking and handling (the
admittedly as close to impossible as possible) marking stack overflow. I agree
that it's dodgy.

 
+  // Mark the heap roots including global variables, stack variables,
+  // etc., and all objects reachable from them.
+  Heap::IterateStrongRoots(visitor);
+
   // There may be overflowed objects in the heap.  Visit them now.
   while (marking_stack.overflowed()) {
     RefillMarkingStack();
@@ -762,21 +791,22 @@
 
 
 #ifdef DEBUG
-void MarkCompactCollector::UpdateLiveObjectCount(HeapObject* obj) {
-  live_bytes_ += obj->Size();
+void MarkCompactCollector::UpdateLiveObjectCount(HeapObject* obj, int scale) {
+  ASSERT(scale == -1 || scale == 1);
+  live_bytes_ += obj->Size() * scale;
   if (Heap::new_space()->Contains(obj)) {
-    live_young_objects_++;
+    live_young_objects_ += scale;
   } else if (Heap::map_space()->Contains(obj)) {
     ASSERT(obj->IsMap());
-    live_map_objects_++;
+    live_map_objects_ += scale;
   } else if (Heap::old_pointer_space()->Contains(obj)) {
-    live_old_pointer_objects_++;
+    live_old_pointer_objects_ += scale;
   } else if (Heap::old_data_space()->Contains(obj)) {
-    live_old_data_objects_++;
+    live_old_data_objects_ += scale;
   } else if (Heap::code_space()->Contains(obj)) {
-    live_code_objects_++;
+    live_code_objects_ += scale;
   } else if (Heap::lo_space()->Contains(obj)) {
-    live_lo_objects_++;
+    live_lo_objects_ +=scale;
   } else {
     UNREACHABLE();
   }