RFR: 8339611: GenShen: Simplify ShenandoahOldHeuristics::trigger_collection_if_fragmented [v3]

[Y. Srinivas Ramakrishna]

On Mon, 9 Sep 2024 20:43:39 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> Replace iterative solution with a single mathematical computation, and adjust old-gen defragmentation efforts to be a bit less aggressive.
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve comments and clarify intent of old_region_span

Left a few clean up comments to make it more readable and more easily understandable.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 552:

> 550: // is required to represent the memory currently used within the old generation.  This trigger looks specifically
> 551: // at density of the old-gen spanned region.  A different mechanism triggers old-gen GC if the total number of
> 552: // old-gen regions (regardless of how close the regions are to one another) grows beyond an anticipated growth target.

I realize this is a private API / work method. Would still be good to place the documentation comment in the .hpp file.


// If the old generation is fragmented, set appropriate collection triggering signals.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 553:

> 551: // at density of the old-gen spanned region.  A different mechanism triggers old-gen GC if the total number of
> 552: // old-gen regions (regardless of how close the regions are to one another) grows beyond an anticipated growth target.
> 553: void ShenandoahOldHeuristics::trigger_collection_if_fragmented(size_t first_old_region, size_t last_old_region,

I'd name this method `set_fragmentation_trigger()`, with the side-effect that the fragmentation measures are recorded for logging, and associated triggers are set which are used later to start an old cycle.

I'd then rename `trigger_maybe()` to `set_triggers()`, and rename `trigger_collection_if_overgrown()` to `set_growth_trigger()`, etc.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 558:

> 556:     // Our intent is to pack old-gen memory into the highest-numbered regions of the heap.  Count all memory
> 557:     // above first_old_region as the "span" of old generation.
> 558:     size_t old_region_span = (first_old_region <= last_old_region)? (num_regions - first_old_region): 0;

Can it ever be the case that `first_old_region > last_old_region` ? When can that be the case?

Then, `old_region_span = 0`....

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 573:

> 571:     size_t old_words_consumed = old_region_count * region_size_words - old_fragmented_available;
> 572:     size_t old_words_spanned = old_region_span * region_size_words;
> 573:     double old_density = ((double) old_words_consumed) / old_words_spanned;

... and if `old_region_span = 0`, then you end up with line 573 causing a divide-by-zero error.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 590:

> 588:       //  old_span_percent is  60% and old_density is below 27.0%, or
> 589:       //  old_span_percent is  50% and old_density is below 18.8%.
> 590:       trigger_old_is_fragmented(old_density, first_old_region, last_old_region);

This method seems a bit useless, why not inline the assignment here? I realize you may be trying to make this method span fewer lines, but I think that can be easily achieved by just moving the description of its work out of line from the code into a block comment at the start that describes how fragmentation is computed and what the trigger threshold criteria are.

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

PR Review: https://git.openjdk.org/shenandoah/pull/492#pullrequestreview-2294311123
PR Review Comment: https://git.openjdk.org/shenandoah/pull/492#discussion_r1752935743
PR Review Comment: https://git.openjdk.org/shenandoah/pull/492#discussion_r1752941163
PR Review Comment: https://git.openjdk.org/shenandoah/pull/492#discussion_r1752945550
PR Review Comment: https://git.openjdk.org/shenandoah/pull/492#discussion_r1752946402
PR Review Comment: https://git.openjdk.org/shenandoah/pull/492#discussion_r1752947994