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

[Thomas Schatzl]

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.

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

- "weird values" are the responsibility of the commandline parsing. :)

Thanks,
  Thomas