On Wed, 13 Nov 2024 12:23:26 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 503 commits: >> >> - 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 >> - Merge >> - 8342861: GenShen: Old generation in unexpected state when abandoning >> mixed gc candidates >> >> Reviewed-by: kdnilsen >> - 8342734: GenShen: Test failure >> gc/shenandoah/TestReferenceRefersToShenandoah.java#generational >> >> Reviewed-by: ysr >> - 8342919: GenShen: Fix whitespace >> >> Reviewed-by: xpeng, kdnilsen >> - 8342927: GenShen: Guarantee slices of time for coalesce and filling >> >> Reviewed-by: kdnilsen >> - 8342924: GenShen: Problem list >> gc/shenandoah/TestReferenceRefersToShenandoah.java >> >> Reviewed-by: kdnilsen, ysr >> - 8342848: Shenandoah: Marking bitmap may not be completely cleared in >> generational mode >> >> Reviewed-by: wkemper >> - ... and 493 more: https://git.openjdk.org/jdk/compare/1c448347...19b25bc3 > > src/hotspot/share/gc/shenandoah/shenandoahAsserts.hpp line 238: > >> 236: #define shenandoah_assert_control_or_vm_thread_at_safepoint() >> 237: #define shenandoah_assert_generational() >> 238: #define shenandoah_assert_generations_reconciled() >> \ > > There seems to be dangling `` on this line. Good catch. > src/hotspot/share/gc/shenandoah/shenandoahStackWatermark.cpp line 77: > >> 75: return reinterpret_cast<OopClosure*>(context); >> 76: } else { >> 77: if (_heap->is_concurrent_weak_root_in_progress()) { > > Comprehension check: We flips this because in generational mode we _can_ have > both concurrent weak roots and concurrent mark in progress at the same time, > and we need to prioritize evacs, right? Correct. Although no old marking threads will ever run during a young collection. > src/hotspot/share/gc/shenandoah/shenandoahUtils.hpp line 50: > >> 48: switch (generation_type) { >> \ >> 49: case NON_GEN: >> \ >> 50: return prefix " (NON-GENERATIONAL)" postfix; >> \ > > In the interest of keeping the non-generational Shenandoah logging intact, > should we drop ` (NON-GENERATIONAL)` here? No objections here. @ysramakrishna , @kdnilsen ? > src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp line 83: > >> 81: void VM_ShenandoahInitMark::doit() { >> 82: ShenandoahGCPauseMark mark(_gc_id, "Init Mark", >> SvcGCMarker::CONCURRENT); >> 83: set_active_generation(); > > Why is it here, and not down in `entry_*` methods, probably in helper classes? I think it's here for the same reason `propagate_gc_state_to_java_threads` is here. Having it here makes it easier to see that this critical synchronization step happens for every safepoint. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1841308265 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1841308915 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1841310699 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1841313192
