8338737: Shenandoah: Reset marking bitmaps after the cycle

Reviewed-by: wkemper

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

@@ -241,6 +241,12 @@ bool ShenandoahConcurrentGC::collect(GCCause::Cause cause) {
   if (heap->mode()->is_generational()) {
     ShenandoahGenerationalHeap::heap()->complete_concurrent_cycle();
   }
+
+  // Instead of always resetting immediately before the start of a new GC, we can often reset at the end of the
+  // previous GC. This allows us to start the next GC cycle more quickly after a trigger condition is detected,
+  // reducing the likelihood that GC will degenerate.
+  entry_reset_after_collect();
+
   return true;
 }
 
@@ -363,17 +369,6 @@ void ShenandoahConcurrentGC::entry_reset() {
                                 msg);
     op_reset();
   }
-
-  if (_do_old_gc_bootstrap) {
-    static const char* msg = "Concurrent reset (Old)";
-    ShenandoahConcurrentPhase gc_phase(msg, ShenandoahPhaseTimings::conc_reset_old);
-    ShenandoahWorkerScope scope(ShenandoahHeap::heap()->workers(),
-                                ShenandoahWorkerPolicy::calc_workers_for_conc_reset(),
-                                msg);
-    EventMark em("%s", msg);
-
-    heap->old_generation()->prepare_gc();
-  }
 }
 
 void ShenandoahConcurrentGC::entry_scan_remembered_set() {
@@ -583,12 +578,29 @@ void ShenandoahConcurrentGC::entry_cleanup_complete() {
   op_cleanup_complete();
 }
 
+void ShenandoahConcurrentGC::entry_reset_after_collect() {
+  ShenandoahHeap* const heap = ShenandoahHeap::heap();
+  TraceCollectorStats tcs(heap->monitoring_support()->concurrent_collection_counters());
+  const char* msg = conc_reset_after_collect_event_message();
+  ShenandoahConcurrentPhase gc_phase(msg, ShenandoahPhaseTimings::conc_reset_after_collect);
+  EventMark em("%s", msg);
+
+  op_reset_after_collect();
+}
+
 void ShenandoahConcurrentGC::op_reset() {
   ShenandoahHeap* const heap = ShenandoahHeap::heap();
   if (ShenandoahPacing) {
     heap->pacer()->setup_for_reset();
   }
-  _generation->prepare_gc();
+  // If it is old GC bootstrap cycle, always clear bitmap for global gen
+  // to ensure bitmap for old gen is clear for old GC cycle after this.
+  if (_do_old_gc_bootstrap) {
+    assert(!heap->is_prepare_for_old_mark_in_progress(), "Cannot reset old without making it parsable");
+    heap->global_generation()->prepare_gc();
+  } else {
+    _generation->prepare_gc();
+  }
 }
 
 class ShenandoahInitMarkUpdateRegionStateClosure : public ShenandoahHeapRegionClosure {
@@ -1211,6 +1223,26 @@ void ShenandoahConcurrentGC::op_cleanup_complete() {
   ShenandoahHeap::heap()->recycle_trash();
 }
 
+void ShenandoahConcurrentGC::op_reset_after_collect() {
+  ShenandoahWorkerScope scope(ShenandoahHeap::heap()->workers(),
+                          ShenandoahWorkerPolicy::calc_workers_for_conc_reset(),
+                          "reset after collection.");
+
+  ShenandoahHeap* const heap = ShenandoahHeap::heap();
+  if (heap->mode()->is_generational()) {
+    // Resetting bitmaps of young gen when bootstrap old GC or there is preempted old GC
+    // causes crash due to remembered set violation, hence condition is added to fix the crash.
+    // Assuming bitmaps of young gen are not used at all after the cycle, the crash should not
+    // have happend, it seems to tickle a bug in remembered set scan. Root causing and fixing of the bug
+    // will be tracked via ticket https://bugs.openjdk.org/browse/JDK-8347371
+    if (!_do_old_gc_bootstrap && !heap->is_concurrent_old_mark_in_progress()) {
+      heap->young_generation()->reset_mark_bitmap<false>();
+    }
+  } else {
+    _generation->reset_mark_bitmap<false>();
+  }
+}
+
 bool ShenandoahConcurrentGC::check_cancellation_and_abort(ShenandoahDegenPoint point) {
   if (ShenandoahHeap::heap()->cancelled_gc()) {
     _degen_point = point;
@@ -1260,6 +1292,14 @@ const char* ShenandoahConcurrentGC::conc_reset_event_message() const {
   }
 }
 
+const char* ShenandoahConcurrentGC::conc_reset_after_collect_event_message() const {
+  if (ShenandoahHeap::heap()->unload_classes()) {
+    SHENANDOAH_RETURN_EVENT_MESSAGE(_generation->type(), "Concurrent reset after collect", " (unload classes)");
+  } else {
+    SHENANDOAH_RETURN_EVENT_MESSAGE(_generation->type(), "Concurrent reset after collect", "");
+  }
+}
+
 const char* ShenandoahConcurrentGC::final_roots_event_message() const {
   if (ShenandoahHeap::heap()->unload_classes()) {
     SHENANDOAH_RETURN_EVENT_MESSAGE(_generation->type(), "Pause Final Roots", " (unload classes)");

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

@@ -118,10 +118,14 @@ protected:
   void op_final_update_refs();
   void op_final_roots();
   void op_cleanup_complete();
+  void op_reset_after_collect();
 
   // Check GC cancellation and abort concurrent GC
   bool check_cancellation_and_abort(ShenandoahDegenPoint point);
 
+  // Called when concurrent GC succeeds.
+  void entry_reset_after_collect();
+
 private:
   void start_mark();
 
@@ -134,6 +138,7 @@ private:
   const char* final_roots_event_message() const;
   const char* conc_mark_event_message() const;
   const char* conc_reset_event_message() const;
+  const char* conc_reset_after_collect_event_message() const;
   const char* conc_weak_refs_event_message() const;
   const char* conc_weak_roots_event_message() const;
   const char* conc_cleanup_event_message() const;

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

@@ -197,16 +197,11 @@ void ShenandoahFullGC::do_it(GCCause::Cause gc_cause) {
       update_roots(true /*full_gc*/);
     }
 
-    // d. Reset the bitmaps for new marking
-    heap->global_generation()->reset_mark_bitmap();
-    assert(heap->marking_context()->is_bitmap_clear(), "sanity");
-    assert(!heap->global_generation()->is_mark_complete(), "sanity");
-
-    // e. Abandon reference discovery and clear all discovered references.
+    // d. Abandon reference discovery and clear all discovered references.
     ShenandoahReferenceProcessor* rp = heap->global_generation()->ref_processor();
     rp->abandon_partial_discovery();
 
-    // f. Sync pinned region status from the CP marks
+    // e. Sync pinned region status from the CP marks
     heap->sync_pinned_region_status();
 
     if (heap->mode()->is_generational()) {
@@ -287,30 +282,15 @@ void ShenandoahFullGC::do_it(GCCause::Cause gc_cause) {
   }
 }
 
-class ShenandoahPrepareForMarkClosure: public ShenandoahHeapRegionClosure {
-private:
-  ShenandoahMarkingContext* const _ctx;
-
-public:
-  ShenandoahPrepareForMarkClosure() : _ctx(ShenandoahHeap::heap()->marking_context()) {}
-
-  void heap_region_do(ShenandoahHeapRegion *r) override {
-    _ctx->capture_top_at_mark_start(r);
-    r->clear_live_data();
-  }
-
-  bool is_thread_safe() override { return true; }
-};
-
 void ShenandoahFullGC::phase1_mark_heap() {
   GCTraceTime(Info, gc, phases) time("Phase 1: Mark live objects", _gc_timer);
   ShenandoahGCPhase mark_phase(ShenandoahPhaseTimings::full_gc_mark);
 
   ShenandoahHeap* heap = ShenandoahHeap::heap();
 
-  ShenandoahPrepareForMarkClosure prepare_for_mark;
-  ShenandoahExcludeRegionClosure<FREE> cl(&prepare_for_mark);
-  heap->parallel_heap_region_iterate(&cl);
+  heap->global_generation()->reset_mark_bitmap<true, true>();
+  assert(heap->marking_context()->is_bitmap_clear(), "sanity");
+  assert(!heap->global_generation()->is_mark_complete(), "sanity");
 
   heap->set_unload_classes(heap->global_generation()->heuristics()->can_unload_classes());
 

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

@@ -41,53 +41,44 @@
 
 #include "utilities/quickSort.hpp"
 
-
-class ShenandoahResetUpdateRegionStateClosure : public ShenandoahHeapRegionClosure {
+template <bool PREPARE_FOR_CURRENT_CYCLE, bool FULL_GC = false>
+class ShenandoahResetBitmapClosure final : public ShenandoahHeapRegionClosure {
 private:
-  ShenandoahHeap* _heap;
-  ShenandoahMarkingContext* const _ctx;
-public:
-  ShenandoahResetUpdateRegionStateClosure() :
-    _heap(ShenandoahHeap::heap()),
-    _ctx(_heap->marking_context()) {}
+  ShenandoahHeap*           _heap;
+  ShenandoahMarkingContext* _ctx;
 
-  void heap_region_do(ShenandoahHeapRegion* r) override {
-    if (r->is_active()) {
-      // Reset live data and set TAMS optimistically. We would recheck these under the pause
-      // anyway to capture any updates that happened since now.
-      _ctx->capture_top_at_mark_start(r);
-      r->clear_live_data();
+public:
+  explicit ShenandoahResetBitmapClosure() :
+    ShenandoahHeapRegionClosure(), _heap(ShenandoahHeap::heap()), _ctx(_heap->marking_context()) {}
+
+  void heap_region_do(ShenandoahHeapRegion* region) override {
+    assert(!_heap->is_uncommit_in_progress(), "Cannot uncommit bitmaps while resetting them.");
+    if (PREPARE_FOR_CURRENT_CYCLE) {
+      if (region->need_bitmap_reset() && _heap->is_bitmap_slice_committed(region)) {
+        _ctx->clear_bitmap(region);
+      } else {
+        region->set_needs_bitmap_reset();
+      }
+      // Capture Top At Mark Start for this generation.
+      if (FULL_GC || region->is_active()) {
+        // Reset live data and set TAMS optimistically. We would recheck these under the pause
+        // anyway to capture any updates that happened since now.
+        _ctx->capture_top_at_mark_start(region);
+        region->clear_live_data();
+      }
+    } else {
+      if (_heap->is_bitmap_slice_committed(region)) {
+        _ctx->clear_bitmap(region);
+        region->unset_needs_bitmap_reset();
+      } else {
+        region->set_needs_bitmap_reset();
+      }
     }
   }
 
   bool is_thread_safe() override { return true; }
 };
 
-class ShenandoahResetBitmapTask : public WorkerTask {
-private:
-  ShenandoahRegionIterator _regions;
-  ShenandoahGeneration* _generation;
-
-public:
-  ShenandoahResetBitmapTask(ShenandoahGeneration* generation) :
-    WorkerTask("Shenandoah Reset Bitmap"), _generation(generation) {}
-
-  void work(uint worker_id) {
-    ShenandoahHeap* heap = ShenandoahHeap::heap();
-    assert(!heap->is_uncommit_in_progress(), "Cannot uncommit bitmaps while resetting them.");
-    ShenandoahHeapRegion* region = _regions.next();
-    ShenandoahMarkingContext* const ctx = heap->marking_context();
-    while (region != nullptr) {
-      auto const affiliation = region->affiliation();
-      bool needs_reset = affiliation == FREE || _generation->contains(affiliation);
-      if (needs_reset && heap->is_bitmap_slice_committed(region)) {
-        ctx->clear_bitmap(region);
-      }
-      region = _regions.next();
-    }
-  }
-};
-
 // Copy the write-version of the card-table into the read-version, clearing the
 // write-copy.
 class ShenandoahMergeWriteTable: public ShenandoahHeapRegionClosure {
@@ -225,15 +216,20 @@ void ShenandoahGeneration::log_status(const char *msg) const {
               PROPERFMTARGS(v_soft_max_capacity), PROPERFMTARGS(v_max_capacity), PROPERFMTARGS(v_available));
 }
 
+template <bool PREPARE_FOR_CURRENT_CYCLE, bool FULL_GC>
 void ShenandoahGeneration::reset_mark_bitmap() {
   ShenandoahHeap* heap = ShenandoahHeap::heap();
   heap->assert_gc_workers(heap->workers()->active_workers());
 
   set_mark_incomplete();
 
-  ShenandoahResetBitmapTask task(this);
-  heap->workers()->run_task(&task);
+  ShenandoahResetBitmapClosure<PREPARE_FOR_CURRENT_CYCLE, FULL_GC> closure;
+  parallel_heap_region_iterate_free(&closure);
 }
+// Explicit specializations
+template void ShenandoahGeneration::reset_mark_bitmap<true, false>();
+template void ShenandoahGeneration::reset_mark_bitmap<true, true>();
+template void ShenandoahGeneration::reset_mark_bitmap<false, false>();
 
 // The ideal is to swap the remembered set so the safepoint effort is no more than a few pointer manipulations.
 // However, limitations in the implementation of the mutator write-barrier make it difficult to simply change the
@@ -265,12 +261,7 @@ void ShenandoahGeneration::merge_write_table() {
 }
 
 void ShenandoahGeneration::prepare_gc() {
-
-  reset_mark_bitmap();
-
-  // Capture Top At Mark Start for this generation (typically young) and reset mark bitmap.
-  ShenandoahResetUpdateRegionStateClosure cl;
-  parallel_heap_region_iterate_free(&cl);
+  reset_mark_bitmap<true>();
 }
 
 void ShenandoahGeneration::parallel_heap_region_iterate_free(ShenandoahHeapRegionClosure* cl) {

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

@@ -159,6 +159,7 @@ private:
   void log_status(const char* msg) const;
 
   // Used directly by FullGC
+  template <bool FOR_CURRENT_CYCLE, bool FULL_GC = false>
   void reset_mark_bitmap();
 
   // Used by concurrent and degenerated GC to reset remembered set.

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

@@ -76,10 +76,11 @@ ShenandoahHeapRegion::ShenandoahHeapRegion(HeapWord* start, size_t index, bool c
   _live_data(0),
   _critical_pins(0),
   _update_watermark(start),
-  _age(0)
+  _age(0),
 #ifdef SHENANDOAH_CENSUS_NOISE
-  , _youth(0)
+  _youth(0),
 #endif // SHENANDOAH_CENSUS_NOISE
+  _needs_bitmap_reset(false)
   {
 
   assert(Universe::on_page_boundary(_bottom) && Universe::on_page_boundary(_end),

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

@@ -266,6 +266,8 @@ private:
 
   ShenandoahSharedFlag _recycling; // Used to indicate that the region is being recycled; see try_recycle*().
 
+  bool _needs_bitmap_reset;
+
 public:
   ShenandoahHeapRegion(HeapWord* start, size_t index, bool committed);
 
@@ -477,6 +479,18 @@ public:
 
   CENSUS_NOISE(void clear_youth() { _youth = 0; })
 
+  inline bool need_bitmap_reset() const {
+    return _needs_bitmap_reset;
+  }
+
+  inline void set_needs_bitmap_reset() {
+    _needs_bitmap_reset = true;
+  }
+
+  inline void unset_needs_bitmap_reset() {
+    _needs_bitmap_reset = false;
+  }
+
 private:
   void decrement_humongous_waste() const;
   void do_commit();

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

@@ -50,6 +50,7 @@ class outputStream;
 
 #define SHENANDOAH_PHASE_DO(f)                                                         \
   f(conc_reset,                                     "Concurrent Reset")                \
+  f(conc_reset_after_collect,                       "Concurrent Reset After Collect")  \
   f(conc_reset_old,                                 "Concurrent Reset (OLD)")          \
   f(init_mark_gross,                                "Pause Init Mark (G)")             \
   f(init_mark,                                      "Pause Init Mark (N)")             \