RFR: 8325808: GenShen: Move generational mode code out of shFullGC.cpp [v2]

[Y. Srinivas Ramakrishna]

On Mon, 26 Feb 2024 21:49:56 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 460:
>> 
>>> 458: 
>>> 459: template<typename ClosureType>
>>> 460: void ShenandoahPrepareForCompactionTask::prepare_for_compaction(ClosureType& cl,
>> 
>> Looking at this method, it looks like it actually belongs as a public method in the closure, with that closure's methods invoked here becoming private to the closure. Makes for a narrower public API all around and keeps the loop in what appears to me in its more natural place.
>
> Hmm, I agree from a design perspective. However, we have two versions of the closure. For them to share the implementation of `prepare_for_compaction` as you suggest, I believe we'd need to introduce a common base class (or parent one closure to another) and make some of these methods virtual. Or were you thinking of making `prepare_for_compaction` a static member of a common base class and keeping the same signature. Or, just duplicating the implementation of `prepare_for_compaction` in each closure? Not sure if any of those are simpler than what we have here.

Yes, I think you are right. When I was writing that comment, I was thinking that the loop could just be duplicated into each of the two closures. I guess may be that doesn't particularly make it any simpler and moreover leads to some avoidable duplication. I'll leave it up to you as to which makes more sense to you.

Reviewed and approved!

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/398#discussion_r1503523836