RFR: 8329350: GenShen: Do not reset mark bitmaps on a safepoint [v2]

[Y. Srinivas Ramakrishna]

On Mon, 1 Apr 2024 17:55:30 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Shenandoah is currently resetting mark bitmaps during the init mark pause. This work should happen during the concurrent reset phase to avoid prolonging the safepoint. Also, free regions need to have the corresponding bitmap region reset or we risk having marked regions with no live data (which violates asserts during final mark).
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix zero build

LGTM. Some very minor comments. No need for re-review, irrespective of any subsequent changes.

Would be good to see any comparative performance numbers if available (init mark or general improvement).

src/hotspot/share/gc/shenandoah/shenandoahHeapRegionClosures.hpp line 31:

> 29: #include "gc/shenandoah/shenandoahHeap.hpp"
> 30: #include "gc/shenandoah/shenandoahHeapRegion.inline.hpp"
> 31: 

Please add a 1-line documentation for each of these classes.

```A closure that is applied to all regions with this affiliation```

and 

```A closure that is applied to all regions outside this affiliation```

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp line 249:

> 247: 
> 248: void ShenandoahOldGeneration::parallel_heap_region_iterate(ShenandoahHeapRegionClosure* cl) {
> 249:   ShenandoahIncludeRegionClosure<OLD_GENERATION> old_regions(cl);

Subjective nit: Not something you introduced, but I'd name the variable `old_region_cl` rather than `old_regions` just to avoid dimensional mismatch (to borrow a term from physics) in the naming. It makes code easier to read if one avoids such naming type/dimension mismatch.

Ignore this suggestion if this will affect upstream Shenandoah as well (unless the renaming can be pushed separately and avoid an issue with the diffs).

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/413#pullrequestreview-1972263399
PR Review Comment: https://git.openjdk.org/shenandoah/pull/413#discussion_r1546935512
PR Review Comment: https://git.openjdk.org/shenandoah/pull/413#discussion_r1546948525