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

[Liang Mao]

On Thu, 11 Jul 2024 01:03:28 GMT, Leela Mohan Venati <duke at openjdk.org> wrote:

>> @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 part of [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-343f154ae8ddb14a561b33cbe2a667c8e...

Hi @leelamv , I just want to fix the unpaired set/reset of clear-soft-ref for Parallel GC. You can still refactor the policy on Parallel GC and @albertnetymk had some suggestions in review comments. I think that would be quite welcome.

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

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