RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v6]

[Aleksey Shipilev]

On Mon, 4 Nov 2024 19:52:30 GMT, William Kemper <wkemper at 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