8310680: GenShen: In-place region promotions may fail

Reviewed-by: kdnilsen, ysr

Author: William Kemper

src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.cpp

@@ -41,6 +41,20 @@
 #include "runtime/globals_extension.hpp"
 #include "utilities/quickSort.hpp"
 
+inline void assert_no_in_place_promotions() {
+#ifdef ASSERT
+  class ShenandoahNoInPlacePromotions : public ShenandoahHeapRegionClosure {
+  public:
+    void heap_region_do(ShenandoahHeapRegion *r) override {
+      assert(r->get_top_before_promote() == nullptr,
+             "Region " SIZE_FORMAT " should not be ready for in-place promotion", r->index());
+    }
+  } cl;
+  ShenandoahHeap::heap()->heap_region_iterate(&cl);
+#endif
+}
+
+
 // sort by decreasing garbage (so most garbage comes first)
 int ShenandoahHeuristics::compare_by_garbage(RegionData a, RegionData b) {
   if (a._u._garbage > b._u._garbage)
@@ -125,6 +139,10 @@ static int compare_by_aged_live(AgedRegionData a, AgedRegionData b) {
 // Returns bytes of old-gen memory consumed by selected aged regions
 size_t ShenandoahHeuristics::select_aged_regions(size_t old_available, size_t num_regions,
                                                  bool candidate_regions_for_promotion_by_copy[]) {
+
+  // There should be no regions configured for subsequent in-place-promotions carried over from the previous cycle.
+  assert_no_in_place_promotions();
+
   ShenandoahHeap* heap = ShenandoahHeap::heap();
   assert(heap->mode()->is_generational(), "Only in generational mode");
   ShenandoahMarkingContext* const ctx = heap->marking_context();
@@ -153,22 +171,27 @@ size_t ShenandoahHeuristics::select_aged_regions(size_t old_available, size_t nu
       continue;
     }
     if (r->age() >= InitialTenuringThreshold) {
-      r->save_top_before_promote();
       if ((r->garbage() < old_garbage_threshold)) {
         HeapWord* tams = ctx->top_at_mark_start(r);
         HeapWord* original_top = r->top();
         if (tams == original_top) {
-          // Fill the remnant memory within this region to assure no allocations prior to promote in place.  Otherwise,
-          // newly allocated objects will not be parseable when promote in place tries to register them.  Furthermore, any
-          // new allocations would not necessarily be eligible for promotion.  This addresses both issues.
+          // No allocations from this region have been made during concurrent mark. It meets all the criteria
+          // for in-place-promotion. Though we only need the value of top when we fill the end of the region,
+          // we use this field to indicate that this region should be promoted in place during the evacuation
+          // phase.
+          r->save_top_before_promote();
+
           size_t remnant_size = r->free() / HeapWordSize;
           if (remnant_size > ShenandoahHeap::min_fill_size()) {
             ShenandoahHeap::fill_with_object(original_top, remnant_size);
+            // Fill the remnant memory within this region to assure no allocations prior to promote in place.  Otherwise,
+            // newly allocated objects will not be parseable when promote in place tries to register them.  Furthermore, any
+            // new allocations would not necessarily be eligible for promotion.  This addresses both issues.
             r->set_top(r->end());
             promote_in_place_pad += remnant_size * HeapWordSize;
           } else {
             // Since the remnant is so small that it cannot be filled, we don't have to worry about any accidental
-            // allocations occuring within this region before the region is promoted in place.
+            // allocations occurring within this region before the region is promoted in place.
           }
           promote_in_place_regions++;
           promote_in_place_live += r->get_live_data_bytes();
@@ -319,7 +342,7 @@ void ShenandoahHeuristics::choose_collection_set(ShenandoahCollectionSet* collec
           // ShenandoahOldGarbageThreshold so it will be promoted in place, or because there is not sufficient room
           // in old gen to hold the evacuated copies of this region's live data.  In both cases, we choose not to
           // place this region into the collection set.
-          if (region->garbage_before_padded_for_promote() < old_garbage_threshold) {
+          if (region->get_top_before_promote() != nullptr) {
             regular_regions_promoted_in_place++;
             regular_regions_promoted_usage += region->used_before_promote();
           }

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

@@ -1271,9 +1271,11 @@ void ShenandoahConcurrentGC::op_final_updaterefs() {
 
 void ShenandoahConcurrentGC::op_final_roots() {
 
-  ShenandoahHeap* heap = ShenandoahHeap::heap();
-  if (heap->is_aging_cycle()) {
-    ShenandoahMarkingContext* ctx = heap->complete_marking_context();
+  ShenandoahHeap *heap = ShenandoahHeap::heap();
+  heap->set_concurrent_weak_root_in_progress(false);
+
+  if (heap->mode()->is_generational()) {
+    ShenandoahMarkingContext *ctx = heap->complete_marking_context();
 
     for (size_t i = 0; i < heap->num_regions(); i++) {
       ShenandoahHeapRegion *r = heap->get_region(i);
@@ -1282,14 +1284,12 @@ void ShenandoahConcurrentGC::op_final_roots() {
         HeapWord* top = r->top();
         if (top > tams) {
           r->reset_age();
-        } else {
+        } else if (heap->is_aging_cycle()) {
           r->increment_age();
         }
       }
     }
   }
-
-  ShenandoahHeap::heap()->set_concurrent_weak_root_in_progress(false);
 }
 
 void ShenandoahConcurrentGC::op_cleanup_complete() {

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

@@ -1718,7 +1718,11 @@ class ShenandoahGenerationalEvacuationTask : public WorkerTask {
           // doing this work during a safepoint.  We cannot put humongous regions into the collection set because that
           // triggers the load-reference barrier (LRB) to copy on reference fetch.
           r->promote_humongous();
-        } else if (r->is_regular() && (r->garbage_before_padded_for_promote() < old_garbage_threshold) && (r->get_top_before_promote() == tams)) {
+        } else if (r->is_regular() && (r->get_top_before_promote() != nullptr)) {
+          assert(r->garbage_before_padded_for_promote() < old_garbage_threshold,
+                 "Region " SIZE_FORMAT " has too much garbage for promotion", r->index());
+          assert(r->get_top_before_promote() == tams,
+                 "Region " SIZE_FORMAT " has been used for allocations before promotion", r->index());
           // Likewise, we cannot put promote-in-place regions into the collection set because that would also trigger
           // the LRB to copy on reference fetch.
           r->promote_in_place();
@@ -2996,7 +3000,7 @@ class ShenandoahFinalUpdateRefsUpdateRegionStateClosure : public ShenandoahHeapR
     // Maintenance of region age must follow evacuation in order to account for evacuation allocations within survivor
     // regions.  We consult region age during the subsequent evacuation to determine whether certain objects need to
     // be promoted.
-    if (_is_generational && r->is_young()) {
+    if (_is_generational && r->is_young() && r->is_active()) {
       HeapWord *tams = _ctx->top_at_mark_start(r);
       HeapWord *top = r->top();
 

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

@@ -71,6 +71,7 @@ ShenandoahHeapRegion::ShenandoahHeapRegion(HeapWord* start, size_t index, bool c
   _end(start + RegionSizeWords),
   _new_top(nullptr),
   _empty_time(os::elapsedTime()),
+  _top_before_promoted(nullptr),
   _state(committed ? _empty_committed : _empty_uncommitted),
   _top(start),
   _tlab_allocs(0),

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

@@ -185,8 +185,8 @@ inline size_t ShenandoahHeapRegion::garbage() const {
 }
 
 inline size_t ShenandoahHeapRegion::garbage_before_padded_for_promote() const {
-  size_t used_before_promote = byte_size(bottom(), get_top_before_promote());
   assert(get_top_before_promote() != nullptr, "top before promote should not equal null");
+  size_t used_before_promote = byte_size(bottom(), get_top_before_promote());
   assert(used_before_promote >= get_live_data_bytes(),
          "Live Data must be a subset of used before promotion live: " SIZE_FORMAT " used: " SIZE_FORMAT,
          get_live_data_bytes(), used_before_promote);
@@ -251,11 +251,8 @@ inline void ShenandoahHeapRegion::save_top_before_promote() {
 
 inline void ShenandoahHeapRegion::restore_top_before_promote() {
   _top = _top_before_promoted;
-#ifdef ASSERT
   _top_before_promoted = nullptr;
-#endif
  }
 
 
-
 #endif // SHARE_GC_SHENANDOAH_SHENANDOAHHEAPREGION_INLINE_HPP