[12] RFR (XS): 8215548: G1PeriodicGCSystemLoadThreshold needs to be a double

[sangheon.kim at oracle.com]

Hi Thomas,

On 12/20/18 5:18 AM, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2018-12-19 at 13:16 -0800, sangheon.kim at oracle.com wrote:
>> Hi Thomas,
>>
>> On 12/19/18 6:58 AM, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Wed, 2018-12-19 at 10:37 +0100, Thomas Schatzl wrote:
>>>> Hi all,
>>>>
>>>>     can I have reviews for this small change that changes the
>>>> G1PeriodicGCSystemLoadThreshold value type from an uintx to a
>>>> double?
>>>>
>>>> (Manual) Testing by Ruslan from jElastic showed that integers are
>>>> too coarse for this value, as typical load values you want to
>>>> compare with are ~0.5.
>>>>
>>>> Unfortunately this has not been discovered during review; also
>>>> testing against a particular load value provided by the OS is
>>>> hard, so there has not been a regression test for this particular
>>>> flag where this probably would have been found out. :(
>>>     there has been a question about the comparison against 0.0 in
>>> g1YoungRemSetSamplingThread.cpp:75, whether this will catch all
>>> cases.
>>>
>>> Any negative values will be blocked already by the "range"
>>> specifier, but there might be unintentional issues with -0.0. I
>>> changed the comparison to "> 0.0" to exclude it in any case.
>>>
>>> I updated the webrev in place.
>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8215548
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8215548/webrev/
>> Looks good as is.
> Thanks for your review.
>
>> Just minor things to consider would be:
>> 1. The 'range' of G1PeriodicGCSystemLoadThreshold can be much
>> smaller than 'max_uintx' as nobody will expect the periodic gc under
>> heavy loaded situation. I had same question when this feature was
>> being reviewed but I didn't have any suggestion. So as is? :)
> As is.
OK.

>
>> 2. Since this patch is changing to 'double type' comparison,
>> comparison with epsilon would be needed(strictly saying). But
>> probably fine in this limited case? Unless
>> G1PeriodicGCSystemLoadThreshold is set with really
>> weird value and complain the periodic gc. IIRC, commandline options
>> processing has a buffer limit but wrong comparison still would
>> happen within the buffer limit.
> - the value we compare against is exactly representable. Even then it
> would not really matter. I doubt that any digits more than maybe two
> after the decimal point of the value returned by load_avg() are that
> meaningful in the first place. The same for the input value - a few
> digits are probably reasonable, but the load difference between e.g.
> 0.5551 and 0.5552 is probably negligible for this application.
Right.
After two decimal points would be negligible in this case, so agreed 
your patch is good.
Not sure for other platforms but Linux returns 2 decimal points from 
os::loadavg().
I thought it would be good to check now.

>
> - "weird values" are the responsibility of the commandline parsing. :)
Actually command-line parsing is doing its job. What I meant 'weird 
values' was something like 
G1PeriodicGCSystemLoadThreshold=0.17000000000000001 vs. recent_load=0.17. :)
But again negligible.

Ship it!

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>
>