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

[Thomas Schatzl]

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)

Thanks,
   Thomas