On Thu, 21 Nov 2024 00:59:27 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 524 commits: > > - Merge remote-tracking branch 'shenandoah/master' into > great-genshen-pr-redux > - 8344670: GenShen: Use concurrent worker session for concurrent mark phase > > Reviewed-by: kdnilsen > - 8344640: GenShen: Reuse existing card mark barrier function when dropping > references > > Reviewed-by: shade, ysr > - 8344592: GenShen: Remove unnecessary comments and changes > > Reviewed-by: kdnilsen > - 8344263: GenShen: Reduce extraneous log messages at INFO level > > Reviewed-by: ysr, shade > - 8344638: GenShen: Verifier should not touch claim token > > Reviewed-by: shade > - Merge > - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap > > Reviewed-by: wkemper, ysr > - 8344321: GenShen: Fix various sonar scan warnings > > Reviewed-by: kdnilsen, shade > - 8344264: GenShen: Improve comments and method names > > Reviewed-by: shade > - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403 229/229 files read! This is an impressive, monumental piece of work, kudos. More comments from this read follow. I am going to circle back to some open threads in this PR. I am also running local tests, and in my ad-hoc performance tests generational mode performs admirably well. src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 881: > 879: // update costs on slow path. > 880: monitoring_support()->notify_heap_changed(); > 881: _heap_changed.set(); Why not leave `try_set` intact? This alleviates contention on the shared counter, AFAICS. src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1972: > 1970: // Check that if concurrent weak root is set then active_gen isn't > null > 1971: assert(!is_concurrent_weak_root_in_progress() || active_generation() > != nullptr, "Error"); > 1972: shenandoah_assert_generations_reconciled(); Why all of this is checked here? I would have thought `gc_state` machinery should only check things related to gc-state. src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 134: > 132: bool is_thread_safe() override { return true; } > 133: }; > 134: This looks like belonging in one of the `*closures.hpp` header. src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 514: > 512: > 513: public: > 514: ShenandoahController* control_thread() { return _control_thread; } This method and field is probably `controller` then, right? src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 663: > 661: // ---------- CDS archive support > 662: > 663: bool can_load_archived_objects() const override { return > !ShenandoahCardBarrier; } This means CDS heap loading is not yet working with generational Shenandoah? This looks OK for the experimental code. Please submit a bug for this and assign it to me. I will take a look at it some time later. src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 754: > 752: oop try_evacuate_object(oop src, Thread* thread, ShenandoahHeapRegion* > from_region, ShenandoahAffiliation target_gen); > 753: public: > 754: I think this new line should be before `public:` src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp line 64: > 62: } > 63: } > 64: return _mark_bit_map.is_bitmap_clear_range(start, end); Comprehension check: this seems to bail out the moment it discovers an uncommitted slice. Does it really happen? More worryingly, if there is a mix of committed and uncommitted chunks, this method returns `true`, even if committed chunks are not actually clean? ------------- PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2450709063 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852054187 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852075850 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851646208 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851763351 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851761345 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851765188 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852139996
