On Mon, 4 Nov 2024 19:52:30 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 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 I read about half, here is a dump of my current observations. I'll read the second half a bit later. src/hotspot/share/gc/shenandoah/c2/shenandoahBarrierSetC2.cpp line 474: > 472: // elision safe. > 473: return; > 474: } Why is this safe for Shenandoah? I suspect it needs `CardTableBarrierSet::on_slowpath_allocation_exit` to work. `G1BarrierSetC2` gets it by subclassing `CardTableBarrierSetC2`. But `ShenandoahBarrierSetC2` does not. Should it? src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.cpp line 67: > 65: _region_data[i].clear(); > 66: } > 67: #endif I understand this is to make sure `union_tag` works well. But why don't we extend `clear` to initialize all fields, and do this block without `ASSERT`? This does not look like frequently used path. Generally, doing these inits only for debug modes might hide some assertion failures that would indicate a problem in product builds. src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.hpp line 118: > 116: #ifdef ASSERT > 117: assert(_union_tag != is_uninitialized, "Cannot fetch region from > uninialized RegionData"); > 118: #endif Style: No point in wrapping single-line `assert`-s in `#ifdef ASSERT`. 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. src/hotspot/share/gc/shenandoah/shenandoahCardStats.hpp line 47: > 45: CARD_STAT_UPDATE_REFS, > 46: MAX_CARD_STAT_LOG_TYPE > 47: }; These are in global namespace. There is a risk they would clash with some other symbol elsewhere. Do you need them to be global, or can they go into `ShenandoahCardStats`? 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? 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? 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? src/hotspot/share/gc/shenandoah/shenandoahVMOperations.hpp line 41: > 39: // - VM_ShenandoahInitUpdateRefs: initiate update references > 40: // - VM_ShenandoahFinalUpdateRefs: finish up update references > 41: // - VM_ShenandoahFinalRoots If we add it here, let's provide a comment: // - VM_ShenandoahFinalRoots: finish up the roots, shortcut cycle src/hotspot/share/jfr/metadata/metadata.xml line 1221: > 1219: <Field type="ulong" contentType="bytes" name="immediateBytes" > label="Immediate Bytes" /> > 1220: </Event> > 1221: Not sure if we _need_ this JFR event as part of GenShen? This bit is what forces us to do CSR, right? test/hotspot/jtreg/ProblemList.txt line 98: > 96: gc/TestAlwaysPreTouchBehavior.java#Epsilon 8334513 generic-all > 97: gc/stress/gclocker/TestExcessGCLockerCollections.java 8229120 generic-all > 98: This change is unnecessary. test/hotspot/jtreg/gc/shenandoah/oom/TestThreadFailure.java line 61: > 59: // case that the previously instantiated NastyThread > accumulated more than SheanndoahNoProgressThreshold > 60: // unproductive GC cycles before failing, the main thread > may not try a Full GC before it experiences > 61: // OutOfMemoryError exception. Do you really need comments in this test? ------------- PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2429755115 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1839969170 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1839985542 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1840008742 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1840173090 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1840153151 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1840187886 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1840195628 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1840199905 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1840201386 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1840096925 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1838665756 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1838196649
