RFR: 8335493: check_gc_overhead_limit should reset SoftRefPolicy::_should_clear_all_soft_refs [v2]

[Leela Mohan Venati]

On Fri, 5 Jul 2024 08:43:47 GMT, Denghui Dong <ddong at openjdk.org> wrote:

>> At a quick glance it looks okay, but I've not looked at it carefully enough to say I've reviewed it.  Given the amount of
>> discussion I can't agree with it being "trivial".  Small, yes, but with potentially significant impact.  And HotSpot rules
>> around trivial changes and bypassing some of the review process requires agreement by the Reviewer, of which
>> there is none recorded in the discussion here.
>
> @kimbarrett 
> 
> Sorry about that. I overlooked the rule that hotspot non-trivial changes require reviews from at least two reviewers.
> I'll be more careful in the future to follow the rules strictly.
> 

Hi @D-D-H / @mmyxym / @albertnetymk / @kimbarrett 

My name is Leela Mohan Venati. I am part of Service Now JDK team (@zhengyu123  is also part of it) . I found this issue few weeks back. We were trying to file this bug upstream to fix it. In the meantime, Good to see this fix coming. Thanks. 

As part of fixing the bug, I did some archeology to understand how this issue occurred.  This bug was introduced as [8198515: Extract SoftReferencePolicy code out of CollectorPolicy ](https://github.com/openjdk/jdk17/commit/f408526f3073acdeda1e001ab53dc360ea6f1eb2). As the name suggests, this change introduced new classes SoftRefPolicy  and SoftRefGenPolicy. These classes factored out CollectorPolicy classes.  These new classes  importantly hold two important attributes - 
a) "_all_soft_refs_clear" -  Boolean value enabled after the successful completion of "clear all" soft ref policy enabled Full GC.  
b) " _should_clear_all_soft_refs" - Boolean value which is enabled ergonomically by overhead checker. Note that Overhead checker is only applicable for parallelGC and only after full GC and when adaptive sizing policy is enabled only.  This value is used for future FullGC. 

Note that first flag is generally be applicable for any GC algorithm irrespective of overhead checker is available or not. Reason:  "Allocation failure" GC requests can ask for "clear all" soft ref policy Full GCs.  It is internally called "max compaction" Full GC.  
However, second flag is only applicable if there is overhead checker which ergonomically enables "clear all" policy. 

GC algorithms which don't have the overhead checker are expected to use SoftRefPolicy type for "_soft_ref_policy" field. Using SoftRefPolicy, there is no need of "resetting" of "_should_clear_all_soft_refs" after Full GC is completed. Check the SoftRefPolicy::cleared_all_soft_refs().   GenCollectedHeap (which is for Serial GC) should have used SoftRefPolicy instead of SoftRefGenPolicy [here](https://github.com/openjdk/jdk17/commit/f408526f3073acdeda1e001ab53dc360ea6f1eb2#diff-983a579a9bfabca3b17e9e6d6ff3b554c35a32e92ff0a0eec1d113064aca21b0R74)

GC algorithms which have the overhead checker are expected to use SoftRefGenPolicy for "_soft_ref_policy" field. This allows "resetting" of "_should_clear_all_soft_refs" after  Full GC is completed.  Check the [SoftRefGenPolicy::cleared_all_soft_refs](https://github.com/openjdk/jdk17/commit/f408526f3073acdeda1e001ab53dc360ea6f1eb2#diff-343f154ae8ddb14a561b33cbe2a667c8eb349d628358282a891f0f19193004eeR30) . Also note that , it handles "near overhead limit" to reset for not.  ParallelScavengeHeap (which is for Parallel GC) should have used SoftRefGenPolicy instead of SoftRefPolicy [here](https://github.com/openjdk/jdk17/commit/f408526f3073acdeda1e001ab53dc360ea6f1eb2#diff-f6a37dd865cabd1f1b1274cf87cdd3119f423be40a66759cd688408560badd13R63).  That's the bug. 

Except for the introduction of the bug, [change](https://github.com/openjdk/jdk17/commit/f408526f3073acdeda1e001ab53dc360ea6f1eb2) itself is nice cleanup change.  

Now why am i explaining this back story ?

I would have liked this change to be fixed using SoftRefGenPolicy type for "_soft_ref_policy" field in ParallelScavengeHeap. However, i do notice that SoftRefGenPolicy is removed in the master that means, we need add the file which was removed.

On jdk17u branch, i made a patch (see the attachment) which renames SoftGenRefPolicy as "PSSoftRefPolicy" (following the convention of parallel GC specific class) and used PSSoftRefPolicy for "soft_ref_policy" field . 
[patch.txt](https://github.com/user-attachments/files/16170231/patch.txt)

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

PR Comment: https://git.openjdk.org/jdk/pull/19982#issuecomment-2221793164