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

Fri Jan 15 08:48:08 UTC 2021

Hi Thomas,

Aleksey and Andrew are generally ok with the patches. For the potential performance
issue(memory footprint) in G1 default behavior, I made a small change to resize heap
 in remark pause only when G1PeriodicGCInterval  is not 0. So that the default behavior in 11u will not
change after the feature is merged.

Could you please provide some comments on this?

Thanks,
Liang

------------------------------------------------------------------
From:MAO, Liang <maoliang.ml at alibaba-inc.com>
Send Time:2020 Oct. 29 (Thu.) 14:47
To:Aleksey Shipilev <shade at redhat.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

Hi All,

For the potential memory footprint issue from JDK-6490394,I made more tests.
Fixed IR test on specjbb2015 shows that memory footprint will climb 
more aggressively than original 11u. That could be an issue but it also exists
in JDK12 to JDK15 for 4 releases. 

(I use following options to test:
-Dspecjbb.controller.type=PRESET -Dspecjbb.controller.preset.ir=4000
-Dspecjbb.controller.preset.duration=1000000  -XX:ParallelGCThreads=8
-XX:+UseG1GC -Xmx10g -Xms1g -jar specjbb2015.jar -m COMPOSITE)

As I suggested in previous mail that we can do resizing in remark only with 
(G1PeriodicGCInterval != 0) :

http://cr.openjdk.java.net/~lmao/jep346.11u/all.webrev.01/

The diff with all.webrev.00 only contains:

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

The tier1 is clean with fastdebug and the above specjbb2015 test won't have
memory footprint issues anymore. So looks like most of impacts will be excluded
from default G1. Could someone please take a look?

Thanks,
Liang

> -----Original Message-----
> From: Liang Mao [mailto:maoliang.ml at alibaba-inc.com]
> Sent: 2020年10月26日 10:19
> To: 'Aleksey Shipilev' <shade at redhat.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
> 
> 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