RFR (M): 8212206: Refactor AdaptiveSizePolicy to separate out code related to GC overhead

[Man Cao]

Thanks for taking a look, Jiangli. I renamed "permanent gen" to "metaspace".
I double checked that currently full GCs due to metaspace expansion will
not affect the calculation of GC overhead .

New webrev: https://cr.openjdk.java.net/~manc/8212206/webrev.04/

-Man


On Sun, Mar 3, 2019 at 11:48 AM Jiangli Zhou <jianglizhou at google.com> wrote:

> Hi Man,
>
> As both Thomas and Per seemed to be okay with the refactoring, you
> probably had sufficient approval already.
>
> The following comment in adaptiveSizePolicy.cpp seems to be outdated. The
> comment predates the PermGen removal. It's probably a good idea to also
> cleanup the comment as part of your refactoring change.
>
>  281     // Note that the gc time limit test only works for the collections 282     // of the young gen + tenured gen and not for collections of the 283     // permanent gen.  That is because the calculation of the space 284     // freed by the collection is the free space in the young gen + 285     // tenured gen.
>
> Thanks and regards,
>
> Jiangli
>
>
> On Fri, Mar 1, 2019 at 1:43 PM Man Cao <manc at google.com> wrote:
>
>> Hi,
>>
>> I rebased the patch to latest tip:
>> https://cr.openjdk.java.net/~manc/8212206/webrev.03/
>>
>> Could someone give a second "looks good"?
>>
>> -Man
>>
>>
>> On Sat, Jan 26, 2019 at 4:35 PM Man Cao <manc at google.com> wrote:
>>
>>> Friendly ping. Could anyone give a second "looks good"?
>>>
>>> As for the develop flag
>>> AdaptiveSizePolicyGCTimeLimitThreshold/GCOverheadLimitThreshold, I added a
>>> note about it in https://bugs.openjdk.java.net/browse/JDK-8212084.
>>>
>>> -Man
>>>
>>>
>>> On Tue, Jan 15, 2019 at 6:41 PM Man Cao <manc at google.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> I rebased the patch to tip and updated year in some headers to 2019,
>>>> without making any real change:
>>>> http://cr.openjdk.java.net/~manc/8212206/webrev.02/
>>>>
>>>>
>>>> I don't foresee that this will be implemented, or even makes sense, for
>>>>> ZGC. As I see it, this is only a thing STW collectors. For that
>>>>> reason,
>>>>> I don't think it belongs in CollectedHeap. Keeping it as a separate
>>>>> utility class for collectors that want to use it sounds better.
>>>>>
>>>> Sounds good to keep this patch in the current state, without further
>>>> changing the CollectedHeap class.
>>>>
>>>> I haven't looked very closely at the patch, but couldn't help to notice
>>>>> that the option is called "GCOverheapLimitThreshold" (and
>>>>> "AdaptiveSizePolicyGCTimeLimitThreshold" before that), which is a
>>>>> tautology and a not very good description of what it is.
>>>>> How about we take the opportunity to clean this up and completely
>>>>> ditch
>>>>> the "gc_overhead_limit_count" thing and get rid of this option? It's a
>>>>> "develop" option, so it's not available to normal users anyway. Has
>>>>> anyone of you ever used this option and actually find it valuable?
>>>>
>>>> I didn't find any users inside Google that require changing this option.
>>>> That said, some users did complain that UseGCOverheadLimit for
>>>> ParallelGC or CMS is too difficult to get
>>>> triggered, because of the requirement for 5 consecutive full GCs, which
>>>> is set by this option.
>>>> I think if it were a normal "product" option, there will definitely be
>>>> users setting it.
>>>> I never understand why it is a "develop" option. I think we could
>>>> either remove it,
>>>> or make it an "experimental" option.
>>>> I'm leaning towards not removing it for now, as I'm not sure if 5 is
>>>> still a reasonable
>>>> default value for UseGCOverheadLimit for G1.
>>>> How about we decide whether to keep or remove this option after
>>>> JDK-8212084 (UseGCOverheadLimit for G1) is fixed?
>>>>
>>>> Also for the hsperfdata counter change, I created
>>>> https://bugs.openjdk.java.net/browse/JDK-8217221. I will draft a CSR
>>>> for it later.
>>>>
>>>> -Man
>>>>
>>>