RFR (S): 8243672: Short term pause time ratio calculation in G1 off

[stefan.johansson at oracle.com]

On 2020-05-19 15:16, Thomas Schatzl wrote:
> Hi Kim, Stefan,
> 
>    thanks for your reviews.
> 
> On 18.05.20 16:39, Kim Barrett wrote:
>>> On May 18, 2020, at 8:42 AM, stefan.johansson at oracle.com wrote:
>>>
>>> Hi Thomas,
>>>
>>> On 2020-05-18 10:45, Thomas Schatzl wrote:
>>>> Hi all,
>>>>    can I have reviews for this change that improves our calculation 
>>>> of the short term pause time ratio calculation in G1?
>>>> Previously we calculated the short term pause time ratio by assuming 
>>>> that the current pause occurred X times in the long term pause time 
>>>> tracking interval spanning X garbage collections.
>>>> That, unfortunately, emphasizes the current gc pause, giving 
>>>> completely off results particularly for pause time increases e.g. 
>>>> during mixed gc (see the comment in the CR for a sample 
>>>> calculation), in turn tending to affect heap sizing negatively in 
>>>> that the heap expansion is far higher than expected.
>>>> (There is no real issue at the moment for too low results as we do 
>>>> not use these calculated time ratios to shrink the heap yet).
>>>> Arguably now the short term gc time ratio will be more spiky now 
>>>> with that change, but short term pause time ratio is not the only 
>>>> metric used for heap expansion, single outliers are not relevant 
>>>> anyway, and there are limits on the per-GC sizing too.
>>>> So overall it mostly works out as before with significant 
>>>> improvements in heap sizing behavior together with JDK-8244603 
>>>> (which this change will be pushed with.)
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8243672
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8243672/webrev/index.html
>>>
>>> Looks good,
>>
>> Looks good to me too.
>>
>>> Just one thing:
>>> src/hotspot/share/gc/g1/g1Policy.cpp
>>> ---
>>> 122   double now = os::elapsedTime();
>>> 123   _analytics->update_recent_gc_times(now, 0.0);
>>>
>>> This is added to G1Policy::init() and two of the three things that 
>>> are update by this call has already been set in the G1Analytics 
>>> constructor. What do you think about adding 
>>> _recent_gc_times_ms->add(0.0) to the constructor instead of this call?
>>
>> +1
>>
> 
> Actually this code is unnecessary and can be removed.
> 
> New webrev:
> http://cr.openjdk.java.net/~tschatzl/8243672/webrev.0_to_1 (diff)
> http://cr.openjdk.java.net/~tschatzl/8243672/webrev.1 (full)
Even better, ship it.

Thanks,
Stefan

> 
> Thanks,
>    Thomas