6964164: MonitorInUseLists leak of contended objects

Summary: fix MonitorInUseLists memory leak and MonitorBound now works
Reviewed-by: chrisphi, dice

src/share/vm/runtime/synchronizer.cpp

@@ -747,6 +747,8 @@ void Thread::muxRelease (volatile intptr_t * Lock)  {
 
 ObjectMonitor * ObjectSynchronizer::gBlockList = NULL ;
 ObjectMonitor * volatile ObjectSynchronizer::gFreeList  = NULL ;
+ObjectMonitor * volatile ObjectSynchronizer::gOmInUseList  = NULL ;
+int ObjectSynchronizer::gOmInUseCount = 0;
 static volatile intptr_t ListLock = 0 ;      // protects global monitor free-list cache
 static volatile int MonitorFreeCount  = 0 ;      // # on gFreeList
 static volatile int MonitorPopulation = 0 ;      // # Extant -- in circulation
@@ -826,6 +828,22 @@ static void InduceScavenge (Thread * Self, const char * Whence) {
     }
   }
 }
+/* Too slow for general assert or debug
+void ObjectSynchronizer::verifyInUse (Thread *Self) {
+   ObjectMonitor* mid;
+   int inusetally = 0;
+   for (mid = Self->omInUseList; mid != NULL; mid = mid->FreeNext) {
+     inusetally ++;
+   }
+   assert(inusetally == Self->omInUseCount, "inuse count off");
+
+   int freetally = 0;
+   for (mid = Self->omFreeList; mid != NULL; mid = mid->FreeNext) {
+     freetally ++;
+   }
+   assert(freetally == Self->omFreeCount, "free count off");
+}
+*/
 
 ObjectMonitor * ATTR ObjectSynchronizer::omAlloc (Thread * Self) {
     // A large MAXPRIVATE value reduces both list lock contention
@@ -853,6 +871,9 @@ ObjectMonitor * ATTR ObjectSynchronizer::omAlloc (Thread * Self) {
              m->FreeNext = Self->omInUseList;
              Self->omInUseList = m;
              Self->omInUseCount ++;
+             // verifyInUse(Self);
+           } else {
+             m->FreeNext = NULL;
            }
            return m ;
         }
@@ -874,13 +895,12 @@ ObjectMonitor * ATTR ObjectSynchronizer::omAlloc (Thread * Self) {
                 guarantee (take->object() == NULL, "invariant") ;
                 guarantee (!take->is_busy(), "invariant") ;
                 take->Recycle() ;
-                omRelease (Self, take) ;
+                omRelease (Self, take, false) ;
             }
             Thread::muxRelease (&ListLock) ;
             Self->omFreeProvision += 1 + (Self->omFreeProvision/2) ;
             if (Self->omFreeProvision > MAXPRIVATE ) Self->omFreeProvision = MAXPRIVATE ;
             TEVENT (omFirst - reprovision) ;
-            continue ;
 
             const int mx = MonitorBound ;
             if (mx > 0 && (MonitorPopulation-MonitorFreeCount) > mx) {
@@ -961,11 +981,34 @@ ObjectMonitor * ATTR ObjectSynchronizer::omAlloc (Thread * Self) {
 // That is, *not* one-at-a-time.
 
 
-void ObjectSynchronizer::omRelease (Thread * Self, ObjectMonitor * m) {
+void ObjectSynchronizer::omRelease (Thread * Self, ObjectMonitor * m, bool fromPerThreadAlloc) {
     guarantee (m->object() == NULL, "invariant") ;
-    m->FreeNext = Self->omFreeList ;
-    Self->omFreeList = m ;
-    Self->omFreeCount ++ ;
+
+    // Remove from omInUseList
+    if (MonitorInUseLists && fromPerThreadAlloc) {
+      ObjectMonitor* curmidinuse = NULL;
+      for (ObjectMonitor* mid = Self->omInUseList; mid != NULL; ) {
+       if (m == mid) {
+         // extract from per-thread in-use-list
+         if (mid == Self->omInUseList) {
+           Self->omInUseList = mid->FreeNext;
+         } else if (curmidinuse != NULL) {
+           curmidinuse->FreeNext = mid->FreeNext; // maintain the current thread inuselist
+         }
+         Self->omInUseCount --;
+         // verifyInUse(Self);
+         break;
+       } else {
+         curmidinuse = mid;
+         mid = mid->FreeNext;
+      }
+    }
+  }
+
+  // FreeNext is used for both onInUseList and omFreeList, so clear old before setting new
+  m->FreeNext = Self->omFreeList ;
+  Self->omFreeList = m ;
+  Self->omFreeCount ++ ;
 }
 
 // Return the monitors of a moribund thread's local free list to
@@ -975,6 +1018,10 @@ void ObjectSynchronizer::omRelease (Thread * Self, ObjectMonitor * m) {
 // consecutive STW safepoints.  Relatedly, we might decay
 // omFreeProvision at STW safepoints.
 //
+// Also return the monitors of a moribund thread"s omInUseList to
+// a global gOmInUseList under the global list lock so these
+// will continue to be scanned.
+//
 // We currently call omFlush() from the Thread:: dtor _after the thread
 // has been excised from the thread list and is no longer a mutator.
 // That means that omFlush() can run concurrently with a safepoint and
@@ -987,24 +1034,50 @@ void ObjectSynchronizer::omRelease (Thread * Self, ObjectMonitor * m) {
 void ObjectSynchronizer::omFlush (Thread * Self) {
     ObjectMonitor * List = Self->omFreeList ;  // Null-terminated SLL
     Self->omFreeList = NULL ;
-    if (List == NULL) return ;
     ObjectMonitor * Tail = NULL ;
-    ObjectMonitor * s ;
     int Tally = 0;
-    for (s = List ; s != NULL ; s = s->FreeNext) {
-        Tally ++ ;
-        Tail = s ;
-        guarantee (s->object() == NULL, "invariant") ;
-        guarantee (!s->is_busy(), "invariant") ;
-        s->set_owner (NULL) ;   // redundant but good hygiene
-        TEVENT (omFlush - Move one) ;
+    if (List != NULL) {
+      ObjectMonitor * s ;
+      for (s = List ; s != NULL ; s = s->FreeNext) {
+          Tally ++ ;
+          Tail = s ;
+          guarantee (s->object() == NULL, "invariant") ;
+          guarantee (!s->is_busy(), "invariant") ;
+          s->set_owner (NULL) ;   // redundant but good hygiene
+          TEVENT (omFlush - Move one) ;
+      }
+      guarantee (Tail != NULL && List != NULL, "invariant") ;
+    }
+
+    ObjectMonitor * InUseList = Self->omInUseList;
+    ObjectMonitor * InUseTail = NULL ;
+    int InUseTally = 0;
+    if (InUseList != NULL) {
+      Self->omInUseList = NULL;
+      ObjectMonitor *curom;
+      for (curom = InUseList; curom != NULL; curom = curom->FreeNext) {
+        InUseTail = curom;
+        InUseTally++;
+      }
+// TODO debug
+      assert(Self->omInUseCount == InUseTally, "inuse count off");
+      Self->omInUseCount = 0;
+      guarantee (InUseTail != NULL && InUseList != NULL, "invariant");
     }
 
-    guarantee (Tail != NULL && List != NULL, "invariant") ;
     Thread::muxAcquire (&ListLock, "omFlush") ;
-    Tail->FreeNext = gFreeList ;
-    gFreeList = List ;
-    MonitorFreeCount += Tally;
+    if (Tail != NULL) {
+      Tail->FreeNext = gFreeList ;
+      gFreeList = List ;
+      MonitorFreeCount += Tally;
+    }
+
+    if (InUseTail != NULL) {
+      InUseTail->FreeNext = gOmInUseList;
+      gOmInUseList = InUseList;
+      gOmInUseCount += InUseTally;
+    }
+
     Thread::muxRelease (&ListLock) ;
     TEVENT (omFlush) ;
 }
@@ -1166,7 +1239,6 @@ ObjectMonitor * ATTR ObjectSynchronizer::inflate (Thread * Self, oop object) {
           // We do this before the CAS in order to minimize the length of time
           // in which INFLATING appears in the mark.
           m->Recycle();
-          m->FreeNext      = NULL ;
           m->_Responsible  = NULL ;
           m->OwnerIsThread = 0 ;
           m->_recursions   = 0 ;
@@ -1174,7 +1246,7 @@ ObjectMonitor * ATTR ObjectSynchronizer::inflate (Thread * Self, oop object) {
 
           markOop cmp = (markOop) Atomic::cmpxchg_ptr (markOopDesc::INFLATING(), object->mark_addr(), mark) ;
           if (cmp != mark) {
-             omRelease (Self, m) ;
+             omRelease (Self, m, true) ;
              continue ;       // Interference -- just retry
           }
 
@@ -1262,7 +1334,6 @@ ObjectMonitor * ATTR ObjectSynchronizer::inflate (Thread * Self, oop object) {
       m->set_object(object);
       m->OwnerIsThread = 1 ;
       m->_recursions   = 0 ;
-      m->FreeNext      = NULL ;
       m->_Responsible  = NULL ;
       m->_SpinDuration = Knob_SpinLimit ;       // consider: keep metastats by type/class
 
@@ -1271,7 +1342,7 @@ ObjectMonitor * ATTR ObjectSynchronizer::inflate (Thread * Self, oop object) {
           m->set_owner  (NULL) ;
           m->OwnerIsThread = 0 ;
           m->Recycle() ;
-          omRelease (Self, m) ;
+          omRelease (Self, m, true) ;
           m = NULL ;
           continue ;
           // interference - the markword changed - just retry.
@@ -1852,6 +1923,10 @@ void ObjectSynchronizer::oops_do(OopClosure* f) {
 // only scans the per-thread inuse lists. omAlloc() puts all
 // assigned monitors on the per-thread list. deflate_idle_monitors()
 // returns the non-busy monitors to the global free list.
+// When a thread dies, omFlush() adds the list of active monitors for
+// that thread to a global gOmInUseList acquiring the
+// global list lock. deflate_idle_monitors() acquires the global
+// list lock to scan for non-busy monitors to the global free list.
 // An alternative could have used a single global inuse list. The
 // downside would have been the additional cost of acquiring the global list lock
 // for every omAlloc().
@@ -1904,6 +1979,7 @@ bool ObjectSynchronizer::deflate_monitor(ObjectMonitor* mid, oop obj,
      if (*FreeHeadp == NULL) *FreeHeadp = mid;
      if (*FreeTailp != NULL) {
        ObjectMonitor * prevtail = *FreeTailp;
+       assert(prevtail->FreeNext == NULL, "cleaned up deflated?"); // TODO KK
        prevtail->FreeNext = mid;
       }
      *FreeTailp = mid;
@@ -1912,6 +1988,39 @@ bool ObjectSynchronizer::deflate_monitor(ObjectMonitor* mid, oop obj,
   return deflated;
 }
 
+// Caller acquires ListLock
+int ObjectSynchronizer::walk_monitor_list(ObjectMonitor** listheadp,
+                                          ObjectMonitor** FreeHeadp, ObjectMonitor** FreeTailp) {
+  ObjectMonitor* mid;
+  ObjectMonitor* next;
+  ObjectMonitor* curmidinuse = NULL;
+  int deflatedcount = 0;
+
+  for (mid = *listheadp; mid != NULL; ) {
+     oop obj = (oop) mid->object();
+     bool deflated = false;
+     if (obj != NULL) {
+       deflated = deflate_monitor(mid, obj, FreeHeadp, FreeTailp);
+     }
+     if (deflated) {
+       // extract from per-thread in-use-list
+       if (mid == *listheadp) {
+         *listheadp = mid->FreeNext;
+       } else if (curmidinuse != NULL) {
+         curmidinuse->FreeNext = mid->FreeNext; // maintain the current thread inuselist
+       }
+       next = mid->FreeNext;
+       mid->FreeNext = NULL;  // This mid is current tail in the FreeHead list
+       mid = next;
+       deflatedcount++;
+     } else {
+       curmidinuse = mid;
+       mid = mid->FreeNext;
+    }
+  }
+  return deflatedcount;
+}
+
 void ObjectSynchronizer::deflate_idle_monitors() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
   int nInuse = 0 ;              // currently associated with objects
@@ -1929,36 +2038,25 @@ void ObjectSynchronizer::deflate_idle_monitors() {
   Thread::muxAcquire (&ListLock, "scavenge - return") ;
 
   if (MonitorInUseLists) {
-    ObjectMonitor* mid;
-    ObjectMonitor* next;
-    ObjectMonitor* curmidinuse;
+    int inUse = 0;
     for (JavaThread* cur = Threads::first(); cur != NULL; cur = cur->next()) {
-      curmidinuse = NULL;
-      for (mid = cur->omInUseList; mid != NULL; ) {
-        oop obj = (oop) mid->object();
-        deflated = false;
-        if (obj != NULL) {
-          deflated = deflate_monitor(mid, obj, &FreeHead, &FreeTail);
-        }
-        if (deflated) {
-          // extract from per-thread in-use-list
-          if (mid == cur->omInUseList) {
-            cur->omInUseList = mid->FreeNext;
-          } else if (curmidinuse != NULL) {
-            curmidinuse->FreeNext = mid->FreeNext; // maintain the current thread inuselist
-          }
-          next = mid->FreeNext;
-          mid->FreeNext = NULL;  // This mid is current tail in the FreeHead list
-          mid = next;
-          cur->omInUseCount--;
-          nScavenged ++ ;
-        } else {
-          curmidinuse = mid;
-          mid = mid->FreeNext;
-          nInuse ++;
-        }
+      nInCirculation+= cur->omInUseCount;
+      int deflatedcount = walk_monitor_list(cur->omInUseList_addr(), &FreeHead, &FreeTail);
+      cur->omInUseCount-= deflatedcount;
+      // verifyInUse(cur);
+      nScavenged += deflatedcount;
+      nInuse += cur->omInUseCount;
      }
-   }
+
+   // For moribund threads, scan gOmInUseList
+   if (gOmInUseList) {
+     nInCirculation += gOmInUseCount;
+     int deflatedcount = walk_monitor_list((ObjectMonitor **)&gOmInUseList, &FreeHead, &FreeTail);
+     gOmInUseCount-= deflatedcount;
+     nScavenged += deflatedcount;
+     nInuse += gOmInUseCount;
+    }
+
   } else for (ObjectMonitor* block = gBlockList; block != NULL; block = next(block)) {
   // Iterate over all extant monitors - Scavenge all idle monitors.
     assert(block->object() == CHAINMARKER, "must be a block header");

src/share/vm/runtime/synchronizer.hpp

@@ -122,8 +122,9 @@ class ObjectSynchronizer : AllStatic {
   static void reenter            (Handle obj, intptr_t recursion, TRAPS);
 
   // thread-specific and global objectMonitor free list accessors
+//  static void verifyInUse (Thread * Self) ; too slow for general assert/debug
   static ObjectMonitor * omAlloc (Thread * Self) ;
-  static void omRelease (Thread * Self, ObjectMonitor * m) ;
+  static void omRelease (Thread * Self, ObjectMonitor * m, bool FromPerThreadAlloc) ;
   static void omFlush   (Thread * Self) ;
 
   // Inflate light weight monitor to heavy weight monitor
@@ -150,6 +151,9 @@ class ObjectSynchronizer : AllStatic {
   // Basically we deflate all monitors that are not busy.
   // An adaptive profile-based deflation policy could be used if needed
   static void deflate_idle_monitors();
+  static int walk_monitor_list(ObjectMonitor** listheadp,
+                               ObjectMonitor** FreeHeadp,
+                               ObjectMonitor** FreeTailp);
   static bool deflate_monitor(ObjectMonitor* mid, oop obj, ObjectMonitor** FreeHeadp,
                               ObjectMonitor** FreeTailp);
   static void oops_do(OopClosure* f);
@@ -163,6 +167,8 @@ class ObjectSynchronizer : AllStatic {
   enum { _BLOCKSIZE = 128 };
   static ObjectMonitor* gBlockList;
   static ObjectMonitor * volatile gFreeList;
+  static ObjectMonitor * volatile gOmInUseList; // for moribund thread, so monitors they inflated still get scanned
+  static int gOmInUseCount;
 
  public:
   static void Initialize () ;

src/share/vm/runtime/thread.hpp

@@ -270,6 +270,7 @@ class Thread: public ThreadShadow {
   static void interrupt(Thread* thr);
   static bool is_interrupted(Thread* thr, bool clear_interrupted);
 
+  ObjectMonitor** omInUseList_addr()             { return (ObjectMonitor **)&omInUseList; }
   Monitor* SR_lock() const                       { return _SR_lock; }
 
   bool has_async_exception() const { return (_suspend_flags & _has_async_exception) != 0; }