RFR: 8324067: GenShen: Isolate regulator thread to generational mode [v5]

[Y. Srinivas Ramakrishna]

On Tue, 13 Feb 2024 01:23:25 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Shenandoah's generational mode uses a second thread to evaluate heuristics. This is necessary so that the heuristics may interrupt a control thread which is running an old cycle in order to run a young cycle.
>> 
>> The changes here move the regulator thread into `ShenandoahGenerationalHeap`. The generational version of the control thread is also now instantiated only by the generational heap. The upstream version of the control thread has more or less been restored. To  summarize:
>> * An abstract base class called `ShenandoahController` has been introduced as the base class for the original and generational control threads. It has just one virtual method and it is not on a fast path. Much of the common code has been pulled up into this class.
>> * The respective control threads no longer need to check what mode they are in. They also no longer need to select which global generation they need to use.  The regulator thread is now only used by the generational mode so it no longer supports running only global cycles.
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix zero build some more

This looks very good; left a minor comment. Not necessarily actionable here or now, but just something to think about.

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

> 51: 
> 52: void ShenandoahControlThread::run_service() {
> 53:   ShenandoahHeap* heap = ShenandoahHeap::heap();

I see that in getting back to being closer to upstream tip of Shenandoah, we have lost (amongst other things), a bunch of const's here as an example. Also some fixes to comments as well. Was the the intention?

The alternative is that these kinds of changes to upstream code could be upstreamed so that they aren't lost in trying to reconcile with legacy Shenandoah.

Thoughts?

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

> 53:   ShenandoahHeap* heap = ShenandoahHeap::heap();
> 54: 
> 55:   GCMode default_mode = concurrent_normal;

Lost const.

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

> 64:   // shrinking with lag of less than 1/10-th of true delay.
> 65:   // ShenandoahUncommitDelay is in msecs, but shrink_period is in seconds.
> 66:   double shrink_period = (double)ShenandoahUncommitDelay / 1000 / 10;

Lost const.

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

> 74:     GCCause::Cause requested_gc_cause = _requested_gc_cause;
> 75:     bool explicit_gc_requested = is_gc_requested && is_explicit_gc(requested_gc_cause);
> 76:     bool implicit_gc_requested = is_gc_requested && !is_explicit_gc(requested_gc_cause);

Lost consts. I'll stop adding these "Lost const" comments further below, but you get the idea.

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

> 258:       }
> 259:     } else {
> 260:       // Allow allocators to know we have seen this much regions

"words allocated" was more accurate than "regions" in comment above.

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/391#pullrequestreview-1881341745
PR Review Comment: https://git.openjdk.org/shenandoah/pull/391#discussion_r1490102749
PR Review Comment: https://git.openjdk.org/shenandoah/pull/391#discussion_r1490103541
PR Review Comment: https://git.openjdk.org/shenandoah/pull/391#discussion_r1490104148
PR Review Comment: https://git.openjdk.org/shenandoah/pull/391#discussion_r1490104574
PR Review Comment: https://git.openjdk.org/shenandoah/pull/391#discussion_r1490108285