8263387: G1GarbageCollection JFR event gets gc phase, not gc type

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -43,7 +43,7 @@
 #include "gc/g1/g1FullCollector.hpp"
 #include "gc/g1/g1GCParPhaseTimesTracker.hpp"
 #include "gc/g1/g1GCPhaseTimes.hpp"
-#include "gc/g1/g1GCTypes.hpp"
+#include "gc/g1/g1GCPauseType.hpp"
 #include "gc/g1/g1HeapSizingPolicy.hpp"
 #include "gc/g1/g1HeapTransition.hpp"
 #include "gc/g1/g1HeapVerifier.hpp"
@@ -2882,6 +2882,21 @@ bool G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_
   return true;
 }
 
+void G1CollectedHeap::gc_tracer_report_gc_start() {
+  _gc_timer_stw->register_gc_start();
+  _gc_tracer_stw->report_gc_start(gc_cause(), _gc_timer_stw->gc_start());
+}
+
+void G1CollectedHeap::gc_tracer_report_gc_end(bool concurrent_operation_is_full_mark,
+                                              G1EvacuationInfo& evacuation_info) {
+  _gc_tracer_stw->report_evacuation_info(&evacuation_info);
+  _gc_tracer_stw->report_tenuring_threshold(_policy->tenuring_threshold());
+
+  _gc_timer_stw->register_gc_end();
+  _gc_tracer_stw->report_gc_end(_gc_timer_stw->gc_end(),
+  _gc_timer_stw->time_partitions());
+}
+
 void G1CollectedHeap::do_collection_pause_at_safepoint_helper(double target_pause_time_ms) {
   GCIdMark gc_id_mark;
 
@@ -2890,8 +2905,7 @@ void G1CollectedHeap::do_collection_pause_at_safepoint_helper(double target_paus
 
   policy()->note_gc_start();
 
-  _gc_timer_stw->register_gc_start();
-  _gc_tracer_stw->report_gc_start(gc_cause(), _gc_timer_stw->gc_start());
+  gc_tracer_report_gc_start();
 
   wait_for_root_region_scanning();
 
@@ -2926,8 +2940,6 @@ void G1CollectedHeap::do_collection_pause_at_safepoint_helper(double target_paus
   {
     G1EvacuationInfo evacuation_info;
 
-    _gc_tracer_stw->report_yc_phase(collector_state()->young_gc_phase());
-
     GCTraceCPUTime tcpu;
 
     GCTraceTime(Info, gc) tm(young_gc_name(), NULL, gc_cause(), true);
@@ -2940,7 +2952,7 @@ void G1CollectedHeap::do_collection_pause_at_safepoint_helper(double target_paus
 
     G1MonitoringScope ms(g1mm(),
                          false /* full_gc */,
-                         collector_state()->young_gc_phase() == Mixed /* all_memory_pools_affected */);
+                         collector_state()->in_mixed_phase() /* all_memory_pools_affected */);
 
     G1HeapTransition heap_transition(this);
 
@@ -3008,6 +3020,10 @@ void G1CollectedHeap::do_collection_pause_at_safepoint_helper(double target_paus
         // evacuation, eventually aborting it.
         concurrent_operation_is_full_mark = policy()->concurrent_operation_is_full_mark("Revise IHOP");
 
+        // Need to report the collection pause now since record_collection_pause_end()
+        // modifies it to the next state.
+        _gc_tracer_stw->report_yc_pause(collector_state()->young_gc_pause_type(concurrent_operation_is_full_mark));
+
         double sample_end_time_sec = os::elapsedTime();
         double pause_time_ms = (sample_end_time_sec - sample_start_time_sec) * MILLIUNITS;
         policy()->record_collection_pause_end(pause_time_ms, concurrent_operation_is_full_mark);
@@ -3042,10 +3058,7 @@ void G1CollectedHeap::do_collection_pause_at_safepoint_helper(double target_paus
     // before any GC notifications are raised.
     g1mm()->update_sizes();
 
-    _gc_tracer_stw->report_evacuation_info(&evacuation_info);
-    _gc_tracer_stw->report_tenuring_threshold(_policy->tenuring_threshold());
-    _gc_timer_stw->register_gc_end();
-    _gc_tracer_stw->report_gc_end(_gc_timer_stw->gc_end(), _gc_timer_stw->time_partitions());
+    gc_tracer_report_gc_end(concurrent_operation_is_full_mark, evacuation_info);
   }
   // It should now be safe to tell the concurrent mark thread to start
   // without its logging output interfering with the logging output

src/hotspot/share/gc/g1/g1CollectedHeap.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -36,7 +36,7 @@
 #include "gc/g1/g1EvacStats.hpp"
 #include "gc/g1/g1EvacuationInfo.hpp"
 #include "gc/g1/g1GCPhaseTimes.hpp"
-#include "gc/g1/g1GCTypes.hpp"
+#include "gc/g1/g1GCPauseType.hpp"
 #include "gc/g1/g1HeapTransition.hpp"
 #include "gc/g1/g1HeapVerifier.hpp"
 #include "gc/g1/g1HRPrinter.hpp"
@@ -388,6 +388,9 @@ class G1CollectedHeap : public CollectedHeap {
 
   G1NewTracer* _gc_tracer_stw;
 
+  void gc_tracer_report_gc_start();
+  void gc_tracer_report_gc_end(bool concurrent_operation_is_full_mark, G1EvacuationInfo& evacuation_info);
+
   // The current policy object for the collector.
   G1Policy* _policy;
   G1HeapSizingPolicy* _heap_sizing_policy;

src/hotspot/share/gc/g1/g1CollectorState.cpp

@@ -24,37 +24,24 @@
 
 #include "precompiled.hpp"
 #include "gc/g1/g1CollectorState.hpp"
-#include "gc/g1/g1GCTypes.hpp"
+#include "gc/g1/g1GCPauseType.hpp"
 
 G1GCPauseType G1CollectorState::young_gc_pause_type(bool concurrent_operation_is_full_mark) const {
   assert(!in_full_gc(), "must be");
   if (in_concurrent_start_gc()) {
     assert(!in_young_gc_before_mixed(), "must be");
-    return concurrent_operation_is_full_mark ? ConcurrentStartMarkGC : ConcurrentStartUndoGC;
+    return concurrent_operation_is_full_mark ? G1GCPauseType::ConcurrentStartMarkGC :
+                                               G1GCPauseType::ConcurrentStartUndoGC;
   } else if (in_young_gc_before_mixed()) {
     assert(!in_concurrent_start_gc(), "must be");
-    return LastYoungGC;
+    return G1GCPauseType::LastYoungGC;
   } else if (in_mixed_phase()) {
     assert(!in_concurrent_start_gc(), "must be");
     assert(!in_young_gc_before_mixed(), "must be");
-    return MixedGC;
+    return G1GCPauseType::MixedGC;
   } else {
     assert(!in_concurrent_start_gc(), "must be");
     assert(!in_young_gc_before_mixed(), "must be");
-    return YoungGC;
-  }
-}
-
-G1GCYoungPhase G1CollectorState::young_gc_phase() const {
-  assert(!in_full_gc(), "must be");
-
-  if (in_concurrent_start_gc()) {
-    return ConcurrentStart;
-  } else if (mark_or_rebuild_in_progress()) {
-    return DuringMarkOrRebuild;
-  } else if (in_young_only_phase()) {
-    return Normal;
-  } else {
-    return Mixed;
+    return G1GCPauseType::YoungGC;
   }
 }

src/hotspot/share/gc/g1/g1CollectorState.hpp

@@ -25,7 +25,7 @@
 #ifndef SHARE_GC_G1_G1COLLECTORSTATE_HPP
 #define SHARE_GC_G1_G1COLLECTORSTATE_HPP
 
-#include "gc/g1/g1GCTypes.hpp"
+#include "gc/g1/g1GCPauseType.hpp"
 #include "utilities/globalDefinitions.hpp"
 
 // State of the G1 collection.
@@ -112,8 +112,6 @@ class G1CollectorState {
 
   // Calculate GC Pause Type from internal state.
   G1GCPauseType young_gc_pause_type(bool concurrent_operation_is_full_mark) const;
-  G1GCYoungPhase young_gc_phase() const;
-
 };
 
 #endif // SHARE_GC_G1_G1COLLECTORSTATE_HPP

src/hotspot/share/gc/g1/g1GCTypes.hpp → src/hotspot/share/gc/g1/g1GCPauseType.hpp

@@ -22,21 +22,13 @@
  *
  */
 
-#ifndef SHARE_GC_G1_G1GCTYPES_HPP
-#define SHARE_GC_G1_G1GCTYPES_HPP
+#ifndef SHARE_GC_G1_G1GCPAUSETYPES_HPP
+#define SHARE_GC_G1_G1GCPAUSETYPES_HPP
 
 #include "utilities/debug.hpp"
+#include "utilities/enumIterator.hpp"
 
-// Enumarate the phases in which the collection cycle can be.
-enum G1GCYoungPhase {
-  Normal,
-  ConcurrentStart,
-  DuringMarkOrRebuild,
-  Mixed,
-  G1GCYoungPhaseEndSentinel
-};
-
-enum G1GCPauseType {
+enum class G1GCPauseType : uint {
   YoungGC,
   LastYoungGC,
   ConcurrentStartMarkGC,
@@ -45,50 +37,55 @@ enum G1GCPauseType {
   Remark,
   MixedGC,
   FullGC,
-  G1GCPauseTypeEndSentinel
+  Invalid
 };
 
-class G1GCTypeHelper {
- public:
+ENUMERATOR_RANGE(G1GCPauseType, G1GCPauseType::YoungGC, G1GCPauseType::FullGC)
+
+class G1GCPauseTypeHelper {
+public:
 
   static void assert_is_young_pause(G1GCPauseType type) {
-    assert(type != FullGC, "must be");
-    assert(type != Remark, "must be");
-    assert(type != Cleanup, "must be");
+    assert(type != G1GCPauseType::FullGC, "must be");
+    assert(type != G1GCPauseType::Remark, "must be");
+    assert(type != G1GCPauseType::Cleanup, "must be");
   }
 
   static bool is_young_only_pause(G1GCPauseType type) {
     assert_is_young_pause(type);
-    return type == ConcurrentStartUndoGC ||
-           type == ConcurrentStartMarkGC ||
-           type == LastYoungGC ||
-           type == YoungGC;
+    return type == G1GCPauseType::ConcurrentStartUndoGC ||
+           type == G1GCPauseType::ConcurrentStartMarkGC ||
+           type == G1GCPauseType::LastYoungGC ||
+           type == G1GCPauseType::YoungGC;
   }
 
   static bool is_mixed_pause(G1GCPauseType type) {
     assert_is_young_pause(type);
-    return type == MixedGC;
+    return type == G1GCPauseType::MixedGC;
   }
 
   static bool is_last_young_pause(G1GCPauseType type) {
     assert_is_young_pause(type);
-    return type == LastYoungGC;
+    return type == G1GCPauseType::LastYoungGC;
   }
 
   static bool is_concurrent_start_pause(G1GCPauseType type) {
     assert_is_young_pause(type);
-    return type == ConcurrentStartMarkGC || type == ConcurrentStartUndoGC;
+    return type == G1GCPauseType::ConcurrentStartMarkGC || type == G1GCPauseType::ConcurrentStartUndoGC;
   }
 
-  static const char* to_string(G1GCYoungPhase type) {
-    switch(type) {
-      case Normal: return "Normal";
-      case ConcurrentStart: return "Concurrent Start";
-      case DuringMarkOrRebuild: return "During Mark";
-      case Mixed: return "Mixed";
-      default: ShouldNotReachHere(); return NULL;
-    }
+  static const char* to_string(G1GCPauseType type) {
+    static const char* pause_strings[] = { "Normal",
+                                           "Prepare Mixed",
+                                           "Concurrent Start Mark",
+                                           "Concurrent Start Undo",
+                                           "Cleanup",
+                                           "Remark",
+                                           "Mixed",
+                                           "Full",
+                                           "Invalid"};
+    return pause_strings[static_cast<uint>(type)];
   }
 };
 
-#endif // SHARE_GC_G1_G1GCTYPES_HPP
+#endif // SHARE_GC_G1_G1GCPAUSETYPES_HPP