RFR: 8310574 GenShen: Should not update-references for in-place-promotions

[Y. Srinivas Ramakrishna]

On Thu, 22 Jun 2023 23:41:25 GMT, William Kemper <wkemper at openjdk.org> wrote:

> When a cycle _only_ promotes regions in place, it is not necessary to update refs.

I'll go back and re-read this so I understand the higher level shape of this, but I don't think I understand the transitions and states sufficiently well here to "approve". Hopefully my comments & questions help a bit in terms of a review of sorts.

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

> 197:     // Concurrently evacuate
> 198:     entry_evacuate();
> 199:     if (check_cancellation_and_abort(ShenandoahDegenPoint::_degenerated_evac)) return false;

Not something you introduced, but could you change this to:
 ```
    if (check_cancellation_and_abort(ShenandoahDegenPoint::_degenerated_evac)) {
      return false;
    }

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

> 786:         }
> 787: 
> 788:         heap->set_evacuation_in_progress(true);

The new code at lines 796-801 makes sense. However, then why do we set evacuation in progress at line 788 above if there might be nothing to evacuate in the event that the collection set is empty, in which case the boolean parm should be like at line 799 below? If so, wouldn't the two conditions be synonymous? If not, what is the information in one that is not in the other. In that case, could the `set_evacuation_in_progress` method be renamed for greater clarity?

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

> 844:         // From here on, we need to update references.
> 845:         heap->set_has_forwarded_objects(true);
> 846: 

At both places, this makes sense since the collection set isn't empty, apropos of my earlier comment.

What is the reason to not have `set_has_forwarded_objects` being true at verification time? I notice that you made the same change in the earlier part as well. Would it be worth a documentation comment?

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 803:

> 801:       case _verify_gcstate_evacuation:
> 802:         enabled = true;
> 803:         expected = ShenandoahHeap::EVACUATION;

May be the semantics of these states should be explained via a state diagram where these are defined using "may" and "must" modalities, so the semantics are clear and there isn't ambiguity. ("has" to me feels like a "must" modality, in the modal logic sense; `EVACUATION` could similarly be clarified with a `MAY` or `MUST` modal sense.)

I am rambling, but hopefully you get the idea of what I am trying to articulate :-)

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

PR Review: https://git.openjdk.org/shenandoah/pull/291#pullrequestreview-1494179735
PR Review Comment: https://git.openjdk.org/shenandoah/pull/291#discussion_r1239161786
PR Review Comment: https://git.openjdk.org/shenandoah/pull/291#discussion_r1239170171
PR Review Comment: https://git.openjdk.org/shenandoah/pull/291#discussion_r1239173448
PR Review Comment: https://git.openjdk.org/shenandoah/pull/291#discussion_r1239185711