RFR: 8328307: GenShen: Re-enable old-has-grown trigger for old-generation GC [v6]
Y. Srinivas Ramakrishna
ysr at openjdk.org
Sat Apr 13 01:35:03 UTC 2024
On Thu, 4 Apr 2024 15:32:05 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:
>
> Move trigger code to ShenandoahOldHeuristics from ShenandoahOldGeneration
A few comments, but may be not worth addressing all of them at this time. The division of code & state between the heuristics object on the one hand, and the heap/generation/space objects on the other is still a bit unclear, and we might want to take a fresh look afterwards and clean it up to make things more self-contained, if possible. SpaceInfo goes part of the way in abstracting these APIs and embedding a hook into the heuristics object, but specific heuristics objects also seem to want to look at more specific information from the generations/spaces in places, which makes this a bit difficult (or at least somewhat awkward in places).
src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 548:
> 546: }
> 547:
> 548: void ShenandoahOldHeuristics::trigger_collection_if_fragmented(ShenandoahGenerationalHeap* gen_heap, ShenandoahOldGeneration* old_gen,
Should `ShenandoahOldHeurtitics` just keep pointers to `gen_heap` and `old_gen`, and slim down these APIs so these don't need to be passed around? (Similarly `ShenandoahYoungHeuristics`?)
src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 564:
> 562: // humongous allocation was not required.
> 563:
> 564: size_t old_available = old_gen->available();
Can you use `_space_info->available()` and avoid passing `old_gen` to this method?
src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 566:
> 564: size_t old_available = old_gen->available();
> 565: size_t region_size_bytes = ShenandoahHeapRegion::region_size_bytes();
> 566: size_t old_unaffiliated_available = old_gen->free_unaffiliated_regions() * region_size_bytes;
Alas, `ShenandoahSpaceInfo` doesn't have the notion of `free_unaffliated_regions()`, although perhaps it could be extended to include it? I am not sure if all the implementors of the `ShenandoahSpaceInfo` interface would have a notion of "free" and "affiliated" regions, though (in particular, the non-generational heap's space info).
src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 579:
> 577: double density_threshold = (eighths - 2) / 8.0;
> 578: if ((old_region_span >= span_threshold) && (old_density < density_threshold)) {
> 579: gen_heap->old_heuristics()->trigger_old_is_fragmented(old_density, first_old_region, last_old_region);
Just `trigger_old_is_fragmented()` because `gen_heap->old_heuristics()` is `this` ?
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`.
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?
src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 595:
> 593: old_gen->used(), old_gen->get_humongous_waste(), gen_heap->capacity());
> 594: if (old_used > trigger_threshold) {
> 595: gen_heap->old_heuristics()->trigger_old_has_grown();
Just `trigger_old_has_grown()` because `gen_heap->old_heuristics()` is the same as`this` ?
-------------
Marked as reviewed by ysr (Committer).
PR Review: https://git.openjdk.org/shenandoah/pull/409#pullrequestreview-1998774551
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1563432976
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1563440112
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1563452131
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1563430879
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1563504301
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1563510168
PR Review Comment: https://git.openjdk.org/shenandoah/pull/409#discussion_r1563500293
More information about the shenandoah-dev
mailing list