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

[Y. Srinivas Ramakrishna]

On Tue, 22 Aug 2023 16:26:07 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> 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

Would this be more succinct and easier to read?


  if (mode()->is_generational() && is_concurrent_old_mark_in_progress()) {
     assert(_gc_state.is_set(MARKING), "Marking flag should be set");
     mask = YOUNG_MARKING;
  } else {
     assert(_gc_state.is_clear(MARKING|OLD_MARKING|YOUNG_MARKING), "All marking flags should be clear");
     mask = MARKING | YOUNG_MARKING;
  }
  set_gc_state_mask(mask, in_progress);


where `is_set()` and `is_clear()` check if all flag bits in argument are either all set or all clear (which is different from their current sense). See comment below.

>> 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

This could also be simplified along the lines of my previous suggestion above.

Also the "if and only if" for state at the beginning here can be simplified to (but continue reading below):


   assert(has_forwarded_objects() == (_gc_state.is_set(UPDATEREFS | EVACUATION), "Error");


provided `is_set()` and `is_unset()` (or ideally called `is_clear()`)  methods are defined symmetrically to test all of the bits passed in the mask argument; i.e. `is_set()` answers true iff they are _all_ set, and `is_unset()` returns true iff they are _all_ unset. This will simplify many of the asserts that need to test the state of multiple bits at once.

That change might want to go in separately as it would affect a bunch of existing code/tests. And, yes, it is possible that that extent of change would be considered unacceptable upstream (but I am guessing it should be fine if made on tip now as a clean-up/refactor).

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/309#discussion_r1302212292
PR Review Comment: https://git.openjdk.org/shenandoah/pull/309#discussion_r1302235992