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

[Y. Srinivas Ramakrishna]

On Thu, 28 Mar 2024 15:37:28 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> Enable old-gen growth triggers, which were inadvertantly disabled.  This passes the internal regression pipeline tests.
>> 
>> As would be expected, we see an increase in mixed-evacuation triggers.  We also see significant improvement on certain extremem workloads due to improved clearing of old-gen.
>> 
>> 
>>    Control: shenandoah-x86-template
>> Experiment: enable-old-growth-triggers-gh-x86
>> 
>>                           Most impacted benchmarks |                              Most impacted metrics
>> -------------------------------------------------------------------------------------------------------
>>                            Genshen/extremem-phased |                             trigger_expedite_mixed
>>                 Genshen/specjbb2015_weak_ref_patch |                                    trigger_failure
>>                                Genshen/specjbb2015 |                               context_switch_count
>>                     Genshen/hyperalloc_a3072_o4096 |                                     sla_25000_jops
>>                             Shenandoah/specjbb2015 |                                      trigger_learn
>> 
>> 
>>                                 Only in experiment |                                    Only in control
>> -------------------------------------------------------------------------------------------------------
>>      hyperalloc_a2048_o2048/trigger_expedite_mixed |                   compress/concurrent_thread_roots
>>      hyperalloc_a2048_o4096/trigger_expedite_mixed |                        crypto.rsa/ctr_thread_roots
>>      hyperalloc_a3072_o2048/trigger_expedite_mixed |                               crypto.rsa/ctr_total
>>      hyperalloc_a3072_o4096/trigger_expedite_mixed |               extremem-large-31g/trigger_expansion
>>               extremem-large-31g/trigger_overgrown |                  extremem-phased/trigger_expansion
>> 
>> Genshen
>> -------------------------------------------------------------------------------------------------------
>> +685.00% specjbb2015_weak_ref_patch/trigger_expedite_mixed p=0.00002
>>   Control:      2.500   (+/-  0.68  )         30
>>   Test:        19.625   (+/-  4.79  )         10
>> 
>> +561.90% specjbb2015/trigger_expedite_mixed p=0.00001
>>   Control:      2.625   (+/-  0.92  )         30
>>   Test:        17.375   (+/-  3.89  )         10
>> 
>> +225.42% extremem-phased/trigger_expedite_mixed p=0.00000
>>   Control:      9.833   (+/-  3.48  )         30
>>   Test:        32.000   (+/-  2.58  )       ...
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix another typo

Wondering if the new trigger methods belong in ShenandoahOldHeuristics to keep all triggering code in one place and to integrate with existing triggering code?

Will read through the triggering criteria details shortly.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 3202:

> 3200: 
> 3201:   if (mode()->is_generational()) {
> 3202:     ShenandoahGenerationalHeap* gen_heap = (ShenandoahGenerationalHeap*) this;

May be `ShenandoahGenerationalHeap::heap()` like was done further above? (subjective nit can be ignored; see another suggestion further below that affects this as well.)

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 3202:

> 3200: 
> 3201:   if (mode()->is_generational()) {
> 3202:     ShenandoahGenerationalHeap* gen_heap = (ShenandoahGenerationalHeap*) this;

One other question: would something like this not be under the purview of the appropriate heuristics object associated with the heap? May be ShenandoahOldHeuristics? Just wondering about the correct abstraction boundaries here.

How does the regular (non-generational collector) do this following the rebuild?

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

> 508: }
> 509: 
> 510: void ShenandoahOldGeneration::trigger_collection_if_fragmented(ShenandoahGenerationalHeap* gen_heap, size_t first_old_region,

As pointed out further above, would this more naturally belong here or in ShenandoahOldHeuristics? There are a bunch of triggers & checks there as well. Would appear to make sense to keep all of this in one place rather than scattering them between generation and heuristic for clear abstraction boundaries and maintainability of code.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp line 228:

> 226:   void set_live_bytes_after_last_mark(size_t new_live);
> 227: 
> 228:   void trigger_collection_if_fragmented(ShenandoahGenerationalHeap* gen_heap, size_t first_old_region, size_t last_old_region,

Move to ShenandoahOldHeuristics?

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

PR Review: https://git.openjdk.org/shenandoah/pull/409#pullrequestreview-1971490771
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1546448172
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1546460372
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1546466306
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1546467747