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

Sat Nov 7 16:08:10 UTC 2020

Hi All, will it be considered as a "green light" if no additional feedback
/ comments come during 14 days? Or is there a possibility to block the
progress by ignoring a thread? Please help me to understand how the
backporting process works.
Thank you

On Thu, 29 Oct 2020 at 08:47, Liang Mao <maoliang.ml at alibaba-inc.com> wrote:

> 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
>
>

-- 
Ruslan Synytsky
CEO @ Jelastic Multi-Cloud PaaS <https://jelastic.com/>