RFR: 8338737: Shenandoah: Reset marking bitmaps after the cycle [v3]

[Kelvin Nilsen]

On Mon, 30 Dec 2024 22:54:27 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.
>> 
>> ...
>
> Xiaolong Peng has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 17 additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into reset-bitmap
>  - Address review comments
>  - Merge branch 'openjdk:master' into reset-bitmap
>  - Remove ShenandoahResetUpdateRegionStateClosure
>  - Always set_mark_incomplete when reset mark bitmap
>  - Fix
>  - Add comments
>  - fix
>  - Not reset_mark_bitmap after cycle when  is_concurrent_old_mark_in_progress or is_prepare_for_old_mark_in_progress
>  - Not invoke set_mark_incomplete when reset bitmap after cycle
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/82c2f771...f82fdfaa

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

> 240:   // Instead of always reset before collect, some reset can be done after collect to save
> 241:   // the time before before the cycle so the cycle can be started as soon as possible.
> 242:   entry_reset_after_collect();

For comment, I would say: "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."

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

> 590:   // If it is old GC bootstrap cycle, always clear bitmap for global gen
> 591:   // to ensure bitmap for old gen is clear for old GC cycle after this.
> 592:   if (_do_old_gc_bootstrap) {

This may deserve a comment.  It seems we ought to clear the old-gen mark bitmap at the end of coalesce-and-fill.  But that does not allow us to avoid clearing old-gen mark bitmaps at start of bootstrap because when young-gen regions are promoted in place, the mark bitmap is preserved for those regions, and since they are considered old at the end of the GC cycle during which they were promoted, those bitmaps will not be cleared by op_reset_after_collect().  Is there a way to improve this behavior?

For example, in op_reset_after_collect(), maybe we should clear old-gen bitmaps also (at least for recently promoted in place regions) unless old marking is in process and/or mixed evacuations are in progress.

Maybe this can be tackled in a separate PR, but would be good to file JBS ticket now if there is agreement on the approach.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22778#discussion_r1907828996
PR Review Comment: https://git.openjdk.org/jdk/pull/22778#discussion_r1907845214