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 Some more stuff. 180/230 files done %) src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 420: > 418: } > 419: int gc_state = _heap->gc_state(); > 420: if ((gc_state & ShenandoahHeap::YOUNG_MARKING) != 0) { It is not very clear this works fine with single-gen mode. Does `MARKING` imply `YOUNG_MARKING` in single-gen mode? I think we better make it abundantly clear with `is_generational()` checks. src/hotspot/share/gc/shenandoah/shenandoahCollectionSetPreselector.hpp line 34: > 32: ShenandoahCollectionSet* _cset; > 33: bool* _pset; > 34: ResourceMark _rm; Not necessarily for this PR, but something to rectify going forward. It is generally not safe to hide `ResourceMark`-s like this in the objects. Sometimes the callers return stuff allocated in resource area, and if the object like this leaves the scope, it would corrupt the memory. Normally, when we develop resource-area-returning paths, we look around near `ResourceMark`-s to see is any thing fishy is going on. It does not help when `RM` is hidden in the object like this. src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 276: > 274: log_info(gc)("GC cancellation took %.3fs", cancel_time); > 275: _cancel_requested_time = 0; > 276: } Do we need this? Is this useful? src/hotspot/share/gc/shenandoah/shenandoahHeapRegionCounters.cpp line 49: > 47: const char* cns = PerfDataManager::name_space("shenandoah", > "regions"); > 48: _name_space = NEW_C_HEAP_ARRAY(char, strlen(cns)+1, mtGC); > 49: strcpy(_name_space, cns); // copy cns into _name_space Suggestion: strcpy(_name_space, cns); (The comment is self-obvious) src/hotspot/share/gc/shenandoah/shenandoahHeapRegionCounters.cpp line 57: > 55: PerfDataManager::create_constant(SUN_GC, cname, PerfData::U_None, > num_regions, CHECK); > 56: > 57: cname = PerfDataManager::counter_name(_name_space, > "protocol_version"); //creating new protocol_version Suggestion: cname = PerfDataManager::counter_name(_name_space, "protocol_version"); src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.cpp line 193: > 191: Phase phase = Phase(pi); > 192: if (is_worker_phase(phase)) { > 193: double sum = uninitialized(); I think you can avoid changing this method to limit the churn. src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp line 455: > 453: // Note: would be sufficient to mark only the card that holds the > start of this Reference object. > 454: > heap->old_generation()->card_scan()->mark_range_as_dirty(cast_from_oop<HeapWord*>(reference), > reference->size()); > 455: } Two things: a) This sounds like `card_table_barrier(reference, raw_referent)`? Since this code is getting called for every dropped reference, just checking a `ShenandoahCardBarrier` flag sounds more efficient. b) Is there a point in dirtying up to `reference->size()`? src/hotspot/share/gc/shenandoah/shenandoahSharedVariables.hpp line 125: > 123: ShenandoahSharedValue ov = Atomic::load_acquire(&value); > 124: // We require all bits of mask_val to be set > 125: if ((ov & mask_val) == mask_val) { This looks like a pre-existing bug, right? I assume this does not affect current Shenandoah, since we do not set multiple bits. I am fine with doing this in this PR, but it would probably be good to backport this fix in earlier releases too. ------------- PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2439178281 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848286637 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1846888999 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844183614 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844187355 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844186936 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1846892659 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1846923725 PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848264678
