RFR (M): 8212657: Implementation of JDK-8204089 Promptly Return Unused Committed Memory from G1 [Was: RFR (M): 8212657: Implementation of JDK-8204089 Timely Reduce Unused Committed Memory]

[Stefan Johansson]

Hi Thomas,

Just a few comments on the current patch.

On 2018-11-12 10:39, Thomas Schatzl wrote:
> Hi all,
> 
>    over the weekend this JEP has been made candidate; there has been a
> name change of the feature to "Promptly Return Unused Committed Memory
> from G1", no other changes.
> 
> -------- Forwarded Message --------
> From: Thomas Schatzl <thomas.schatzl at oracle.com>
> To: hotspot-gc-dev at openjdk.java.net
> Subject: RFR (M): 8212657: Implementation of JDK-8204089 Timely Reduce
> Unused Committed Memory
> Date: Fri, 09 Nov 2018 12:02:17 +0100
> 
> Hi all,
> 
>    please review the implementation for the JEP candidate[0] registered
> under JDK-8204089: Promptly Return Unused Committed Memory from G1[1].
> 
> This change introduces a periodic gc that kicks in when the VM has been
> idle for some time. This will result in G1 reducing memory footprint,
> both native and Java heap memory, if possible in a "timely" fashion.
> 
> Idle detection is currently based on GC activity, and optionally based
> on system load (default: disabled). Future improvements may add further
> conditions as needed.
> 
> This periodic gc may be either a continuation of any current concurrent
> cycle, optionally a full gc, or just disabled if you want maximum
> throughput.
> 
> Continuation of current gc cycle means that every time the conditions
> for such a periodic gc are met, G1 advances its current state, not
> throwing away any recent effort. E.g. assume that the VM gets idle just
> after marking completed; now a periodic gc will not throw away these
> results, but start the mixed phase, and do mixed collections the next
> few times this periodic gc is triggered, compacting the heap in the
> process automatically. After that mixed phase it might do another
> concurrent mark again, that releases any additional, just emptied
> regions.
> 
> The optional issuance of a full GC during periodic gc may be more
> efficient in returning more memory, at the cost of such a gc.
> 
> For more information, I added some example log under the "gc,periodic"
> tags see the JEP [1]. In addition to that, such GCs will always have a
> cause of "G1 Periodic Collection" to distinguish them from others
> easily.
> 
> Since this change adds some command line flag there is a CSR too[2],
> which gives a shorter summary of the options than in the JEP.
> 
> (CC'ing Kirk P. since he was kind of wary of the entire method in early
> discussions, but I think the functionality turned out the way he would
> like it with default settings).
> 
> The initial implementation and ideas have been contributed by Rodrigo
> Bruno from University of Lisbon and Ruslan Synytsky from JElastic with
> lots of additional input from the community.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8212657
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8212657/webrev/index.html

src/hotspot/share/gc/g1/g1YoungRemSetSamplingThread.cpp
   67     log_debug(gc, periodic)("Last GC occurred " UINTX_FORMAT "ms 
earlier which is higher than threshold " UINTX_FORMAT "ms. Skipping.",
   68                             time_since_last_gc, G1PeriodicGCInterval);

I might be reading this wrong, but I would like the log to say something 
like "Last GC occured X ms ago which is below periodic threshold Y ms, 
Skipping.".

   96     if ((os::elapsedTime() - _last_periodic_gc_attempt_s) > 
(G1PeriodicGCInterval / 1000.0)) {
   97       log_debug(gc, periodic)("Checking for periodic GC.");
   98       if (should_start_periodic_gc()) {
   99         Universe::heap()->collect(GCCause::_g1_periodic_collection);
  100       }
  101       _last_periodic_gc_attempt_s = os::elapsedTime();
  102     }

Move this into a separate method like check_for_periodic_gc().
---

Otherwise I think this looks good.

Thanks,
Stefan

> Testing:
> hs-tier1-5, jdk-tier1-3
> 
> Thanks,
>    Thomas
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8204089
> 
> [2] https://bugs.openjdk.java.net/browse/JDK-8212658
> 
> 
> 
> 
>