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

Fri Dec 21 00:58:29 UTC 2018

Hi,
I have new webrev at http://cr.openjdk.java.net/~kkharbas/8211425/webrev.17_to_18/
http://cr.openjdk.java.net/~kkharbas/8211425/webrev.18/

This update corrects the typo and add releasing of temporary nv-dimm file.

Tested with tier1 jtreg tests.

Thanks
Kishor

> -----Original Message-----
> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
> Sent: Thursday, December 20, 2018 2:10 PM
> 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 Kishor,
> 
> On 2018-12-20 21:18, Kharbas, Kishor wrote:
> > 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.
> Fix looks good, I will push it through our testing environment for verification.
> 
> Thanks,
> Stefan
> >
> > 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_committe
> >> d
> >>> () <= 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
> >>>>>>>>