RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC

Thu Dec 20 20:18:06 UTC 2018

Thanks Stefan,

I was able to reproduce and fix the issue. 
http://cr.openjdk.java.net/~kkharbas/8211425/webrev.17
http://cr.openjdk.java.net/~kkharbas/8211425/webrev.16_to_17

On my system (2S Skylake server, with 28 cores/56 threads per socket), I do not see new failures when new flag is enabled.

About the fix for latest issue:
When handling evacuation failure, I overlooked the scenario when G1AllocationFailure is for multi-region humongous object.
satisfy_failed_allocation() for multi-region humongous object should also fail when we have non-zero borrowed_regions.
The fix in the webrev solves the problem and the failed test is now successful.

As you suggested, I will add an RFE for adding a better abstraction for CM prev/next bitmap.

Thanks
Kishor

> -----Original Message-----
> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
> Sent: Thursday, December 20, 2018 6:43 AM
> To: Kharbas, Kishor <kishor.kharbas at intel.com>; sangheon.kim at oracle.com
> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; 'hotspot-gc-
> dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev
> runtime' <hotspot-runtime-dev at openjdk.java.net>
> Subject: Re: RFR(M): 8211425: Allocation of old generation of java heap on
> alternate memory devices - G1 GC
> 
> Hi,
> 
> On 2018-12-20 15:21, Stefan Johansson wrote:
> > Hi,
> >
> > On 2018-12-20 08:47, Kharbas, Kishor wrote:
> >> Hi Stefan, Sangheon,
> >>
> >> I have a new webrev -
> >> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.16
> >> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.15_to_16
> > I think the solution to never have the bitmaps uncommitted is
> > acceptable but I would like us to clean it up going forward. Please
> > create a RFE for adding a better abstraction to avoid having to
> > explicitly call commit_and_set_special. Basically we know when we are
> > creating the mapper for the bitmaps that they need to be pinned and we
> > should be able to pass that in as a argument to the constructor.
> >
> > Otherwise I think this looks ok.
> >
> >>
> >> 1. It incorporates comment about FormatBuffer and has some alignment
> >> changes.
> > Thanks.
> >
> >> 2. Provides fix for the tests which fail when feature is turned on.
> >> Comments in heterogeneousHeapRegionManager.cpp:52 and
> >> heterogeneousHeapRegionManager.cpp:472 describe them.
> > I'm still seeing some tests asserting when running with the feature
> > turned on, they all hit the same assert:
> > Crash: Internal Error
> >
> ...heterogeneousHeapRegionManager.cpp...assert(total_regions_committed
> > () <= max_expandable_length()) failed: must be
> >
> > Tests:
> > vmTestbase/vm/gc/concurrent/lp30yp25rp30mr70st0/TestDescription.java
> >
> vmTestbase/vm/gc/concurrent/lp30yp25rp30mr0st300/TestDescription.java
> >
> vmTestbase/vm/gc/compact/Compact_InternedStrings_NonbranchyTree/Te
> stDe
> > scription.java
> >
> > vmTestbase/gc/lock/jniref/jnireflock01/TestDescription.java
> > vmTestbase/gc/lock/jni/jnilock001/TestDescription.java
> > vmTestbase/gc/lock/jni/jnilock003/TestDescription.java
> >
> > I have had a hard time reproducing those locally but managed to get
> > the first to fail by setting it to use 12 threads. This has to be
> > added in the test file, below is a diff I used to make the test fail
> > more
> > frequently:
> > ---
> > diff --git
> >
> a/test/hotspot/jtreg/vmTestbase/vm/gc/concurrent/lp30yp25rp0mr30st300/
> > TestDescription.java
> >
> b/test/hotspot/jtreg/vmTestbase/vm/gc/concurrent/lp30yp25rp0mr30st300
> /
> > TestDescription.java
> >
> > ---
> >
> a/test/hotspot/jtreg/vmTestbase/vm/gc/concurrent/lp30yp25rp0mr30st300/
> > TestDescription.java
> >
> > +++
> >
> b/test/hotspot/jtreg/vmTestbase/vm/gc/concurrent/lp30yp25rp0mr30st300
> /
> > TestDescription.java
> >
> > @@ -41,6 +41,7 @@
> >    *      -mr 30
> >    *      -st 300
> >    *      -ms high
> > + *      -t 12
> >    *      -gp circularList(high)
> >    *      -gp1 nonbranchyTree(low)
> >    */
> > ---
> Realized I pasted the wrong diff, this is the test I'm making fail with the above
> change:
> vmTestbase/vm/gc/concurrent/lp30yp25rp30mr70st0/TestDescription.java
> 
> Cheers,
> Stefan
> >
> > Please look into what the cause for this is.
> >
> > Cheers,
> > Stefan
> >
> >>
> >> I am able to successfully re-run the failed tests. I am re-run the
> >> whole test suite, will provide status in the morning PST.
> >>
> >> Thanks
> >> Kishor
> >>
> >>> -----Original Message-----
> >>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
> >>> Sent: Monday, December 17, 2018 3:06 AM
> >>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> >>> sangheon.kim at oracle.com
> >>> Cc: 'hotspot-gc-dev at openjdk.java.net'
> >>> <hotspot-gc-dev at openjdk.java.net>;
> >>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
> >>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
> >>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java
> >>> heap on alternate memory devices - G1 GC
> >>>
> >>> Hi Kishor,
> >>>
> >>> Thanks for addressing my second to last comment :) Unfortunately the
> >>> current patch does not build. Instead of using a temp string and
> >>> os::snprintf you should use FromatBuffer::append with the format
> >>> string.
> >>> I've uploaded a patch fixing this at:
> >>> http://cr.openjdk.java.net/~sjohanss/8211425/Final-Comment-G1/
> >>>
> >>> Please incorporate this into your changeset.
> >>>
> >>> Thanks,
> >>> Stefan
> >>>
> >>> On 2018-12-15 03:04, Kharbas, Kishor wrote:
> >>>> Thanks for the review,
> >>>>
> >>>> New webrev at -
> >>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.15
> >>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.14_to_15
> >>>>
> >>>> Regards
> >>>> Kishor
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
> >>>>> Sent: Friday, December 14, 2018 2:40 AM
> >>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> >>>>> sangheon.kim at oracle.com
> >>>>> Cc: 'hotspot-gc-dev at openjdk.java.net'
> >>>>> <hotspot-gc-dev at openjdk.java.net>;
> >>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
> >>>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
> >>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java
> >>>>> heap on alternate memory devices - G1 GC
> >>>>>
> >>>>> Hi Kishor,
> >>>>>
> >>>>> I have one (maybe last) comment on this and it is basically the
> >>>>> same thing as I commented on for Parallel. I think you should use
> >>>>> FormatBuffer instead of a raw char buffer in the sizing code.
> >>>>>
> >>>>> Cheers,
> >>>>> Stefan
> >>>>>
> >>>>> On 2018-12-14 08:54, Kharbas, Kishor wrote:
> >>>>>> Hi Sangheon, Stefan,
> >>>>>>
> >>>>>> I have updated webrev at -
> >>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.14/
> >>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.13_to_14
> >>>>>>
> >>>>>> This webrev works on the latest comments and has some changes in
> >>>>>> young
> >>>>> generation sizing logs messages.
> >>>>>> I am re-running jtreg tests with this patch.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Kishor
> >>>>>>