fix stack mapping code to do as many passes as necessary

Previously, we had been doing exactly two passes over the event log to caculate the stack object reference map at each trace point. It turns out the correct number of passes depends on how many incorrect assumptions we make about what the stack looks like at instructions with multiple predecessors (i.e. targets of jumps and branches). Each time we detect we've made one or more incorrect assumptions during a pass, we must do another pass to correct those assumptions. That pass may in turn reveal further incorrect assumptions, and so on.
ReadyTalk · Mar 5, 2008 · 9fe0083 · 9fe0083
1 parent 7343eea
commit 9fe0083
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 77 deletions.
diff --git a/src/compile.cpp b/src/compile.cpp
@@ -496,6 +496,7 @@ class Context {
   TraceElement* traceLog;
   uint16_t* visitTable;
   uintptr_t* rootTable;
+  bool dirtyRoots;
   Vector eventLog;
   MyProtector protector;
 };
@@ -3537,7 +3538,8 @@ calculateFrameMaps(MyThread* t, Context* context, uintptr_t* originalRoots,
   // that position, or the contents of that position are as yet
   // unknown.
 
-  while (eventIndex < context->eventLog.length()) {
+  unsigned length = context->eventLog.length();
+  while (eventIndex < length) {
     Event e = static_cast<Event>(context->eventLog.get(eventIndex++));
     switch (e) {
     case PushEvent: {
@@ -3549,6 +3551,7 @@ calculateFrameMaps(MyThread* t, Context* context, uintptr_t* originalRoots,
 
     case IpEvent: {
       ip = context->eventLog.get2(eventIndex);
+      eventIndex += 2;
 
       if (DebugFrameMaps) {
         fprintf(stderr, "      roots at ip %3d: ", ip);
@@ -3560,7 +3563,20 @@ calculateFrameMaps(MyThread* t, Context* context, uintptr_t* originalRoots,
 
       if (context->visitTable[ip] > 1) {
         for (unsigned wi = 0; wi < mapSize; ++wi) {
-          tableRoots[wi] &= roots[wi];
+          uintptr_t newRoots = tableRoots[wi] & roots[wi];
+
+          if ((eventIndex == length
+               or context->eventLog.get(eventIndex) == PopEvent)
+              and newRoots != tableRoots[wi])
+          {
+            if (DebugFrameMaps) {
+              fprintf(stderr, "dirty roots!\n");
+            }
+
+            context->dirtyRoots = true;
+          }
+
+          tableRoots[wi] = newRoots;
           roots[wi] &= tableRoots[wi];
         }
 
@@ -3572,92 +3588,20 @@ calculateFrameMaps(MyThread* t, Context* context, uintptr_t* originalRoots,
       } else {
         memcpy(tableRoots, roots, mapSize * BytesPerWord);
       }
-
-      eventIndex += 2;
     } break;
 
     case MarkEvent: {
       unsigned i = context->eventLog.get2(eventIndex);
-      markBit(roots, i);
-
-      eventIndex += 2;
-    } break;
-
-    case ClearEvent: {
-      unsigned i = context->eventLog.get2(eventIndex);
-      clearBit(roots, i);
-
-      eventIndex += 2;
-    } break;
-
-    case TraceEvent: {
-      eventIndex += BytesPerWord;
-    } break;
-
-    default: abort(t);
-    }
-  }
-
-  return eventIndex;
-}
-
-unsigned
-updateTraceElements(MyThread* t, Context* context, uintptr_t* originalRoots,
-                    unsigned eventIndex)
-{
-  unsigned mapSize = frameMapSizeInWords(t, context->method);
-
-  uintptr_t roots[mapSize];
-  if (originalRoots) {
-    memcpy(roots, originalRoots, mapSize * BytesPerWord);
-  } else {
-    memset(roots, 0, mapSize * BytesPerWord);
-  }
-
-  int32_t ip = -1;
-
-  while (eventIndex < context->eventLog.length()) {
-    Event e = static_cast<Event>(context->eventLog.get(eventIndex++));
-    switch (e) {
-    case PushEvent: {
-      eventIndex = updateTraceElements(t, context, roots, eventIndex);
-    } break;
-
-    case PopEvent:
-      return eventIndex;
-
-    case IpEvent: {
-      ip = context->eventLog.get2(eventIndex);
-
-      if (DebugFrameMaps) {
-        fprintf(stderr, "        map at ip %3d: ", ip);
-        printSet(*roots);
-        fprintf(stderr, "\n");
-      }
-
-      if (context->visitTable[ip] > 1) {
-        uintptr_t* tableRoots = context->rootTable + (ip * mapSize);
-
-        for (unsigned wi = 0; wi < mapSize; ++wi) {
-          roots[wi] &= tableRoots[wi];
-        }
-      }
-
       eventIndex += 2;
-    } break;
 
-    case MarkEvent: {
-      unsigned i = context->eventLog.get2(eventIndex);
       markBit(roots, i);
-
-      eventIndex += 2;
     } break;
 
     case ClearEvent: {
       unsigned i = context->eventLog.get2(eventIndex);
-      clearBit(roots, i);
-
       eventIndex += 2;
+
+      clearBit(roots, i);
     } break;
 
     case TraceEvent: {
@@ -3811,6 +3755,7 @@ compile(MyThread* t, Context* context)
   compile(t, &frame, 0);
   if (UNLIKELY(t->exception)) return 0;
 
+  context->dirtyRoots = false;
   unsigned eventIndex = calculateFrameMaps(t, context, 0, 0);
 
   object eht = codeExceptionHandlerTable(t, methodCode(t, context->method));
@@ -3867,7 +3812,10 @@ compile(MyThread* t, Context* context)
     }
   }
 
-  updateTraceElements(t, context, 0, 0);
+  while (context->dirtyRoots) {
+    context->dirtyRoots = false;
+    calculateFrameMaps(t, context, 0, 0);
+  }
 
   return finish(t, context, 0);
 }

diff --git a/test/GC.java b/test/GC.java
@@ -87,6 +87,23 @@ private static void stackMap5(boolean predicate) {
     System.gc();
   }
 
+  private static void stackMap6(boolean predicate) {
+    if (predicate) {
+      int a = 42;
+    } else {
+      Object a = null;
+    }
+
+    if (predicate) {
+      noop();
+    } else {
+      Object a = null;
+    }
+
+    noop();
+    System.gc();
+  }
+
   public static void main(String[] args) {
     Object[] array = new Object[1024 * 1024];
     array[0] = new Object();
@@ -119,6 +136,9 @@ public static void main(String[] args) {
 
     stackMap5(true);
     stackMap5(false);
+
+    stackMap6(true);
+    stackMap6(false);
   }
 
 }