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

[William Kemper]

On Tue, 21 May 2024 01:23:32 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 when used concurrently by mutators while it was being modified by the controller thread. To deal with this, we have carefully gone through the setting and use of the field, and found that an expedient fix for the race is to split the variable into two:
>> - _gc_generation is set & cleared by the controller thread whenever it enters and exits a GC scope, and services concurrent gc cycles for young or old generations.
>> - _active_generation is set to the value in _gc_generation at the start of each Shenandoah GC safepoint operation so that mutator threads and load barriers always see a consistent value between safepoints.
>> 
>> Asserts check the protocol for setting and clearing the variables.
>> 
>> An alternative approach is to not use a global variable for the _gc_generation indirected through the heap, but rather to pass it into the closures that do the work. This would work as well, but the changes would potentially touch more code. We would still have to have set the variable that is consulted by the load barriers, in a mutator-safe fashion at a safepoint, like we do today. This or other alternative approaches may be investigated in the future to potentially make this protocol more self-contained and robust rather than leaking as it does today into many places in the code.
>> 
>> *Testing*:
>> - [x] code pipeline
>> - [x] specjbb testing
>> - [ ] specjbb performance
>> - [x] jtreg:hotspot_gc and jtreg:hotspot:tier1 w/fastdebug
>> - [x] GHA
>
> Y. Srinivas Ramakrishna has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Small clean-ups.

Left a few minor coding suggestings. On a higher level, it's not clear to me when and which threads should use `gc_generation` or `active_generation`. I gather that active_generation is set on the safepoint and `gc_generation` is set by control thread, does that mean any code before `init-mark` needs to use `gc_generation`?

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 931:

> 929:   if (!CompressedOops::is_null(obj)) {
> 930:     if (!_mark_context->is_marked(obj)) {
> 931:       _heap->assert_generations_reconciled();

Could put this in `shenandoahAsserts.hpp` as macro: `shenandoah_assert_generations_reconciled()`.

src/hotspot/share/gc/shenandoah/shenandoahController.cpp line 31:

> 29: #include "gc/shenandoah/shenandoahHeapRegion.inline.hpp"
> 30: 
> 31: Thread* ShenandoahController::_thread = nullptr;

I don't think this is necessary. We already have `ShenandoahHeap::control_thread()`, which is the same thing. That is `ShenandoahControlThread::this == Thread::current() == ShenandoahHeap::_control_thread`.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp line 134:

> 132:   {
> 133:     const size_t old_garbage_threshold = (ShenandoahHeapRegion::region_size_bytes() * ShenandoahOldGarbageThreshold) / 100;
> 134:     assert(_heap->gc_generation()->is_mark_complete(), "sanity");

Could use `_heap->gc_generation()->complete_marking_context()` on line 129, which just asserts that `is_mark_complete` before returning marking context.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1578:

> 1576: void ShenandoahHeap::set_gc_generation(ShenandoahGeneration* generation) {
> 1577:   _gc_generation = generation;
> 1578:   assert(Thread::current() == ShenandoahController::thread() ||

Could use `shenandoah_assert_control_or_vm_thread` from `shenandoahAsserts.hpp` here?

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 376:

> 374: 
> 375:   assert(is_in(obj), "only check if is in active generation for objects (" PTR_FORMAT ") in heap", p2i(obj));
> 376:   assert(gen == (ShenandoahGeneration*)old_generation() ||

These casts shouldn't be necessary any more.

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

Changes requested by wkemper (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/407#pullrequestreview-2074963704
PR Review Comment: https://git.openjdk.org/shenandoah/pull/407#discussion_r1612282304
PR Review Comment: https://git.openjdk.org/shenandoah/pull/407#discussion_r1612299050
PR Review Comment: https://git.openjdk.org/shenandoah/pull/407#discussion_r1612286279
PR Review Comment: https://git.openjdk.org/shenandoah/pull/407#discussion_r1612289975
PR Review Comment: https://git.openjdk.org/shenandoah/pull/407#discussion_r1612294430