RFR: 8328235: GenShen: Robustify ShenandoahGCSession and fix missing use [v3]

[Y. Srinivas Ramakrishna]

On Fri, 17 May 2024 16:11:37 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> ~ShenandoahGCSession is intended to create a scope where the ShenandoahHeap's _gc_cause and _gc_generation field reflect the current gc cycle. We now check that we do not overwrite existing non-default settings (respectively _no_gc and nullptr). The destructor of the scope/stack object also resets these fields to their default settings, ensuring intended uses. This uncovered a situation where the scope was not entered when it should have been, which we have now fixed. A case of flickering of active_generation() was identified, and found to be benign. An assert now checks for this situation. The code has been made robust wrt the flickering (seen only by mutators executing load barriers).~ To be fixed.
>> 
>> *Testing*:
>> - [x] code pipeline
>> - [x] specjbb
>> - [x] jtreg:hotspot_gc and jtreg:hotspot:tier1 w/fastdebug
>> - [x] GHA
>
> Y. Srinivas Ramakrishna has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 32 additional commits since the last revision:
> 
>  - jcheck whitespace
>  - Fixed mismerge because of code moving between files in merge from
>    master; encapsulate assert & provide more useful debugging info. We
>    aren't done yet... It might be the case that we do away with the
>    sync/async split of the variable and instead move the relevant state
>    into the closure if possible. That might have to wait for another day
>    though based on whether it'll work well everywhere or not.
>  - Merge branch 'master' into active_generation
>  - Greater separation of gc_generation and active_generation fields of
>    ShHeap. WIP.
>  - Clean ups.
>  - jcheck whitespace
>  - Banish forcing entirely. Fix one case where full gc sets
>    active_generation directly. Might be other cases as well. Will need a
>    cleaner and more natural abstraction different from the unseemly
>    under-the-covers groveling that we do currently.
>  - Relax an assert that is too strong; still iterating execution paths,
>    more cleanups planned.
>  - Disallow forcing.
>    Test.
>  - Remove an incorrect gc status setting.
>  - ... and 22 more: https://git.openjdk.org/shenandoah/compare/4a8ee77b...61848c82

src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp line 108:

> 106:   // Weak reference processing
> 107:   ShenandoahReferenceProcessor* rp = heap->gc_generation()->ref_processor();
> 108:   assert(heap->gc_generation() == heap->active_generation(), "Should be identical at stw phases");

`assert_generations_reconciled();` instead.

src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp line 177:

> 175:   ShenandoahWorkerTimingsTracker timer(phase, ShenandoahPhaseTimings::ParallelMark, worker_id);
> 176:   ShenandoahReferenceProcessor* rp = ShenandoahHeap::heap()->gc_generation()->ref_processor();
> 177:   assert(ShenandoahHeap::heap()->gc_generation() == ShenandoahHeap::heap()->active_generation(),

`assert_generations_reconciled();` instead.

test/hotspot/jtreg/gc/shenandoah/oom/TestThreadFailure.java line 81:

> 79:             ProcessBuilder pb = ProcessTools.createLimitedTestJavaProcessBuilder(
> 80:                     "-Xmx32m",
> 81:                     // "-XX:+UnlockExperimentalVMOptions", "-XX:ShenandoahNoProgressThreshold=12", "-XX:+ShowMessageBoxOnError",

Ignore this. Will be removed; debugging detritus left in place until we believe we are ready.

-------------

PR Review Comment: https://git.openjdk.org/shenandoah/pull/407#discussion_r1605345202
PR Review Comment: https://git.openjdk.org/shenandoah/pull/407#discussion_r1605345705
PR Review Comment: https://git.openjdk.org/shenandoah/pull/407#discussion_r1605347017