RFR: 8327000: GenShen: Integrate updated Shenandoah implementation of FreeSet into GenShen [v27]

[Kelvin Nilsen]

On Tue, 25 Jun 2024 21:39:58 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahDegeneratedGC.cpp line 307:
>> 
>>> 305:   // because that probably means the heap is overloaded and/or fragmented.
>>> 306:   if (!metrics.is_good_progress()) {
>>> 307:     heap->notify_gc_no_progress();
>> 
>> Again, let me make sure I understand the reasoning here:
>> 
>> We are already going to cancel gc and degenerate, so the concurrent collect will return failure, and the control thread will not record progress.
>> 
>> But I don't see anything that actually records lack of progress.
>> 
>> I assume then that a degenerated GC in and of itself isn't indicative of lack of progress, and that lack of  progress shouldn't be recorded here, but rather after having worked harder, perhaps in `op_degenerated_futile()`?
>> 
>> It would be good to leave some clarifying comments for when we read this code many months later and have forgotten the context. May be something like:
>> 
>> 
>>   // We haven't made progress; we'll try harder by canceling GC and upgrading to a full gc
>>   // to see if we make more progress.
>
> I think that's intentional. We only want to record no-progress after a full GC.

The description of ShenandoahNoProgressThreshold speaks only in terms of Full GCs.  In the case that degenerated fails to make progress, we will escalate to full GC.  If that Full GC fails to make progress, it will notify_gc_no_progress().  It's really part of the same GC cycle.  I don't think it's appropriate to count no progress twice for this situation.  If the Full GC does make progress, then it will reset the no-progress counter to zero, and there was no value in incrementing it here...

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1653630570