# HG changeset patch
# User Yasumasa Suenaga <suenaga.yasumasa@lab.ntt.co.jp>
# Date 1385014894 -32400
# Node ID 0a4a349799b0ded836dfc7e75c87b742672e456a
# Parent  24f77b8a62fe21a3c0d433cccac41f1476ff1fe6
Bug 1593: HeapStats agent handles concurrent mode failure incorrectly.
reviewed-by: ykubota

diff -r 24f77b8a62fe -r 0a4a349799b0 agent/ChangeLog
--- a/agent/ChangeLog	Tue Nov 19 21:22:04 2013 +0900
+++ b/agent/ChangeLog	Thu Nov 21 15:21:34 2013 +0900
@@ -1,3 +1,7 @@
+2013-11-21  Yasumasa Suenaga  <suenaga.yasumasa@lab.ntt.co.jp>
+
+	* Bug 1593: HeapStats agent handles concurrent mode failure incorrectly.
+
 2013-11-19  KUBOTA Yuji  <kubota.yuji@lab.ntt.co.jp>
 
 	* Bug 1587: Refactor to improve the messaging and gathering the symbols.
diff -r 24f77b8a62fe -r 0a4a349799b0 agent/src/oopUtil.cpp
--- a/agent/src/oopUtil.cpp	Tue Nov 19 21:22:04 2013 +0900
+++ b/agent/src/oopUtil.cpp	Thu Nov 21 15:21:34 2013 +0900
@@ -224,29 +224,6 @@
  */
 bool *useG1       = NULL;
 
-/* Variable for CMS collector state. */
-
-/*!
- * \brief CMS collector is idling.
- */
-const int CMS_IDLING         = 2;
-/*!
- * \brief CMS collector is initial-marking phase.
- */
-const int CMS_INITIALMARKING = 3;
-/*!
- * \brief CMS collector is marking phase.
- */
-const int CMS_MARKING        = 4;
-/*!
- * \brief CMS collector is final-making phase.
- */
-const int CMS_FINALMARKING   = 7;
-/*!
- * \brief CMS collector is sweep phase.
- */
-const int CMS_SWEEPING       = 8;
-
 /*!
  * \brief CMS collector state pointer.
  * \sa concurrentMarkSweepGeneration.hpp<br>
@@ -897,6 +874,11 @@
 THeapObjectCallback gcCallbackFunc = NULL;
 
 /*!
+ * \brief Callback function for CMSGC function hooking.
+ */
+THeapObjectCallback cmsCallbackFunc = NULL;
+
+/*!
  * \brief Callback function for JVMTI iterate hooking.
  */
 THeapObjectCallback jvmtiIteCallbackFunc = NULL;
@@ -945,6 +927,11 @@
 bool isInvokeGCManyTimes = false;
 
 /*!
+ *  * \brief Flag of called parallel gc on using CMS gc.
+ */
+bool isInvokedParallelGC = false;
+
+/*!
  * \brief VTable list which should be hooked.
  */
 void *VTableForTypeArrayOopClosure[2] __attribute__((aligned(16)));
@@ -1222,10 +1209,11 @@
 
 /*!
  * \brief Callback function for snapshot.
+ * \param event [in] Callback function.
  * \param oop [in] Java heap object(OopDesc format).
  * \warning Param "oop" isn't usable for JVMTI and JNI.
  */
-inline void callbackForSnapshot(void *oop) {
+inline void callbackForSnapshot(THeapObjectCallback event, void *oop) {
   
   if (likely(gcCallbackFunc != NULL)) {
     void *klassOop = NULL;
@@ -1240,7 +1228,7 @@
     }
     
     /* Invoke callback. */
-    gcCallbackFunc(klassOop, size);
+    event(klassOop, size);
   }
 }
 
@@ -1253,7 +1241,8 @@
 extern "C" void callbackForParallel(void *oop) {
   
   /* Invoke snapshot callback. */
-  callbackForSnapshot(oop);
+  callbackForSnapshot(gcCallbackFunc, oop);
+  isInvokedParallelGC = true;
 }
 
 /*!
@@ -1264,7 +1253,8 @@
 extern "C" void callbackForParOld(void *oop) {
   
   /* Invoke snapshot callback. */
-  callbackForSnapshot(oop);
+  callbackForSnapshot(gcCallbackFunc, oop);
+  isInvokedParallelGC = true;
 }
 
 /*!
@@ -1285,7 +1275,7 @@
   }
 
   /* Invoke snapshot callback. */
-  callbackForSnapshot(*oop);
+  callbackForSnapshot(gcCallbackFunc, *oop);
 }
 
 /*!
@@ -1317,14 +1307,22 @@
       /* Skip. because collect object in CMS gen by callbackForSweep. */
       return;
     }
-  } else if (*useG1 && checkObjectMap->checkAndMark(oop)) {
-    
-    /* Object is already collected by G1GC collector. */
-    return;
+
+    /* Invoke callback by CMS GC. */
+    callbackForSnapshot(cmsCallbackFunc, oop);
+  }
+  else if(*useG1){
+
+    if(checkObjectMap->checkAndMark(oop)){
+      /* Object is already collected by G1GC collector. */
+      return;
+    }
+
+    /* Invoke snapshot callback. */
+    callbackForSnapshot(gcCallbackFunc, oop);
+    isInvokedParallelGC = true;
   }
   
-  /* Invoke snapshot callback. */
-  callbackForSnapshot(oop);
 }
 
 /*!
@@ -1339,7 +1337,7 @@
   if (likely(isMarkedObject(oop))) {
     
     /* Invoke snapshot callback. */
-    callbackForSnapshot(oop);
+    callbackForSnapshot(cmsCallbackFunc, oop);
   }
 }
 
@@ -1466,7 +1464,7 @@
   switchOverrideFunction(funcs, false);
 
   /* Discard existed snapshot data */
-  snapshotByGC->clear();
+  snapshotByGC->clear(false);
   checkObjectMap->clear();
 }
 
@@ -2285,18 +2283,20 @@
  * \brief Setup hooking.
  * \warning Please this function call at after Agent_OnLoad.
  * \param funcOnGC     [in] Pointer of GC callback function.
+ * \param funcOnCMS    [in] Pointer of CMSGC callback function.
  * \param funcOnJVMTI  [in] Pointer of JVMTI callback function.
  * \param funcOnAdjust [in] Pointer of adjust class callback function.
  * \param funcOnG1GC   [in] Pointer of event callback on G1GC finished.
  * \param maxMemSize   [in] Allocatable maximum memory size of JVM.
  * \return Process result.
  */
-bool setupHook(THeapObjectCallback funcOnGC, THeapObjectCallback funcOnJVMTI,
-  TKlassAdjustCallback funcOnAdjust, TCommonCallback funcOnG1GC,
-  size_t maxMemSize) {
+bool setupHook(THeapObjectCallback funcOnGC, THeapObjectCallback funcOnCMS,
+          THeapObjectCallback funcOnJVMTI, TKlassAdjustCallback funcOnAdjust,
+                                 TCommonCallback funcOnG1GC, size_t maxMemSize){
   
   /* Set function. */
   gcCallbackFunc       = funcOnGC;
+  cmsCallbackFunc      = funcOnCMS;
   jvmtiIteCallbackFunc = funcOnJVMTI;
   adjustCallbackFunc   = funcOnAdjust;
   g1FinishCallbackFunc = funcOnG1GC;
@@ -2362,24 +2362,26 @@
 /*!
  * \brief Check CMS garbage collector state.
  * \param state        [in]  State of CMS garbage collector.
- * \param gcCause      [in]  String of GC cause.
  * \param needSnapShot [out] Is need snapshot now.
+ * \return CMS collector state.
  * \warning Please don't forget call on JVM death.
  */
-void checkCMSState(TGCState state, char *gcCause, bool *needSnapShot) {
+int checkCMSState(TGCState state, bool *needSnapShot) {
   
   /* Sanity check. */
   if (unlikely(needSnapShot == NULL)) {
     PRINT_WARN_MSG("Illegal paramter!");
-    return;
+    return -1;
   }
   
   *needSnapShot = false;
+  static int cmsStateAtStart;
+
   switch (state) {
     case gcStart:
       /* Call on JVMTI GC start. */
       
-      if (*CMS_collectorState <= CMS_IDLING) {
+      if (*CMS_collectorState <= CMS_INITIALMARKING) {
         
         /* Set stored flag of passed sweep phase. */
         *needSnapShot = needSnapShotByCMSPhase;
@@ -2391,6 +2393,9 @@
         SELECT_HOOK_FUNCS(funcs, cms_new)
         switchOverrideFunction(funcs, true);
       }
+
+      cmsStateAtStart = *CMS_collectorState;
+      isInvokedParallelGC = false;
       break;
       
     case gcFinish:
@@ -2402,36 +2407,27 @@
           
         case CMS_SWEEPING:
           /* CMS sweep phase. */
-          THookFunctionInfo *funcs;
-          SELECT_HOOK_FUNCS(funcs, cms_new)
-          switchOverrideFunction(funcs, false);
           needSnapShotByCMSPhase = true;
-          
           break;
           
         default:
-          /* CMS other phase. */
-          const char *notCMSGCCause[] = {
-            /* Call by Java.Lang.System. */
-            "System.gc()",
-            /* Call by JVMTI.            */
-            "JvmtiEnv ForceGarbageCollection",
-            /* End flag.                 */
-            NULL
-          };
-          
-          if (likely(gcCause != NULL)) {
-            /* Check this GC. */
-            for (int i = 0; notCMSGCCause[i] != NULL; i++) {
-              if (strcmp(gcCause, notCMSGCCause[i]) == 0) {
-                
-                /* GC invoke by user or service without CMS. */
-                *needSnapShot = true;
-                break;
-              }
-            }
+          /* If called parallel gc. */
+          if(isInvokedParallelGC){
+            /* GC invoke by user or service without CMS. */
+            *needSnapShot = true;
+            needSnapShotByCMSPhase = false;
           }
+
       }
+
+      /* If enable cms new gen hook. */
+      if(cmsStateAtStart == CMS_FINALMARKING){
+        /* switch hooking for CMS new generation. */
+          THookFunctionInfo *funcs;
+          SELECT_HOOK_FUNCS(funcs, cms_new)
+          switchOverrideFunction(funcs, false);
+      }
+
       break;
       
     case gcLast:
@@ -2446,6 +2442,8 @@
       /* Not reached here. */
       PRINT_WARN_MSG("Illegal GC state.");
   }
+
+  return *CMS_collectorState;
 }
 
 /*!
diff -r 24f77b8a62fe -r 0a4a349799b0 agent/src/oopUtil.hpp
--- a/agent/src/oopUtil.hpp	Tue Nov 19 21:22:04 2013 +0900
+++ b/agent/src/oopUtil.hpp	Thu Nov 21 15:21:34 2013 +0900
@@ -117,6 +117,35 @@
   gcLast   = 3, /*!< Garbage collector is tarminating on JVM death. */
 } TGCState;
 
+
+/* Variable for CMS collector state. */
+
+/*!
+ * \brief CMS collector is idling.
+ */
+#define CMS_IDLING 2
+
+/*!
+ * \brief CMS collector is initial-marking phase.
+ */
+#define CMS_INITIALMARKING 3
+
+/*!
+ * \brief CMS collector is marking phase.
+ */
+#define CMS_MARKING 4
+
+/*!
+ * \brief CMS collector is final-making phase.
+ */
+#define CMS_FINALMARKING 7
+
+/*!
+ * \brief CMS collector is sweep phase.
+ */
+#define CMS_SWEEPING 8
+
+
 /* Macro. */
 
 /*!
@@ -408,12 +437,13 @@
  * \warning Please this function call at after Agent_OnLoad.
  * \param funcOnGC     [in] Pointer of GC callback function.
  * \param funcOnJVMTI  [in] Pointer of JVMTI callback function.
+ * \param funcOnCMS    [in] Pointer of CMSGC callback function.
  * \param funcOnAdjust [in] Pointer of adjust class callback function.
  * \param funcOnG1GC   [in] Pointer of event callback on G1GC finished.
  * \param maxMemSize   [in] Allocatable maximum memory size of JVM.
  * \return Process result.
  */
-bool setupHook(THeapObjectCallback funcOnGC,
+bool setupHook(THeapObjectCallback funcOnGC, THeapObjectCallback funcOnCMS,
           THeapObjectCallback funcOnJVMTI, TKlassAdjustCallback funcOnAdjust,
                                   TCommonCallback funcOnG1GC, size_t maxMemSize);
 
@@ -462,11 +492,11 @@
 /*!
  * \brief Check CMS garbage collector state.
  * \param state        [in]  State of CMS garbage collector.
- * \param gcCause      [in]  String of GC cause.
  * \param needSnapShot [out] Is need snapshot now.
+ * \return CMS collector state.
  * \warning Please don't forget call on JVM death.
  */
-void checkCMSState(TGCState state, char *gcCause, bool *needSnapShot);
+int checkCMSState(TGCState state, bool *needSnapShot);
 
 /*!
  * \brief Setup hooking for inner GC event.
diff -r 24f77b8a62fe -r 0a4a349799b0 agent/src/snapShotContainer.cpp
--- a/agent/src/snapShotContainer.cpp	Tue Nov 19 21:22:04 2013 +0900
+++ b/agent/src/snapShotContainer.cpp	Thu Nov 21 15:21:34 2013 +0900
@@ -133,7 +133,7 @@
   if (unlikely(instance == NULL)) {
     return;
   }
-  
+
   unsigned int stockCount = MAX_STOCK_COUNT;
   ENTER_PTHREAD_SECTION(&instanceLocker) {
     stockCount = stockQueue->size();
@@ -146,13 +146,14 @@
   } else {
     
     /* Clear data. */
-    instance->clear();
+    instance->clear(false);
     
     ENTER_PTHREAD_SECTION(&instanceLocker) {
       /* Keep instance. */
       stockQueue->push(instance);
     } EXIT_PTHREAD_SECTION(&instanceLocker)
   }
+
 }
 
 /*!
@@ -172,6 +173,8 @@
     throw ret;
   }
 
+  this->isCleared = true;
+
   this->counterBuf->count = 1;
   tlsLock = 0;
   pthread_key_create(&tlsKey, NULL);
@@ -301,7 +304,11 @@
 /*!
  * \brief Clear snapshot data.
  */
-void TSnapShotContainer::clear(void){
+void TSnapShotContainer::clear(bool isForce){
+
+  if(!isForce && this->isCleared){
+    return;
+  }
 
   /* Phase1: clear parent counter */
 
@@ -340,9 +347,10 @@
   /* Phase2: clear counters in TLS */
   for(TLocalSShotContainer::iterator itr = this->sshotContainerMap.begin();
                                   itr != this->sshotContainerMap.end(); itr++){
-    itr->second->clear();
+    itr->second->clear(true);
   }
 
+  this->isCleared = true;
 }
 
 /*!
diff -r 24f77b8a62fe -r 0a4a349799b0 agent/src/snapShotContainer.hpp
--- a/agent/src/snapShotContainer.hpp	Tue Nov 19 21:22:04 2013 +0900
+++ b/agent/src/snapShotContainer.hpp	Thu Nov 21 15:21:34 2013 +0900
@@ -306,7 +306,7 @@
     /*!
      * \brief Clear snapshot data.
      */
-    void clear(void);
+    void clear(bool isForce);
 
     /*!
      * \brief Get TLS SnapShotContainer.
@@ -354,6 +354,13 @@
      */
     void mergeChildren(void);
 
+    /*!
+     * \brief Set "isCleared" flag.
+     */
+    inline void setIsCleared(bool flag){
+      this->isCleared = flag;
+    }
+
   protected:
     /*!
      * \brief TSnapshotContainer constructor.
@@ -419,6 +426,12 @@
      * \brief Now processing SnapShotContainer in TLS.
      */
     pthread_key_t snapShotKey;
+
+   /*!
+    * \brief Is this container is cleared ?
+    */
+   volatile bool isCleared;
+
 };
 
 #endif // _SNAPSHOT_CONTAINER_HPP
diff -r 24f77b8a62fe -r 0a4a349799b0 agent/src/snapShotMain.cpp
--- a/agent/src/snapShotMain.cpp	Tue Nov 19 21:22:04 2013 +0900
+++ b/agent/src/snapShotMain.cpp	Thu Nov 21 15:21:34 2013 +0900
@@ -100,6 +100,10 @@
  */
 TSnapShotContainer *snapshotByGC    = NULL;
 /*!
+ * \brief Container instance stored got snapshot by CMSGC.
+ */
+TSnapShotContainer *snapshotByCMS   = NULL;
+/*!
  * \brief Container instance stored got snapshot by interval or dump request.
  */
 TSnapShotContainer *snapshotByJvmti = NULL;
@@ -213,7 +217,7 @@
   jvmInfo->resumeGCinfo();
 
   /* Clear unfinished snapshot data. */
-  snapshotByGC->clear();
+  snapshotByGC->clear(false);
 }
 
 
@@ -260,17 +264,21 @@
   snapshotByGC = TSnapShotContainer::getInstance();
 }
 
-/*!
+ /*!
  * \brief Count object size in Heap.
+ * \param snapshot [in] Snapshot instance.
  * \param klassOop [in] Pointer of java class object(KlassOopDesc).
  * \param size     [in] Size of object instance.
  */
-void HeapObjectCallbackOnGC(void *klassOop, jlong size) {
+void calculateObjectUsage(TSnapShotContainer *snapshot,
+                                       void *klassOop, jlong size){
   
   /* Sanity check. */
-  if (unlikely(snapshotByGC == NULL)) {
+  if (unlikely(snapshot == NULL)) {
     return;
   }
+
+  snapshot->setIsCleared(false);
   
   TObjectCounter *counter = NULL;
   TObjectData *objData = NULL;
@@ -295,7 +303,7 @@
 
   if (likely(objData != NULL)) {
 
-    my_container = snapshotByGC->getTLSContainer();
+    my_container = snapshot->getTLSContainer();
 
     /* Search class. */
     counter = my_container->findClass(objData);
@@ -315,32 +323,30 @@
 }
 
 /*!
+ * \brief Count object size in Heap.
+ * \param klassOop [in] Pointer of java class object(KlassOopDesc).
+ * \param size     [in] Size of object instance.
+ */
+void HeapObjectCallbackOnGC(void *klassOop, jlong size) {
+  calculateObjectUsage(snapshotByGC, klassOop, size);
+}
+
+/*!
+ * \brief Count object size in Heap by CMSGC.
+ * \param klassOop [in] Pointer of java class object(KlassOopDesc).
+ * \param size     [in] Size of object instance.
+ */
+void HeapObjectCallbackOnCMS(void *klassOop, jlong size) {
+  calculateObjectUsage(snapshotByCMS, klassOop, size);
+}
+
+/*!
  * \brief Count object size in Heap by JVMTI iterateOverHeap.
  * \param klassOop [in] Pointer of java class object(KlassOopDesc).
  * \param size     [in] Size of object instance.
  */
 void HeapObjectCallbackOnJvmti(void *klassOop, jlong size) {
-  TObjectCounter *counter = NULL;
-  TObjectData *objData = NULL;
-  
-  /* Search class. */
-  objData = clsContainer->findClass(klassOop);
-  
-  /* Search counter. */
-  if (likely(objData != NULL)) {
-    counter = snapshotByJvmti->findClass(objData);
-    
-    if (unlikely(counter == NULL)) {
-      counter = snapshotByJvmti->pushNewClass(objData);
-    }
-
-  }
-  
-  if (likely(counter != NULL)) {
-    /* Count class size and instance count. */
-    snapshotByJvmti->Inc(counter, size);
-  }
-
+  calculateObjectUsage(snapshotByJvmti, klassOop, size);
 }
 
 /*!
@@ -361,7 +367,7 @@
   
   /* Get CMS state. */
   bool needShapShot = false;
-  checkCMSState(gcStart, NULL, &needShapShot);
+  int cmsState = checkCMSState(gcStart, &needShapShot);
   
   /* If occurred snapshot target GC. */
   if(needShapShot){
@@ -369,14 +375,24 @@
     /* If need getting snapshot. */
     if(gcWatcher->needToStartGCTrigger()){
       /* Set information and push waiting queue. */
-      outputSnapShotByGC(snapshotByGC);
-      snapshotByGC = NULL;
+      outputSnapShotByGC(snapshotByCMS);
+      snapshotByCMS = NULL;
     }
   }
   
   if(likely(snapshotByGC == NULL)){
     snapshotByGC = TSnapShotContainer::getInstance();
   }
+  else{
+    snapshotByGC->clear(false);
+  }
+
+  if(likely(snapshotByCMS == NULL)){
+    snapshotByCMS = TSnapShotContainer::getInstance();
+  }
+  else if(cmsState == CMS_FINALMARKING){
+    snapshotByCMS->clear(false);
+  }
 
   /* Enable inner GC event. */
   setupHookForInnerGCEvent(true, &onInnerGarbageCollectionInterrupt);
@@ -387,10 +403,12 @@
  * \param jvmti [in] JVMTI environment object.
  */
 void JNICALL OnCMSGCFinish(jvmtiEnv *jvmti) {
+  /* Disable inner GC event. */
+  setupHookForInnerGCEvent(false, NULL);
   
   /* Get CMS state. */
   bool needShapShot = false;
-  checkCMSState(gcFinish, jvmInfo->getGCCause(), &needShapShot);
+  checkCMSState(gcFinish, &needShapShot);
   
   /* If occurred snapshot target GC. */
   if(needShapShot){
@@ -400,6 +418,7 @@
       /* Set information and push waiting queue. */
       outputSnapShotByGC(snapshotByGC);
       snapshotByGC = NULL;
+      snapshotByCMS->clear(false);
     }
   }
 
@@ -600,8 +619,9 @@
 void onVMInitForSnapShot(jvmtiEnv *jvmti, JNIEnv *env) {
   size_t maxMemSize = jvmInfo->getMaxMemory();
   /* Setup for hooking. */
-  setupHook(&HeapObjectCallbackOnGC, &HeapObjectCallbackOnJvmti,
-    &HeapKlassAdjustCallback, &OnG1GarbageCollectionFinish, maxMemSize);
+  setupHook(&HeapObjectCallbackOnGC, &HeapObjectCallbackOnCMS,
+                  &HeapObjectCallbackOnJvmti, &HeapKlassAdjustCallback,
+                                       &OnG1GarbageCollectionFinish, maxMemSize);
   
   /* JVMTI Extension Event Setup. */
   int eventIdx = GetClassUnloadingExtEventIndex(jvmti);
@@ -638,14 +658,14 @@
   if (*useCMS) {
     /* Get CMS state. */
     bool needShapShot = false;
-    checkCMSState(gcLast, NULL, &needShapShot);
+    checkCMSState(gcLast, &needShapShot);
     
     /* If occurred snapshot target GC. */
-    if (needShapShot && snapshotByGC != NULL) {
+    if (needShapShot && snapshotByCMS != NULL) {
       
       /* Output snapshot. */
-      outputSnapShotByGC(snapshotByGC);
-      snapshotByGC = NULL;
+      outputSnapShotByGC(snapshotByCMS);
+      snapshotByCMS = NULL;
     }
   }