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

[Liang Mao]

Hi Aleksey,

Thanks very much for your careful initial review!

> So, for the current action items:
>    a) JDK-8238932 needs to be added, trivially;
Yes. It should be added although trival.


>    b) JDK-6490394 needs to be investigated on footprint impact, possibly
> involving Man Cao / Google;

 JDK-6490394 might increase the memory footprint because introducing the
heap resize in remark phase. But we also need this to shrink the heap for
saving memory. The heap sizing heuristics in 11u has some original issues
that it would unnecessarily expand heap size with a fixed input. Thomas
made a lot of efforts on this later:
https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-June/030090.html
Even with a fixed IR running for specjbb2015 (as -Dspecjbb.controller.preset.ir=4000),
the heap will expand continuously.

So I think that's why JEP346 is valuable in 11u because not only memory footprint
issue in Java is a common sense but there're also some uncessary memory
expansion in G1.

For the potential issue of increasing footprint by default, can we make a very
simple fix that we only do resize in remark with  (G1PeriodicGCInterval != 0):

+if (G1PeriodicGCInterval != 0) {
     _g1h->resize_heap_if_necessary();
+}

So the additional sizing only happen with a specified G1PeriodicGCInterval. 

>    c) JDK-8218880 deserves more attention/testing, possibly involving others;
>

It seems the only real severe issue brought by the JEP. But periodic triggering
GC with GC locks on is not a common scenario. So it is acceptable that it took 3
months to find the bug. And the fix is straight forward  that just don't trigger GC
when GC locks on. After half and one year, there're no other bugs from the
feature. It looks stable for quite a long time (3 releases).

Thanks,
Liang

> -----Original Message-----
> From: Aleksey Shipilev [mailto:shade at redhat.com]
> Sent: 2020年10月22日 20:08
> To: Liang Mao <maoliang.ml at alibaba-inc.com>; jdk-updates-dev <jdk-updates-
> dev at openjdk.java.net>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
> Martijn Verburg <Martijn.Verburg at microsoft.com>; Ruslan Synytsky
> <rs at jelastic.com>
> Subject: Re: [11u] Backport JEP 346 : Promptly Return Unused Committed
> Memory from G1
> 
> 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