8368152: Shenandoah: Incorrect behavior at end of degenerated cycle

src/hotspot/share/gc/shenandoah/shenandoahCollectorPolicy.cpp

@@ -37,6 +37,7 @@ ShenandoahCollectorPolicy::ShenandoahCollectorPolicy() :
   _abbreviated_degenerated_gcs(0),
   _success_full_gcs(0),
   _consecutive_degenerated_gcs(0),
+  _consecutive_degenerated_gcs_without_progress(0),
   _consecutive_young_gcs(0),
   _mixed_gcs(0),
   _success_old_gcs(0),
@@ -67,14 +68,14 @@ void ShenandoahCollectorPolicy::record_alloc_failure_to_degenerated(ShenandoahGC
 }
 
 void ShenandoahCollectorPolicy::record_degenerated_upgrade_to_full() {
-  _consecutive_degenerated_gcs = 0;
+  reset_consecutive_degenerated_gcs();
   _alloc_failure_degenerated_upgrade_to_full++;
 }
 
 void ShenandoahCollectorPolicy::record_success_concurrent(bool is_young, bool is_abbreviated) {
   update_young(is_young);
 
-  _consecutive_degenerated_gcs = 0;
+  reset_consecutive_degenerated_gcs();
   _success_concurrent_gcs++;
   if (is_abbreviated) {
     _abbreviated_concurrent_gcs++;
@@ -95,11 +96,18 @@ void ShenandoahCollectorPolicy::record_interrupted_old() {
   _interrupted_old_gcs++;
 }
 
-void ShenandoahCollectorPolicy::record_success_degenerated(bool is_young, bool is_abbreviated) {
+void ShenandoahCollectorPolicy::record_degenerated(bool is_young, bool is_abbreviated, bool progress) {
   update_young(is_young);
 
   _success_degenerated_gcs++;
   _consecutive_degenerated_gcs++;
+
+  if (progress) {
+    _consecutive_degenerated_gcs_without_progress = 0;
+  } else {
+    _consecutive_degenerated_gcs_without_progress++;
+  }
+
   if (is_abbreviated) {
     _abbreviated_degenerated_gcs++;
   }
@@ -114,7 +122,7 @@ void ShenandoahCollectorPolicy::update_young(bool is_young) {
 }
 
 void ShenandoahCollectorPolicy::record_success_full() {
-  _consecutive_degenerated_gcs = 0;
+  reset_consecutive_degenerated_gcs();
   _consecutive_young_gcs = 0;
   _success_full_gcs++;
 }

src/hotspot/share/gc/shenandoah/shenandoahCollectorPolicy.hpp

@@ -42,6 +42,7 @@ class ShenandoahCollectorPolicy : public CHeapObj<mtGC> {
   // Written by control thread, read by mutators
   volatile size_t _success_full_gcs;
   uint _consecutive_degenerated_gcs;
+  uint _consecutive_degenerated_gcs_without_progress;
   volatile size_t _consecutive_young_gcs;
   size_t _mixed_gcs;
   size_t _success_old_gcs;
@@ -55,8 +56,25 @@ class ShenandoahCollectorPolicy : public CHeapObj<mtGC> {
   ShenandoahSharedFlag _in_shutdown;
   ShenandoahTracer* _tracer;
 
+  void reset_consecutive_degenerated_gcs() {
+    _consecutive_degenerated_gcs = 0;
+    _consecutive_degenerated_gcs_without_progress = 0;
+  }
 
 public:
+  // The most common scenario for lack of good progress following a degenerated GC is an accumulation of floating
+  // garbage during the most recently aborted concurrent GC effort.  With generational GC, it is far more effective to
+  // reclaim this floating garbage with another degenerated cycle (which focuses on young generation and might require
+  // a pause of 200 ms) rather than a full GC cycle (which may require over 2 seconds with a 10 GB old generation).
+  //
+  // In generational mode, we'll only upgrade to full GC if we've done two degen cycles in a row and both indicated
+  // bad progress.  In non-generational mode, we'll preserve the original behavior, which is to upgrade to full
+  // immediately following a degenerated cycle with bad progress.  This preserves original behavior of non-generational
+  // Shenandoah to avoid introducing "surprising new behavior."  It also makes less sense with non-generational
+  // Shenandoah to replace a full GC with a degenerated GC, because both have similar pause times in non-generational
+  // mode.
+  static constexpr size_t GENERATIONAL_CONSECUTIVE_BAD_DEGEN_PROGRESS_THRESHOLD = 2;
+
   ShenandoahCollectorPolicy();
 
   void record_mixed_cycle();
@@ -69,7 +87,12 @@ class ShenandoahCollectorPolicy : public CHeapObj<mtGC> {
   // cycles are very efficient and are worth tracking. Note that both degenerated and
   // concurrent cycles can be abbreviated.
   void record_success_concurrent(bool is_young, bool is_abbreviated);
-  void record_success_degenerated(bool is_young, bool is_abbreviated);
+
+  // Record that a degenerated cycle has been completed. Note that such a cycle may or
+  // may not make "progress". We separately track the total number of degenerated cycles,
+  // the number of consecutive degenerated cycles and the number of consecutive cycles that
+  // fail to make good progress.
+  void record_degenerated(bool is_young, bool is_abbreviated, bool progress);
   void record_success_full();
   void record_alloc_failure_to_degenerated(ShenandoahGC::ShenandoahDegenPoint point);
   void record_alloc_failure_to_full();
@@ -94,6 +117,11 @@ class ShenandoahCollectorPolicy : public CHeapObj<mtGC> {
     return _consecutive_degenerated_gcs;
   }
 
+  // Genshen will only upgrade to a full gc after the configured number of futile degenerated cycles.
+  bool generational_should_upgrade_degenerated_gc() const {
+    return _consecutive_degenerated_gcs_without_progress >= GENERATIONAL_CONSECUTIVE_BAD_DEGEN_PROGRESS_THRESHOLD;
+  }
+
   static bool is_allocation_failure(GCCause::Cause cause);
   static bool is_shenandoah_gc(GCCause::Cause cause);
   static bool is_requested_gc(GCCause::Cause cause);

src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp

@@ -49,8 +49,7 @@ ShenandoahDegenGC::ShenandoahDegenGC(ShenandoahDegenPoint degen_point, Shenandoa
   ShenandoahGC(),
   _degen_point(degen_point),
   _generation(generation),
-  _abbreviated(false),
-  _consecutive_degen_with_bad_progress(0) {
+  _abbreviated(false) {
 }
 
 bool ShenandoahDegenGC::collect(GCCause::Cause cause) {
@@ -247,7 +246,6 @@ void ShenandoahDegenGC::op_degenerated() {
           ShenandoahHeapRegion* r;
           while ((r = heap->collection_set()->next()) != nullptr) {
             if (r->is_pinned()) {
-              heap->cancel_gc(GCCause::_shenandoah_upgrade_to_full_gc);
               op_degenerated_fail();
               return;
             }
@@ -312,30 +310,14 @@ void ShenandoahDegenGC::op_degenerated() {
 
   metrics.snap_after();
 
-  // The most common scenario for lack of good progress following a degenerated GC is an accumulation of floating
-  // garbage during the most recently aborted concurrent GC effort.  With generational GC, it is far more effective to
-  // reclaim this floating garbage with another degenerated cycle (which focuses on young generation and might require
-  // a pause of 200 ms) rather than a full GC cycle (which may require over 2 seconds with a 10 GB old generation).
-  //
-  // In generational mode, we'll only upgrade to full GC if we've done two degen cycles in a row and both indicated
-  // bad progress.  In non-generational mode, we'll preserve the original behavior, which is to upgrade to full
-  // immediately following a degenerated cycle with bad progress.  This preserves original behavior of non-generational
-  // Shenandoah so as to avoid introducing "surprising new behavior."  It also makes less sense with non-generational
-  // Shenandoah to replace a full GC with a degenerated GC, because both have similar pause times in non-generational
-  // mode.
-  if (!metrics.is_good_progress(_generation)) {
-    _consecutive_degen_with_bad_progress++;
-  } else {
-    _consecutive_degen_with_bad_progress = 0;
-  }
-  if (!heap->mode()->is_generational() ||
-      ((heap->shenandoah_policy()->consecutive_degenerated_gc_count() > 1) && (_consecutive_degen_with_bad_progress >= 2))) {
-    heap->cancel_gc(GCCause::_shenandoah_upgrade_to_full_gc);
-    op_degenerated_futile();
-  } else {
+  // Decide if this cycle made good progress, and, if not, should it upgrade to a full GC.
+  const bool progress = metrics.is_good_progress(_generation);
+  ShenandoahCollectorPolicy* policy = heap->shenandoah_policy();
+  policy->record_degenerated(_generation->is_young(), _abbreviated, progress);
+  if (progress) {
     heap->notify_gc_progress();
-    heap->shenandoah_policy()->record_success_degenerated(_generation->is_young(), _abbreviated);
-    _generation->heuristics()->record_success_degenerated();
+  } else if (!heap->mode()->is_generational() || policy->generational_should_upgrade_degenerated_gc()) {
+    op_degenerated_futile();
   }
 }
 
@@ -483,6 +465,7 @@ const char* ShenandoahDegenGC::degen_event_message(ShenandoahDegenPoint point) c
 void ShenandoahDegenGC::upgrade_to_full() {
   log_info(gc)("Degenerated GC upgrading to Full GC");
   ShenandoahHeap* heap = ShenandoahHeap::heap();
+  heap->cancel_gc(GCCause::_shenandoah_upgrade_to_full_gc);
   heap->increment_total_collections(true);
   heap->shenandoah_policy()->record_degenerated_upgrade_to_full();
   ShenandoahFullGC full_gc;

src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.hpp

@@ -36,7 +36,6 @@ class ShenandoahDegenGC : public ShenandoahGC {
   const ShenandoahDegenPoint  _degen_point;
   ShenandoahGeneration* _generation;
   bool _abbreviated;
-  size_t _consecutive_degen_with_bad_progress;
 
 public:
   ShenandoahDegenGC(ShenandoahDegenPoint degen_point, ShenandoahGeneration* generation);

test/hotspot/gtest/gc/shenandoah/test_shenandoahCollectorPolicy.cpp

@@ -0,0 +1,72 @@
+/*
+ * Copyright Amazon.com Inc. 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+#include "gc/shenandoah/shenandoahCollectorPolicy.hpp"
+#include "unittest.hpp"
+
+TEST(ShenandoahCollectorPolicyTest, track_degen_cycles_sanity) {
+  ShenandoahCollectorPolicy policy;
+  EXPECT_EQ(policy.consecutive_degenerated_gc_count(), 0UL);
+  EXPECT_EQ(policy.generational_should_upgrade_degenerated_gc(), false);
+}
+
+TEST(ShenandoahCollectorPolicyTest, track_degen_cycles_no_upgrade) {
+  ShenandoahCollectorPolicy policy;
+  policy.record_degenerated(true, true, true);
+  policy.record_degenerated(true, true, true);
+  EXPECT_EQ(policy.consecutive_degenerated_gc_count(), 2UL);
+  EXPECT_EQ(policy.generational_should_upgrade_degenerated_gc(), false);
+}
+
+TEST(ShenandoahCollectorPolicyTest, track_degen_cycles_upgrade) {
+  ShenandoahCollectorPolicy policy;
+  policy.record_degenerated(true, true, false);
+  policy.record_degenerated(true, true, false);
+  EXPECT_EQ(policy.consecutive_degenerated_gc_count(), 2UL);
+  EXPECT_EQ(policy.generational_should_upgrade_degenerated_gc(), true);
+}
+
+TEST(ShenandoahCollectorPolicyTest, track_degen_cycles_reset_progress) {
+  ShenandoahCollectorPolicy policy;
+  policy.record_degenerated(true, true, false);
+  policy.record_degenerated(true, true, true);
+  EXPECT_EQ(policy.consecutive_degenerated_gc_count(), 2UL);
+  EXPECT_EQ(policy.generational_should_upgrade_degenerated_gc(), false);
+}
+
+TEST(ShenandoahCollectorPolicyTest, track_degen_cycles_full_reset) {
+  ShenandoahCollectorPolicy policy;
+  policy.record_degenerated(true, true, false);
+  policy.record_success_full();
+  EXPECT_EQ(policy.consecutive_degenerated_gc_count(), 0UL);
+  EXPECT_EQ(policy.generational_should_upgrade_degenerated_gc(), false);
+}
+
+TEST(ShenandoahCollectorPolicyTest, track_degen_cycles_reset) {
+  ShenandoahCollectorPolicy policy;
+  policy.record_degenerated(true, true, false);
+  policy.record_success_concurrent(true, true);
+  EXPECT_EQ(policy.consecutive_degenerated_gc_count(), 0UL);
+  EXPECT_EQ(policy.generational_should_upgrade_degenerated_gc(), false);
+}