[11u] Backport JEP 346 : Promptly Return Unused Committed Memory from G1

[Aleksey Shipilev]

On 9/25/20 8:42 AM, Liang Mao wrote:
> All changes of the JEP only contains of hunderds lines of code:
> http://cr.openjdk.java.net/~lmao/jep346.11u/all.webrev.00/

Andrew asked me to follow up on this from the GC implementation/risk perspective.

The first cursory review follows.

This issue seems to be missing from the batch:
   JDK-8238932: Invalid tier1_gc_1 test group definition

A philosophical point: the obvious risk for this kind of backport is breaking something. Since G1 is 
the default garbage collector in 11u, breaking it would break most applications that run with out of 
the box settings. This can be partially mitigated if we had the kill-switch for the code, and that 
depends on how code is structured.

Per-issue comments:

> 1) JDK-8071913: Filter out entries to free/uncommitted regions during iteration
> http://cr.openjdk.java.net/~lmao/jep346.11u/8071913.webrev.00/

This looks like a faithful reproduction of original change. This thing concerns me the most: it 
touches the RSet code without any recourse in case it is broken.

> 2) JDK-6490394: G1: Allow heap shrinking / memory unmapping after reclaiming regions during Remark

Man Cao reports in the comments there that Google's internal JDK 11 had regressed memory footprint 
after this change. I cannot see if there was any follow-up on that.

> 3) JDK-8213898: CDS dumping of springboot asserts in G1ArchiveAllocator::alloc_new_region

This seems to be a simple fix for another regression introduced by JDK-6490394. Looks fine.

> 4) JDK-8212657: Implementation of JDK-8204089 Promptly Return Unused Committed Memory from G1
> http://cr.openjdk.java.net/~lmao/jep346.11u/8212657.webrev.00/

This looks like a faithful reproduction of the original change. It seems fairly innocious, and 
codepaths revert to nops with -XX:G1PeriodicGCInterval=0 and -XX:G1PeriodicGCSystemLoadThreshold=0.

update_rs_lengths_prediction() and update_ihop_prediction() are called in the different order now, 
but that seems fine, as they look independent.

As said above, JDK-8238932 is missing as the follow-up.

> 5) JDK-8215120: 32-bit build failures after JDK-8212657 (Promptly Return Unused Committed Memory from G1)

Simple follow up after JDK-8212657. Looks fine.

> 6) JDK-8215149: TestOptionsWithRangesDynamic.java fails after JDK-8215120

...and another one. Looks fine.

> 7) JDK-8215548: G1PeriodicGCSystemLoadThreshold needs to be a double

Simple follow up for JDK-8212657. Looks fine.

> 8) JDK-8216490: Spammy periodic GC log message contains random time stamp with periodic gc disabled

UX fix after JDK-8212657. Looks fine.

> 9) JDK-8218880: G1 crashes when issuing a periodic GC while the GCLocker is held
> http://cr.openjdk.java.net/~lmao/jep346.11u/8218880.webrev.00/

This looks like a faithful reproduction of the original change.

What concerns me, though, is that the bug was found full 3 months after introducing! And it crashed 
the VM hard, because periodic GC entered collect() from a non-Java thread! I would need some time to 
breathe out and prove to myself it is safe now.

> 10) JDK-8212883: Setting a double manageable flag with jcmd/jinfo crashes the JVM

This one can actually be backported ahead of time to 11u, I think. I understand it is needed for 
JDK-8215548 in this queue, but I don't think that's a blocker for a separate inclusion, and it could 
be useful for other backports.

So, for the current action items:
   a) JDK-8238932 needs to be added, trivially;
   b) JDK-6490394 needs to be investigated on footprint impact, possibly involving Man Cao / Google;
   c) JDK-8218880 deserves more attention/testing, possibly involving others;

Thanks,
-Aleksey