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

[Y. Srinivas Ramakrishna]

On Tue, 25 Jun 2024 16:18:59 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> The mainline implementation of ShenandoahFreeSet was recently updated.  This PR integrates the upstream changes
>> into Generational Shenandoah.
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve consistency of OOM in GenShen mode
>   
>   Treat any full GC that enables a previously blocked allocation as good
>   progress.

A few questions/comments, but (delta of) changes look good to me.

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 329:

> 327:   if (gc.collect(cause)) {
> 328:     // Cycle is complete
> 329:     heap->notify_gc_progress();

I am trying to understand this. Is the following a reasonable summary of the underlying reasoning?

Any concurrent cycle that completes without degenerating is considered progress irrespective of whether it reclaimed anything. The idea is that if there were allocations during that concurrent cycle, they will have succeeded because, if not we'd have degenerated and not reached here: "collect()" would have returned "false".

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.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1032:

> 1030:         result = allocate_memory_under_lock(req, in_new_region);
> 1031:       }
> 1032:       if ((result != nullptr) && mode()->is_generational()) {

I am wondering why we are using a `mode()->is_generational()` filter here.

Would this not work in exactly the same way for the non-generational case as well?

If so, I'd extract this fix out more generally and upstream it separately as its own ticket for fixing the OOM'ing behavior for "no progress OOMs".

src/hotspot/share/gc/shenandoah/shenandoahMmuTracker.cpp line 129:

> 127:   double now = os::elapsedTime();
> 128:   double duration = now - _most_recent_timestamp;
> 129: 

Is this change just to remove spurious differences from upstream openjdk/jdk repo?

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/440#pullrequestreview-2139790908
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1653520690
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1653529666
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1653543616
PR Review Comment: https://git.openjdk.org/shenandoah/pull/440#discussion_r1653544682