RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]

Fri Nov 15 14:49:11 UTC 2024

On Wed, 13 Nov 2024 19:32:53 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> This PR merges JEP 404, a generational mode for the Shenandoah garbage collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We would like to target JDK24 with this PR.
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 510 commits:
> 
>  - Merge branch 'merge-latest' into great-genshen-pr-redux
>  - Use new CompactHeader forwarding APIs in generational mode
>  - Merge remote-tracking branch 'jdk/master' into merge-latest
>  - Merge
>  - 8343649: Shenandoah: ShenandoahEvacInfo event does not follow JFR guidelines
>    
>    Reviewed-by: wkemper
>  - Merge
>  - 8343227: GenShen: Fold resource mark into management of preselected regions
>    
>    Reviewed-by: kdnilsen
>  - Merge openjdk/jdk tip into great-genshen-pr-redux
>  - Merge remote-tracking branch 'jdk/master' into merge-latest
>  - Merge remote-tracking branch 'jdk/master' into merge-latest
>  - ... and 500 more: https://git.openjdk.org/jdk/compare/889f9062...5e02b5d8

Sonar findings:

src/hotspot/share/gc/shenandoah/shenandoahEvacTracker.cpp line 39:

> 37:   if (_use_age_table) {
> 38:     _age_table = new AgeTable(false);
> 39:   }

Sonar caught it: Initialize `_age_table` to `nullptr` for extra safety. Current uses seem to be gated by `_use_age_table`, though.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1505:

> 1503:   size_t transferred_regions = 0;
> 1504:   ShenandoahLeftRightIterator iterator(&_partitions, which_collector, true);
> 1505:   idx_t rightmost = _partitions.rightmost_empty(which_collector);

Sonar: `rightmost` is not used.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1567:

> 1565:                                  <= _partitions.rightmost(ShenandoahFreeSetPartitionId::Collector))) {
> 1566:     ShenandoahHeapLocker locker(_heap->lock());
> 1567:     max_xfer_regions -=

Sonar: Value stored to `max_xfer_regions` here is not used.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1856:

> 1854:     size_t consumed_old_collector = 0;
> 1855:     size_t consumed_mutator = 0;
> 1856:     size_t available_old = 0;

Sonar: `available_old` is not used.  `available_young` is not used.

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 380:

> 378: 
> 379:   size_t old_evacuated = collection_set->get_old_bytes_reserved_for_evacuation();
> 380:   size_t old_evacuated_committed = (size_t) (ShenandoahOldEvacWaste * old_evacuated);

Sonar: implicit conversion from 'size_t' (aka 'unsigned long') to 'double' may lose precision

Not sure if it breaks any math.

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 455:

> 453:     // excess_old < unaffiliated old: we can give back MIN(excess_old/region_size_bytes, unaffiliated_old_regions)
> 454:     size_t excess_regions = excess_old / region_size_bytes;
> 455:     size_t regions_to_xfer = MIN2(excess_regions, unaffiliated_old_regions);

Sonar: Value stored to 'regions_to_xfer' during its initialization is never read

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 549:

> 547:     }
> 548:     if (r->age() >= tenuring_threshold) {
> 549:       if ((r->garbage() < old_garbage_threshold)) {

Sonar: double parentheses.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 276:

> 274:             // where abundance is defined as >= ShenGenHeap::plab_min_size().  In the former case, we try shrinking the
> 275:             // desired PLAB size to the minimum and retry PLAB allocation to avoid cascading of shared memory allocations.
> 276:             if (plab->words_remaining() < plab_min_size()) {

Sonar caught it: There is a `plab != nullptr` check above at L267, which implies `plab` can be `nullptr` here? This would SEGV.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 656:

> 654:     // may not evacuate the entirety of unprocessed candidates in a single mixed evacuation.
> 655:     const size_t max_evac_need = (size_t)
> 656:             (old_generation()->unprocessed_collection_candidates_live_memory() * ShenandoahOldEvacWaste);

Sonar: implicit conversion from 'size_t' (aka 'unsigned long') to 'double' may lose precision

Since this is a heuristics calculation, should we be precise here?

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 1003:

> 1001: }
> 1002: 
> 1003: namespace ShenandoahCompositeRegionClosure {

We usually avoid `namespace`-s. See Hotspot style guide: https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#namespaces

src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp line 89:

> 87: }
> 88: 
> 89: HeapWord* ShenandoahHeapRegion::allocate(size_t size, ShenandoahAllocRequest req) {

Sonar caught it: `ShenandoahAllocRequest` should be passed by reference here?

src/hotspot/share/gc/shenandoah/shenandoahHeapRegionCounters.hpp line 99:

> 97:                       size_t region_size, size_t protocolVersion);
> 98: 
> 99:   uint _count = 0;

Sonar caught it: this `_count` field does not seem to be used.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 157:

> 155:   size_t card_at_start = _rs->card_index_for_addr(address);
> 156:   HeapWord* card_start_address = _rs->addr_for_card_index(card_at_start);
> 157:   uint8_t offset_in_card = address - card_start_address;

Sonar: implicit conversion loses integer precision: 'long' to 'uint8_t' (aka 'unsigned char').

This looks risky. Should probably be `checked_cast<uint8_t>(pointer_delta(address, card_start_address, 1))`?

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 889:

> 887:   size_t previous_group_span = _group_entries[0] * _group_chunk_size[0];
> 888:   for (size_t i = 1; i < _num_groups; i++) {
> 889:     size_t previous_group_entries = (i == 1)? _group_entries[0]: (_group_entries[i-1] - _group_entries[i-2]);

Sonar: Value stored to `previous_group_entries` during its initialization is never read

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

PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2438690431
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843878467
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843904346
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843905277
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843912193
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843933121
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843906258
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843898116
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843857836
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843931737
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843872870
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843886835
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843892961
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843928920
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843907601