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 Dropping the rest of my comments from today's read before I sign off. I have reviewed 200/230 files, will continue tomorrow. src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.hpp line 76: > 74: void vmop_entry_final_roots(); > 75: > 76: private: Feel free to just make all these `protected`. It is more fuzz to try to provide the minimal possible visibility, especially if it gives the ragged visibility blocks in source. The real reason these are `private` is to protect them from accidental external use. `protected` is also good for this. src/hotspot/share/gc/shenandoah/shenandoahMarkClosures.hpp line 34: > 32: class ShenandoahHeapRegion; > 33: > 34: class ShenandoahFinalMarkUpdateRegionStateClosure : public > ShenandoahHeapRegionClosure { There is the `shenandoahHeapRegionClosures.hpp` for these, no? src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp line 44: > 42: for (size_t idx = 0; idx < num_regions; idx++) { > 43: ShenandoahHeapRegion* r = heap->get_region(idx); > 44: if (r->is_affiliated() && heap->is_bitmap_slice_committed(r) && > !is_bitmap_clear_range(r->bottom(), r->end())) { I don't understand this for single gen mode. In that mode `is_affiliated() == false` always, right? So this check never passes, and `is_bitmap_clear` always returns `true`? src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 657: > 655: _generation(nullptr) { > 656: if (_options._verify_marked == > ShenandoahVerifier::_verify_marked_complete_satb_empty) { > 657: Threads::change_thread_claim_token(); It is fairly odd to see Verifier touching the claim token, since the bug _may be_ somewhere in parallel thread oop iteration infra. I think it is fine to just use `Threads::threads_do` (non-parallel version), which AFAIU does not require token modifications. ------------- PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2445988182 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848675048 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848786419 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848781508 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848711178
