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

[William Kemper]

On Fri, 23 Jun 2023 01:21:11 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> When a cycle _only_ promotes regions in place, it is not necessary to update refs.
>
> 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?

I'll add a comment to clarify. The in-place promotions happen during the evacuation phase, _even when the collection set is empty_. We've discussed pulling this action into a separate/distinct phase, or _only_ allowing in-place-promotions  when the collection set is _not_ empty (thus eliminating the special case of cycles that _only_ perform in-place-promotions).

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

Previously, the verifier treated "evacuation in progress" and "heap has forwarded objects" as the same state. We now have a case where "evacuation"  (promote-in-place) may happen without forwarded objects.

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

I think the `expected` state here is effectively a `MUST`.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/291#discussion_r1240109138
PR Review Comment: https://git.openjdk.org/shenandoah/pull/291#discussion_r1240111292
PR Review Comment: https://git.openjdk.org/shenandoah/pull/291#discussion_r1240114264