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 I reviewed the rest of `src/hotspot/shared/gc/shenandoah`. I tried to find places where this could affect performance or even correctness of single-gen Shenandoah, but could not find any (altough, some places are a bit shady and hard to figure out, e.g. ShFreeSet). Overall I only have a couple of comments. src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 579: > 577: st->print("Status: "); > 578: if (has_forwarded_objects()) st->print("has forwarded > objects, "); > 579: if (is_concurrent_mark_in_progress()) st->print("marking, "); What is this printing when not running with generational mode? src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 535: > 533: ShenandoahPacer* pacer() const { return _pacer; > } > 534: > 535: ShenandoahPhaseTimings* phase_timings() const { return > _phase_timings; } The indentation is off here. src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 335: > 333: uint ShenandoahHeap::get_object_age(oop obj) { > 334: // This is impossible to do unless we "freeze" ABA-type oscillations > 335: // With Lilliput, we can do this more easily. The comment about Lilliput can be removed. Since we only return the actual age when the mark is not displaced, we already to the correct thing. With lightweight-locking, the mark can never be displaced, and this code should just work. src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 396: > 394: } > 395: > 396: inline bool ShenandoahHeap::is_old(oop obj) const { What is the difference between this method and the above is_in_old()? Why does it need to check that active generation is young? src/hotspot/share/gc/shenandoah/shenandoahMmuTracker.hpp line 28: > 26: #define SHARE_GC_SHENANDOAH_SHENANDOAHMMUTRACKER_HPP > 27: > 28: #include "runtime/mutex.hpp" I think you don't use Mutex in this file. ------------- Changes requested by rkennke (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2362458572 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1796745606 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1796795684 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1796806016 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1796807913 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1796848150
