On Tue, 8 Oct 2024 17:20:31 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 478 commits: > > - Fix merge error > - Merge remote-tracking branch 'jdk/master' into great-genshen-pr-redux > - Merge remote-tracking branch 'jdk/master' into great-genshen-pr-redux > - Merge branch 'shenandoah/master' into great-genshen-pr-redux > - Merge > - 8341099: GenShen: assert(HAS_FWD == _heap->has_forwarded_objects()) > failed: Forwarded object status is sane > > Reviewed-by: kdnilsen > - 8341485: GenShen: Make evac tracker a non-product feature and confine it > to generational mode > > Reviewed-by: kdnilsen, ysr > - Merge > - 8341042: GenShen: Reset mark bitmaps for unaffiliated regions when > preparing for a cycle > > Reviewed-by: kdnilsen > - 8339616: GenShen: Introduce new state to distinguish promote-in-place > phase as distinct from concurrent evacuation > > Reviewed-by: kdnilsen, shade, ysr > - ... and 468 more: https://git.openjdk.org/jdk/compare/b9db74a6...4db1e0e1 That is a big change - great work! In this first batch I've reviewed: src/hotspot/cpu src/hotspot/gc/shared src/hotspot/gc/shenandoah/c1 src/hotspot/gc/shenandoah/c2 src/hotspot/gc/shenandoah/heuristics src/hotspot/gc/shenandoah/mode I only have a few comments so far. Will review the remaining parts in separate batches. src/hotspot/share/gc/shenandoah/c1/shenandoahBarrierSetC1.cpp line 75: > 73: > 74: // Create a mask to test if the marking bit is set. > 75: // TODO: can we directly test if bit is set? That comment here is quite justified, IMO. I'm pretty sure that we could test for the flag in a single instruction, instead of doing the and-cmp sequence and even allocating a new register. Unless of course when C1 LIR can't represent it. Have you tried to implement that and failed, and therefore remove the comment? src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 96: > 94: size_t capacity = _space_info->soft_max_capacity(); > 95: size_t max_cset = (size_t)((1.0 * capacity / 100 * > ShenandoahEvacReserve) / ShenandoahEvacWaste); > 96: size_t free_target = (capacity * ShenandoahMinFreeThreshold) / 100 + > max_cset; Does re-arranging the math here risk overflow (e.g. on 32-bit)? src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.hpp line 79: > 77: > 78: protected: > 79: static const uint Moving_Average_Samples = 10; // Number of samples to > store in moving averages I've never seen that style for constants in HotSpot before. I'd either make it MOVING_AVERAGE_SAMPLES or MovingAverageSamples. I think the latter is more prevalent, but I am not certain. ------------- Changes requested by rkennke (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2360208858 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1795359413 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1795377554 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1795401675
