[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: RE: RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
From: "Kharbas, Kishor" <kishor.kharbas () intel ! com>
Date: 2018-12-21 21:05:04
Message-ID: F89640DCD01A85489FCBA68183A6A0F3BEC87C74 () ORSMSX116 ! amr ! corp ! intel ! com
[Download RAW message or body]
Hi,
To make the patch commit go smoothly, I removed some missed trailing white spaces.
Since this is a small code style change, I am not generating new webrev id.
I have replaced webrev.18 : http://cr.openjdk.java.net/~kkharbas/8211425/webrev.18
And renamed old webrev.18 to : http://cr.openjdk.java.net/~kkharbas/8211425/webrev.18.old
Thanks
Kishor
> -----Original Message-----
> From: Stefan Johansson [mailto:stefan.johansson@oracle.com]
> Sent: Friday, December 21, 2018 6:12 AM
> To: Kharbas, Kishor <kishor.kharbas@intel.com>; sangheon.kim@oracle.com
> Cc: Viswanathan, Sandhya <sandhya.viswanathan@intel.com>; 'hotspot-gc-
> dev@openjdk.java.net' <hotspot-gc-dev@openjdk.java.net>; 'Hotspot dev
> runtime' <hotspot-runtime-dev@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-21 01:58, Kharbas, Kishor wrote:
> > 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 looks good. I'm also doing some more testing, should be finished around
> lunch your time.
>
> Cheers,
> Stefan
>
> > 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@oracle.com]
> >> Sent: Thursday, December 20, 2018 2:10 PM
> >> To: Kharbas, Kishor <kishor.kharbas@intel.com>;
> >> sangheon.kim@oracle.com
> >> Cc: Viswanathan, Sandhya <sandhya.viswanathan@intel.com>;
> >> 'hotspot-gc- dev@openjdk.java.net' <hotspot-gc-dev@openjdk.java.net>;
> >> 'Hotspot dev runtime' <hotspot-runtime-dev@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@oracle.com]
> >>>> Sent: Thursday, December 20, 2018 6:43 AM
> >>>> To: Kharbas, Kishor <kishor.kharbas@intel.com>;
> >>>> sangheon.kim@oracle.com
> >>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan@intel.com>;
> >>>> 'hotspot-gc- dev@openjdk.java.net'
> >>>> <hotspot-gc-dev@openjdk.java.net>;
> >>>> 'Hotspot dev runtime' <hotspot-runtime-dev@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@oracle.com]
> >>>>>>> Sent: Monday, December 17, 2018 3:06 AM
> >>>>>>> To: Kharbas, Kishor <kishor.kharbas@intel.com>;
> >>>>>>> sangheon.kim@oracle.com
> >>>>>>> Cc: 'hotspot-gc-dev@openjdk.java.net'
> >>>>>>> <hotspot-gc-dev@openjdk.java.net>;
> >>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev@openjdk.java.net>;
> >>>>>>> Viswanathan, Sandhya <sandhya.viswanathan@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@oracle.com]
> >>>>>>>>> Sent: Friday, December 14, 2018 2:40 AM
> >>>>>>>>> To: Kharbas, Kishor <kishor.kharbas@intel.com>;
> >>>>>>>>> sangheon.kim@oracle.com
> >>>>>>>>> Cc: 'hotspot-gc-dev@openjdk.java.net'
> >>>>>>>>> <hotspot-gc-dev@openjdk.java.net>;
> >>>>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev@openjdk.java.net>;
> >>>>>>>>> Viswanathan, Sandhya <sandhya.viswanathan@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
> >>>>>>>>>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic