On Fri, 15 Nov 2024 14:11:10 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> 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 > > 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. https://bugs.openjdk.org/browse/JDK-8344321 > 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. https://bugs.openjdk.org/browse/JDK-8344321 > 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. https://bugs.openjdk.org/browse/JDK-8344321 > 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? https://bugs.openjdk.org/browse/JDK-8344321 > 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? https://bugs.openjdk.org/browse/JDK-8344321 > 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. https://bugs.openjdk.org/browse/JDK-8344321 > 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))`? https://bugs.openjdk.org/browse/JDK-8344321 > 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 https://bugs.openjdk.org/browse/JDK-8344321 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844218002 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219136 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219821 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219644 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844218123 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844218269 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219351 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219014
