8215221: Serial GC misreports young GC time

Reviewed-by: kbarrett, manc

src/hotspot/share/gc/shared/genCollectedHeap.cpp

@@ -557,8 +557,6 @@ void GenCollectedHeap::do_collection(bool           full,
     return; // GC is disabled (e.g. JNI GetXXXCritical operation)
   }
 
-  GCIdMark gc_id_mark;
-
   const bool do_clear_all_soft_refs = clear_all_soft_refs ||
                           soft_ref_policy()->should_clear_all_soft_refs();
 
@@ -566,42 +564,35 @@ void GenCollectedHeap::do_collection(bool           full,
 
   const size_t metadata_prev_used = MetaspaceUtils::used_bytes();
 
-  print_heap_before_gc();
 
-  {
   FlagSetting fl(_is_gc_active, true);
 
   bool complete = full && (max_generation == OldGen);
   bool old_collects_young = complete && !ScavengeBeforeFullGC;
   bool do_young_collection = !old_collects_young && _young_gen->should_collect(full, size, is_tlab);
 
-    FormatBuffer<> gc_string("%s", "Pause ");
-    if (do_young_collection) {
-      gc_string.append("Young");
-    } else {
-      gc_string.append("Full");
-    }
-
-    GCTraceCPUTime tcpu;
-    GCTraceTime(Info, gc) t(gc_string, NULL, gc_cause(), true);
-
-    gc_prologue(complete);
-    increment_total_collections(complete);
-
   size_t young_prev_used = _young_gen->used();
   size_t old_prev_used = _old_gen->used();
 
   bool run_verification = total_collections() >= VerifyGCStartAt;
-
   bool prepared_for_verification = false;
-    bool collected_old = false;
+  bool do_full_collection = false;
 
   if (do_young_collection) {
+    GCIdMark gc_id_mark;
+    GCTraceCPUTime tcpu;
+    GCTraceTime(Info, gc) t("Pause Young", NULL, gc_cause(), true);
+
+    print_heap_before_gc();
+
     if (run_verification && VerifyGCLevel <= 0 && VerifyBeforeGC) {
       prepare_for_verify();
       prepared_for_verification = true;
     }
 
+    gc_prologue(complete);
+    increment_total_collections(complete);
+
     collect_generation(_young_gen,
                        full,
                        size,
@@ -615,54 +606,72 @@ void GenCollectedHeap::do_collection(bool           full,
       // Allocation request was met by young GC.
       size = 0;
     }
+
+    // Ask if young collection is enough. If so, do the final steps for young collection,
+    // and fallthrough to the end.
+    do_full_collection = should_do_full_collection(size, full, is_tlab, max_generation);
+    if (!do_full_collection) {
+      // Adjust generation sizes.
+      _young_gen->compute_new_size();
+
+      print_heap_change(young_prev_used, old_prev_used);
+      MetaspaceUtils::print_metaspace_change(metadata_prev_used);
+
+      // Track memory usage and detect low memory after GC finishes
+      MemoryService::track_memory_usage();
+
+      gc_epilogue(complete);
     }
 
-    bool must_restore_marks_for_biased_locking = false;
+    print_heap_after_gc();
 
-    if (max_generation == OldGen && _old_gen->should_collect(full, size, is_tlab)) {
+  } else {
-      if (!complete) {
+    // No young collection, ask if we need to perform Full collection.
-        // The full_collections increment was missed above.
+    do_full_collection = should_do_full_collection(size, full, is_tlab, max_generation);
-        increment_total_full_collections();
   }
 
+  if (do_full_collection) {
+    GCIdMark gc_id_mark;
+    GCTraceCPUTime tcpu;
+    GCTraceTime(Info, gc) t("Pause Full", NULL, gc_cause(), true);
+
+    print_heap_before_gc();
+
     if (!prepared_for_verification && run_verification &&
         VerifyGCLevel <= 1 && VerifyBeforeGC) {
       prepare_for_verify();
     }
 
-      if (do_young_collection) {
+    if (!do_young_collection) {
-        // We did a young GC. Need a new GC id for the old GC.
+      gc_prologue(complete);
-        GCIdMark gc_id_mark;
+      increment_total_collections(complete);
-        GCTraceTime(Info, gc) t("Pause Full", NULL, gc_cause(), true);
-        collect_generation(_old_gen, full, size, is_tlab, run_verification && VerifyGCLevel <= 1, do_clear_all_soft_refs, true);
-      } else {
-        // No young GC done. Use the same GC id as was set up earlier in this method.
-        collect_generation(_old_gen, full, size, is_tlab, run_verification && VerifyGCLevel <= 1, do_clear_all_soft_refs, true);
     }
 
-      must_restore_marks_for_biased_locking = true;
+    // Accounting quirk: total full collections would be incremented when "complete"
-      collected_old = true;
+    // is set, by calling increment_total_collections above. However, we also need to
+    // account Full collections that had "complete" unset.
+    if (!complete) {
+      increment_total_full_collections();
     }
 
-    // Update "complete" boolean wrt what actually transpired --
+    collect_generation(_old_gen,
-    // for instance, a promotion failure could have led to
+                       full,
-    // a whole heap collection.
+                       size,
-    complete = complete || collected_old;
+                       is_tlab,
+                       run_verification && VerifyGCLevel <= 1,
+                       do_clear_all_soft_refs,
+                       true);
 
     // Adjust generation sizes.
-    if (collected_old) {
     _old_gen->compute_new_size();
-    }
     _young_gen->compute_new_size();
 
-    if (complete) {
     // Delete metaspaces for unloaded class loaders and clean up loader_data graph
     ClassLoaderDataGraph::purge();
     MetaspaceUtils::verify_metrics();
     // Resize the metaspace capacity after full collections
     MetaspaceGC::compute_new_size();
     update_full_collections_completed();
-    }
 
     print_heap_change(young_prev_used, old_prev_used);
     MetaspaceUtils::print_metaspace_change(metadata_prev_used);
@@ -672,18 +681,21 @@ void GenCollectedHeap::do_collection(bool           full,
 
     gc_epilogue(complete);
 
-    if (must_restore_marks_for_biased_locking) {
     BiasedLocking::restore_marks();
-    }
-  }
 
     print_heap_after_gc();
+  }
 
 #ifdef TRACESPINNING
   ParallelTaskTerminator::print_termination_counts();
 #endif
 }
 
+bool GenCollectedHeap::should_do_full_collection(size_t size, bool full, bool is_tlab,
+                                                 GenCollectedHeap::GenerationType max_gen) const {
+  return max_gen == OldGen && _old_gen->should_collect(full, size, is_tlab);
+}
+
 void GenCollectedHeap::register_nmethod(nmethod* nm) {
   CodeCache::register_scavenge_root_nmethod(nm);
 }

src/hotspot/share/gc/shared/genCollectedHeap.hpp

@@ -497,6 +497,10 @@ private:
 
   // Save the tops of the spaces in all generations
   void record_gen_tops_before_GC() PRODUCT_RETURN;
+
+  // Return true if we need to perform full collection.
+  bool should_do_full_collection(size_t size, bool full,
+                                 bool is_tlab, GenerationType max_gen) const;
 };
 
 #endif // SHARE_GC_SHARED_GENCOLLECTEDHEAP_HPP