RFR: 8338737: Shenandoah: Reset marking bitmaps after the cycle

[William Kemper]

On Tue, 17 Dec 2024 00:09:25 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

> Reset marking bitmaps after collection cycle; for GenShen only do this for young generation, also choose not do this for Degen and full GC since both are running at safepoint, we should leave safepoint as ASAP.
> 
> I have run same workload for 30s with Shenandoah in generational mode and classic mode, average average time of concurrent reset dropped significantly since in most case bitmap for young gen should have been reset after pervious concurrent cycle finishes if there is no need to preserve bitmap states.
> 
> GenShen:
> Before:
> 
> [33.342s][info][gc,stats    ] Concurrent Reset               =    0.023 s (a =     1921 us) (n =    12) (lvls, us =      133,      385,     1191,     1836,     8878)
> 
> 
> After:
> 
> [33.597s][info][gc,stats    ] Concurrent Reset               =    0.004 s (a =      317 us) (n =    13) (lvls, us =       58,      119,      217,      410,      670)
> [33.597s][info][gc,stats    ] Concurrent Reset After Collect =    0.018 s (a =     1365 us) (n =    13) (lvls, us =       91,      186,      818,     1836,     3872)
> 
> 
> Shenandoah:
> Before:
> 
> [33.144s][info][gc,stats    ] Concurrent Reset               =    0.014 s (a =     1067 us) (n =    13) (lvls, us =      139,      277,      898,     1328,     2118)
> 
> After:
> 
> [33.128s][info][gc,stats    ] Concurrent Reset               =    0.003 s (a =      225 us) (n =    13) (lvls, us =       32,       92,      137,      295,      542)
> [33.128s][info][gc,stats    ] Concurrent Reset After Collect =    0.009 s (a =      661 us) (n =    13) (lvls, us =       92,      160,      594,      896,     1661)
> 
> 
> Additional changes:
> * Remove `ShenandoahResetBitmapClosure` and  `ShenandoahPrepareForMarkClosure`, merge the code with `ShenandoahResetBitmapClosure`, saving one iteration over all the regions. 
> * Use API `ShenandoahGeneration::parallel_heap_region_iterate_free` to iterate the regions, two benefits from this:
>   -  Underneath it calls `ShenandoahHeap::parallel_heap_region_iterate`, which is faster for very light tasks, see https://bugs.openjdk.org/browse/JDK-8337154
>   -  `ShenandoahGeneration::parallel_heap_region_iterate_free` decorate the closure with `ShenandoahExcludeRegionClosure`, which simplifies the code in closure.
> * When `_do_old_gc_bootstrap is true`, instead of reset mark bitmap for old gen separately, simply reset the global generations, so we don't need walk the all regions twice. 
> * Clean up FullGC code, remove duplicate code.
> 
> Additional tests:
> - [x] CONF=macosx-aarch64-server-fastdebug make test T...

Looks good. Left a few nits and a few questions in the review.

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 1211:

> 1209:       // Only reset for young generation, bitmap for old generation must be retained,
> 1210:       // except there is collection(global/old/degen/full) trigged to collect regions in old gen.
> 1211:       heap->young_generation()->reset_mark_bitmap<false>();

Shouldn't it be safe to reset young region bitmaps even when old marking is in progress?

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 58:

> 56:     if (PREPARE_FOR_CURRENT_CYCLE) {
> 57:       if (region->need_bitmap_reset() && _heap->is_bitmap_slice_committed(region)) {
> 58:         _ctx->clear_bitmap(region);

Should this also `region->unset_need_bitmap_reset()`?

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 66:

> 64:         // Reset live data and set TAMS optimistically. We would recheck these under the pause
> 65:         // anyway to capture any updates that happened since now.
> 66:         _ctx->capture_top_at_mark_start(region);

Full GC used to do this unconditionally for all affiliated regions. Do we not still need that to happen?

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp line 92:

> 90:   }
> 91:   _recycling.unset();
> 92:   _need_bitmap_reset = true;

Move to initializers? Why does it start with `true`? A new region would have a clear bitmap, right?

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.hpp line 269:

> 267:   ShenandoahSharedFlag _recycling; // Used to indicate that the region is being recycled; see try_recycle*().
> 268: 
> 269:   bool _need_bitmap_reset;

Nit pick, but I think this would read better as `_needs_bitmap_reset`.

src/hotspot/share/gc/shenandoah/shenandoahOldGC.cpp line 161:

> 159:   }
> 160: 
> 161:   entry_reset_after_collect();

Not sure we want to reset old region bitmaps after old marking is compete. Shenandoah opportunistically uses the bitmap for old regions during remembered set scan (it's faster than walking the heap).

-------------

Changes requested by wkemper (Committer).

PR Review: https://git.openjdk.org/jdk/pull/22778#pullrequestreview-2518128727
PR Review Comment: https://git.openjdk.org/jdk/pull/22778#discussion_r1894270072
PR Review Comment: https://git.openjdk.org/jdk/pull/22778#discussion_r1894273795
PR Review Comment: https://git.openjdk.org/jdk/pull/22778#discussion_r1894277047
PR Review Comment: https://git.openjdk.org/jdk/pull/22778#discussion_r1894278663
PR Review Comment: https://git.openjdk.org/jdk/pull/22778#discussion_r1894283547
PR Review Comment: https://git.openjdk.org/jdk/pull/22778#discussion_r1894282722