7182543: NMT ON: Aggregate a few NMT related bugs

Summary: 1) Fixed MemTrackWorker::generations_in_used() calculation 2) Ensured NMT not to leak memory recorders after shutdown 3) Used ThreadCritical to block safepoint safe threads
Reviewed-by: acorn, coleenp, dholmes, kvn

5 files changed, 35 insertions, 35 deletions

diff --git a/src/share/vm/services/memRecorder.cpp b/src/share/vm/services/memRecorder.cpp
index 52b2eae62..33db875af 100644
--- a/src/share/vm/services/memRecorder.cpp
+++ b/src/share/vm/services/memRecorder.cpp
@@ -45,11 +45,11 @@ MemPointer* SequencedRecordIterator::next_record() {
 }
 
 
-debug_only(volatile jint MemRecorder::_instance_count = 0;)
+volatile jint MemRecorder::_instance_count = 0;
 
 MemRecorder::MemRecorder() {
   assert(MemTracker::is_on(), "Native memory tracking is off");
-  debug_only(Atomic::inc(&_instance_count);)
+  Atomic::inc(&_instance_count);
   debug_only(set_generation();)
 
   if (MemTracker::track_callsite()) {
@@ -83,9 +83,7 @@ MemRecorder::~MemRecorder() {
     delete _next;
   }
 
-#ifdef ASSERT
   Atomic::dec(&_instance_count);
-#endif
 }
 
 // Sorting order:
diff --git a/src/share/vm/services/memRecorder.hpp b/src/share/vm/services/memRecorder.hpp
index c243564db..313af97db 100644
--- a/src/share/vm/services/memRecorder.hpp
+++ b/src/share/vm/services/memRecorder.hpp
@@ -249,9 +249,9 @@ class MemRecorder : public CHeapObj<mtNMT|otNMTRecorder> {
 
   SequencedRecordIterator pointer_itr();
 
- public:
+ protected:
   // number of MemRecorder instance
-  debug_only(static volatile jint _instance_count;)
+  static volatile jint _instance_count;
 
  private:
   // sorting function, sort records into following order
diff --git a/src/share/vm/services/memTrackWorker.hpp b/src/share/vm/services/memTrackWorker.hpp
index 6c236784e..969828cab 100644
--- a/src/share/vm/services/memTrackWorker.hpp
+++ b/src/share/vm/services/memTrackWorker.hpp
@@ -67,7 +67,7 @@ class MemTrackWorker : public NamedThread {
   NOT_PRODUCT(int _last_gen_in_use;)
 
   inline int generations_in_use() const {
-    return (_tail <= _head ? (_head - _tail + 1) : (MAX_GENERATIONS - (_tail - _head) + 1));
+    return (_tail >= _head ? (_tail - _head + 1) : (MAX_GENERATIONS - (_head - _tail) + 1));
   }
 };
 
diff --git a/src/share/vm/services/memTracker.cpp b/src/share/vm/services/memTracker.cpp
index 55c985965..6c6c37dec 100644
--- a/src/share/vm/services/memTracker.cpp
+++ b/src/share/vm/services/memTracker.cpp
@@ -351,21 +351,17 @@ void MemTracker::create_memory_record(address addr, MEMFLAGS flags,
     }
 
     if (thread != NULL) {
-#ifdef ASSERT
-      // cause assertion on stack base. This ensures that threads call
-      // Thread::record_stack_base_and_size() method, which will create
-      // thread native stack records.
-      thread->stack_base();
-#endif
-      // for a JavaThread, if it is running in native state, we need to transition it to
-      // VM state, so it can stop at safepoint. JavaThread running in VM state does not
-      // need lock to write records.
       if (thread->is_Java_thread() && ((JavaThread*)thread)->is_safepoint_visible()) {
-        if (((JavaThread*)thread)->thread_state() == _thread_in_native) {
-          ThreadInVMfromNative trans((JavaThread*)thread);
-          create_record_in_recorder(addr, flags, size, pc, thread);
+        JavaThread*      java_thread = static_cast<JavaThread*>(thread);
+        JavaThreadState  state = java_thread->thread_state();
+        if (SafepointSynchronize::safepoint_safe(java_thread, state)) {
+          // JavaThreads that are safepoint safe, can run through safepoint,
+          // so ThreadCritical is needed to ensure no threads at safepoint create
+          // new records while the records are being gathered and the sequence number is changing
+          ThreadCritical tc;
+          create_record_in_recorder(addr, flags, size, pc, java_thread);
         } else {
-          create_record_in_recorder(addr, flags, size, pc, thread);
+          create_record_in_recorder(addr, flags, size, pc, java_thread);
         }
       } else {
         // other threads, such as worker and watcher threads, etc. need to
@@ -390,10 +386,9 @@ void MemTracker::create_memory_record(address addr, MEMFLAGS flags,
 // write a record to proper recorder. No lock can be taken from this method
 // down.
 void MemTracker::create_record_in_recorder(address addr, MEMFLAGS flags,
-    size_t size, address pc, Thread* thread) {
-    assert(thread == NULL || thread->is_Java_thread(), "wrong thread");
+    size_t size, address pc, JavaThread* thread) {
 
-    MemRecorder* rc = get_thread_recorder((JavaThread*)thread);
+    MemRecorder* rc = get_thread_recorder(thread);
     if (rc != NULL) {
       rc->record(addr, flags, size, pc);
     }
@@ -460,17 +455,18 @@ void MemTracker::sync() {
       }
     }
     _sync_point_skip_count = 0;
-    // walk all JavaThreads to collect recorders
-    SyncThreadRecorderClosure stc;
-    Threads::threads_do(&stc);
-
-    _thread_count = stc.get_thread_count();
-    MemRecorder* pending_recorders = get_pending_recorders();
-
     {
       // This method is running at safepoint, with ThreadCritical lock,
       // it should guarantee that NMT is fully sync-ed.
       ThreadCritical tc;
+
+      // walk all JavaThreads to collect recorders
+      SyncThreadRecorderClosure stc;
+      Threads::threads_do(&stc);
+
+      _thread_count = stc.get_thread_count();
+      MemRecorder* pending_recorders = get_pending_recorders();
+
       if (_global_recorder != NULL) {
         _global_recorder->set_next(pending_recorders);
         pending_recorders = _global_recorder;
@@ -486,8 +482,6 @@ void MemTracker::sync() {
 
   // now, it is the time to shut whole things off
   if (_state == NMT_final_shutdown) {
-    _tracking_level = NMT_off;
-
     // walk all JavaThreads to delete all recorders
     SyncThreadRecorderClosure stc;
     Threads::threads_do(&stc);
@@ -499,8 +493,16 @@ void MemTracker::sync() {
         _global_recorder = NULL;
       }
     }
-
-    _state = NMT_shutdown;
+    MemRecorder* pending_recorders = get_pending_recorders();
+    if (pending_recorders != NULL) {
+      delete pending_recorders;
+    }
+    // try at a later sync point to ensure MemRecorder instance drops to zero to
+    // completely shutdown NMT
+    if (MemRecorder::_instance_count == 0) {
+      _state = NMT_shutdown;
+      _tracking_level = NMT_off;
+    }
   }
 }
 
diff --git a/src/share/vm/services/memTracker.hpp b/src/share/vm/services/memTracker.hpp
index b5607c1a0..10f2269d3 100644
--- a/src/share/vm/services/memTracker.hpp
+++ b/src/share/vm/services/memTracker.hpp
@@ -326,7 +326,7 @@ class MemTracker : AllStatic {
   static void create_memory_record(address addr, MEMFLAGS type,
                    size_t size, address pc, Thread* thread);
   static void create_record_in_recorder(address addr, MEMFLAGS type,
-                   size_t size, address pc, Thread* thread);
+                   size_t size, address pc, JavaThread* thread);
 
  private:
   // global memory snapshot