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

[sangheon.kim at oracle.com]

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.

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? :)
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.

Thanks,
Sangheon

>> Testing:
>> Manual testing with a hacked gc/g1/TestPeriodicCollection.java test,
>> and the command line range checking tests.
>> hs-tier1-5
> Thanks,
>    Thomas
>
>