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

[Jiangli Zhou]

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
>>>
>>