On Wed, 13 Nov 2024 19:32:53 GMT, William Kemper <wkem...@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
