RFR (M): 8212657: Implementation of JDK-8204089 Timely Reduce Unused Committed Memory

[Thomas Schatzl]

Hi,

On Mon, 2018-12-17 at 21:44 +0100, Ruslan Synytsky wrote:
> Hi, a quick test results of build 24. The option
> XX:G1PeriodicGCInterval works fine in our containers.  
>  
> [root at node193618-jdk12 ~]# java -XX:G1PeriodicGCInterval=3000 -jar
> app.jar 1
> 07:32:05 -> Init: 32M Used: 1M Committed: 32M Max: 3276M 
> 
> 07:32:08 -> Init: 32M Used: 800M Committed: 1389M Max: 3276M 
> total time = 3357, ms
> done
> 07:32:11 -> Init: 32M Used: 1079M Committed: 1546M Max: 3276M 
> 07:32:14 -> Init: 32M Used: 1080M Committed: 1546M Max: 3276M 
> 07:32:17 -> Init: 32M Used: 1081M Committed: 1546M Max: 3276M 
> 07:32:20 -> Init: 32M Used: 15M Committed: 32M Max: 3276M 
> 07:32:23 -> Init: 32M Used: 15M Committed: 32M Max: 3276M 
> 07:32:26 -> Init: 32M Used: 13M Committed: 32M Max: 3276M 
> 
> It's easy to repeat the test - just download app.jar and run 
> java -XX:G1PeriodicGCInterval=3000 -jar app.jar 1 
> + one more <Enter> in a second 

Great!

> 
> There are couple small improvement suggestions related to the
> description at JEP 346 
> 
> G1 triggers a this periodic garbage collection if both: 
> 1) More than G1PeriodicGCInterval milliseconds have passed since any
> previous garbage collection pause...
> 2) The average one-minute system load value as returned by the
> getloadavg() call on the host system is above
> G1PeriodicGCSystemLoadThreshold. This condition is ignored if
> G1PeriodicGCSystemLoadThreshold is zero.
> 
> The description of loadavg threshold can be improved in the following
> way:   
> 
> 2) The average one-minute system load value as returned by the
> getloadavg() call on the JVM host system (VM or container) is below
> G1PeriodicGCSystemLoadThreshold. This condition is ignored if
> G1PeriodicGCSystemLoadThreshold is zero.

Fixed.

> I believe the red is a technical mistake in the description, the
> source code comment is quite confusing too, but at least it sounds
> correct:
> 
> Maximum recent system wide system load as returned by the 1m value of
> getloadavg() at which G1 triggers a periodic GC. A load above this
> value cancels a given periodic GC.  

I will provide some (hopefully) improvements along the ms/s issue
for G1PeriodicGCInterval changes.

> 
> Also the blue addition provides more clarity on the host system.   
> 
> One more minor suggestion - G1PeriodicGCInterval should be defined in
> milliseconds at the moment which seems impractical to me. Not clear
> why somebody would need to trigger GC less than in minutes, so people
> will always write a lot of 000. Can we update it to seconds at least?

You can use SI prefixes like "k" or "M" (probably do not need "G" :) )
to reduce the number of zeros.

Not really hung up on the scale, I am going to ask for more opinions.
We need to come to a conclusion relatively quickly though as we are
late in the release process already.

I think the reason for choosing ms is because so far all times were in
ms, and Shenandoah also uses that scale. Otoh J9 uses seconds too.

Thanks,
  Thomas