RFR: JDK-8314777: GenShen: Alias young and old marking bits to legacy Shenandoah marking bit in gc state

[Kelvin Nilsen]

On Tue, 22 Aug 2023 16:20:54 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> This change sets the MARKING bit of _gc_state whenever either Young marking or Old marking is active.  With this change, barrier code more closely resembles the code in the original single-generation Shenandoah.  The performance impact is negligible.  The primary benefit is to simplify code reviews and clarify that the addition of generational mode to Shenandoah does not negatively impact performance of single-generation Shenandoah.
>
> src/hotspot/share/gc/shenandoah/c2/shenandoahBarrierSetC2.cpp line 944:
> 
>> 942:     int flags = ShenandoahHeap::HAS_FORWARDED;
>> 943:     if (ShenandoahIUBarrier) {
>> 944:       assert(!ShenandoahHeap::heap()->mode()->is_generational(), "Generational mode does not support IU barrier");
> 
> Not sure we need an assert here, we could check this condition in `shenandoahGenerationalMode.cpp` or `shenandoahArguments.cpp` to minimize deltas from upstream.

Thanks.  Moving this check to shenandoahArguments.cpp.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2460:
> 
>> 2458:   if (!in_progress && is_concurrent_old_mark_in_progress()) {
>> 2459:     assert(mode()->is_generational(), "Only generational GC has old marking");
>> 2460:     // If old-marking is in progress when we turn off YOUNG_MARKING, leave MARKING (and OLD_MARKING) on
> 
> Can we assert that gc state mask has `MARKING` set here?

adding this

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2468:
> 
>> 2466: }
>> 2467: 
>> 2468: void ShenandoahHeap::set_concurrent_old_mark_in_progress(bool in_progress) {
> 
> Should we assert that if `has_forwarded_objects` that the gc state mask has `UPDATEREFS` set?

I think original code may have been wrong.  When we set_gc_state_mask(), this sets the bits specified in the argument and leaves other bits alone.  So UPDATEREFS or EVACUATION bit should have already been on.  I'll go ahead and add this assert.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2470:
> 
>> 2468: void ShenandoahHeap::set_concurrent_old_mark_in_progress(bool in_progress) {
>> 2469:   if (!in_progress && is_concurrent_young_mark_in_progress()) {
>> 2470:     // If young-marking is in progress when we turn off OLD_MARKING, leave MARKING (and YOUNG_MARKING) on
> 
> Similarly, assert that `MARKING` is set here?

and adding this

> src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 661:
> 
>> 659: 
>> 660: inline bool ShenandoahHeap::is_idle() const {
>> 661:   return _gc_state.is_unset(MARKING | YOUNG_MARKING | OLD_MARKING | EVACUATION | UPDATEREFS);
> 
> We should just need to check `MARKING` now, right?

true.  (I guess I can also add assert (!MARKING || (YOUNG_MARKING || OLD_MARKING)).  I'll make this change.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/309#discussion_r1301926924
PR Review Comment: https://git.openjdk.org/shenandoah/pull/309#discussion_r1301900108
PR Review Comment: https://git.openjdk.org/shenandoah/pull/309#discussion_r1301990462
PR Review Comment: https://git.openjdk.org/shenandoah/pull/309#discussion_r1301900332
PR Review Comment: https://git.openjdk.org/shenandoah/pull/309#discussion_r1301905689