RFR: 8328307: GenShen: Re-enable old-has-grown trigger for old-generation GC [v6]

[Kelvin Nilsen]

On Sat, 13 Apr 2024 01:14:23 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Move trigger code to ShenandoahOldHeuristics from ShenandoahOldGeneration
>
> src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 588:
> 
>> 586: 
>> 587: void ShenandoahOldHeuristics::trigger_collection_if_overgrown(ShenandoahGenerationalHeap* gen_heap, ShenandoahOldGeneration* old_gen) {
>> 588:   size_t old_used = old_gen->used() + old_gen->get_humongous_waste();
> 
> There's a `used_including_humongous_waste()` method in `ShenandoahGeneration` but unfortunately not yet in `ShenandoahSpaceInfo`.

I'm inclined to keep code as is for this integration.  We can think about refactoring and using ShenandoahSpaceInfo services in a future refinement.

> src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 593:
> 
>> 591:   assert(old_used <= gen_heap->capacity(),
>> 592:          "Old used (" SIZE_FORMAT ", " SIZE_FORMAT") must not be more than heap capacity (" SIZE_FORMAT ")",
>> 593:          old_gen->used(), old_gen->get_humongous_waste(), gen_heap->capacity());
> 
> Does this assert belong here, rather than in the `used()` method?

I think I'll keep it here for two reasons:
1. We know the context here, and we know that this invariant should be true here.  Sometimes, during certain transitions that occur during Full GC for example, the invariant might be briefly violated, which could cause spurious false flags if I put the assert into used() method.
2. I'm actually summing two values to computed the value of old_used that feeds into this assert.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1566229498
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1566228371