RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Kharbas, Kishor
kishor.kharbas at intel.com
Fri Dec 14 07:54:00 UTC 2018
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
> -----Original Message-----
> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> Sent: Thursday, December 13, 2018 2:31 PM
> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
> <stefan.johansson 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,
>
> On 12/13/18 2:14 AM, Kharbas, Kishor wrote:
> > Missed one change earlier -
> > http://cr.openjdk.java.net/~kkharbas/8211425/webrev.13/
> > http://cr.openjdk.java.net/~kkharbas/8211425/webrev.12_to_13/
> It was a bit confusing to review webrev.11, 12 and 13.
> Here are some comments.
>
>
> -----------------------------------------------
> src/hotspot/share/gc/g1/g1Policy.cpp
> 86 delete _ihop_control;
> - Need to add _young_gen_sizer as well here?
>
> 188 printf("Young gen sizer is %p\n", _young_gen_sizer);
> - Remove this line.
>
>
> -----------------------------------------------
> src/hotspot/share/gc/g1/g1YoungGenSizer.cpp
> - Copyright update
>
> 133 G1YoungGenSizer*
> G1YoungGenSizer::create_gen_sizer(G1CollectorPolicy* policy) {
> - I think you wanted to initialize _young_gen_sizer at initialization
> list and use create_gen_sizer() method. You can do that if you pass
> G1CollectorPolicy* when constructing G1Policy.
>
> G1Policy::G1Policy(G1CollectorPolicy* policy, STWGCTimer* gc_timer) :
> ...
> _young_gen_sizer(G1YoungGenSizer::create_gen_sizer(policy)),
> ...
>
> G1Policy* G1Policy::create_policy(G1CollectorPolicy* policy, STWGCTimer*
> gc_timer_stw) {
> if (policy->is_hetero_heap()) {
> return new G1HeterogeneousHeapPolicy(policy, gc_timer_stw);
> } else {
> return new G1Policy(policy, gc_timer_stw);
> }
> }
>
>
> -----------------------------------------------
> src/hotspot/share/gc/g1/g1HeterogeneousCollectorPolicy.cpp
> 38 log_info(gc, ergo)("Setting MaxNewSize to " SIZE_FORMAT "
> which is 80%% of dram memory. Dram usage can be lowered by setting
> MaxNewSize to a lower value", (size_t)reasonable_max);
> - If you want to avoid hard-coded value of '80': "which is " SIZE_FORMAT
> "%%" ... , MaxRamFractionForYoung * 100"
>
>
> -----------------------------------------------
> test/hotspot/jtreg/TEST.groups
> -gc/{nvdimm}
> - Remove '{}'.
> - FYI, test groups will still work but will not exclude 'nvdimm' folder
> because of parenthesis. I didn't mean to use 'gc/{nvdimm}', it was just
> an example. :)
>
>
> Thanks,
> Sangheon
>
>
> >
> > Thanks
> > Kishor
> >
> >> -----Original Message-----
> >> From: Kharbas, Kishor
> >> Sent: Thursday, December 13, 2018 1:14 AM
> >> To: 'sangheon.kim at oracle.com' <sangheon.kim at oracle.com>; 'Stefan
> >> Johansson' <stefan.johansson 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>; Kharbas,
> Kishor
> >> <kishor.kharbas at intel.com>
> >> Subject: RE: RFR(M): 8211425: Allocation of old generation of java heap on
> >> alternate memory devices - G1 GC
> >>
> >> New webrev : http://cr.openjdk.java.net/~kkharbas/8211425/webrev.12/
> >> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.11_to_12/
> >>
> >> Some compilation errors were introduced in earlier webrev, fixed those
> and
> >> some cosmetic changes.
> >>
> >> Thanks
> >> Kishor
> >>
> >>> -----Original Message-----
> >>> From: Kharbas, Kishor
> >>> Sent: Wednesday, December 12, 2018 6:45 PM
> >>> To: 'sangheon.kim at oracle.com' <sangheon.kim at oracle.com>; 'Stefan
> >>> Johansson' <stefan.johansson 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>; Kharbas,
> >> Kishor
> >>> <kishor.kharbas at intel.com>
> >>> Subject: RE: RFR(M): 8211425: Allocation of old generation of java heap
> on
> >>> alternate memory devices - G1 GC
> >>>
> >>> Hi Sangheon,
> >>> Here is the complete re-based patch
> >>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.11/
> >>>
> >>>> -----Original Message-----
> >>>> From: Kharbas, Kishor
> >>>> Sent: Wednesday, December 12, 2018 6:07 PM
> >>>> To: 'sangheon.kim at oracle.com' <sangheon.kim at oracle.com>; Stefan
> >>>> Johansson <stefan.johansson 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>; Kharbas,
> >>> Kishor
> >>>> <kishor.kharbas at intel.com>
> >>>> Subject: RE: RFR(M): 8211425: Allocation of old generation of java heap
> >> on
> >>>> alternate memory devices - G1 GC
> >>>>
> >>>> Hi Sangheon,
> >>>> New incremental webrev and comments inline -
> >>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.10_to_11/
> >>>> Will re-base the complete webrev can send it soon.
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> >>>>> Sent: Wednesday, December 12, 2018 4:22 PM
> >>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
> >>>>> <stefan.johansson 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,
> >>>>>
> >>>>> On 12/12/18 12:49 AM, Kharbas, Kishor wrote:
> >>>>>> Hi Sangheon,
> >>>>>> I have worked on your comments in webrev.10. Replying inline just to
> >>>>> formally check all points as done.
> >>>>> Thanks for addressing all my comments.
> >>>>> Here are some minor comment. As this review thread is going to the
> >> end,
> >>>>> I tried to review full webrev.
> >>>>>
> >>>>> - webrev.10 conflicts with the latest source code.
> >>>>>
> >>>>> ----------------------------------------------------------
> >>>>> src/hotspot/share/gc/g1/jvmFlagConstraintsG1.cpp
> >>>>> - My previous comment was based on my bad assumption about your
> >>>> logic,
> >>>>> sorry. I was thinking you need fast exit path if
> >>>>> NewSize/MaxNewSize/(whatever options related to young gen size)
> are
> >>>>> larger than physical RAM. So I recommended to add it on constraint
> >>>>> function.
> >>>>> If we need ergonomic value, definitely have to add on other place. e.g.
> >>>>> collector policy. Ergonomically choosing value behavior seems better
> >>>>> than exiting unless given options are conflicting.
> >>>>> - Similar work done at Parallel GC patch(8211424) needs here too.
> >>>> [Kharbas, Kishor] Yes I have G1HeterogeneousCollectorPolicy and
> >>>> G1HeterogeneousYoungGenSizer to do similar checking as 8211424.
> >>>>
> >>>>> - Could you do some tests with young gen related flags added? I will
> >>>>> try too.
> >>>> [Kharbas, Kishor] I did some testing. Will do more.
> >>>>
> >>>>>
> >>>>> ----------------------------------------------------------
> >>>>> src/hotspot/share/prims/whitebox.cpp
> >>>>> 524 return (jlong)g1h->base() +
> >>>>> Universe::heap()->collector_policy()->max_heap_byte_size();
> >>>>> - Use g1h instead of Universe::heap().
> >>>>>
> >>>>>
> >>>>> ----------------------------------------------------------
> >>>>> 2571 experimental(ccstr, AllocateOldGenAt,
> >>>>> NULL, \
> >>>>> 2572 "Path to the directoy where a temporary file will be
> >>>>> " \
> >>>>> 2573 "created to use as the backing store for old
> >>>>> generation") \
> >>>>> - We discussed to add somewhere about 'a temporary file will be
> >> created
> >>>>> for the size of Xmx'. Are you planning to add it to release note?
> >>>>> For the record, this is slightly different than AllocateHeapAt which
> >>>>> creates Xmx sized file for whole Java Heap and use no RAM for java
> >> heap.
> >>>>> This feature is only allocating old gen portion on nvdimm but the
> >>>>> temporal file will be created size of Xmx for performance reason. But
> >>>>> there is a logic to manage not to commit more than Xmx for both
> >>>> young/old.
> >>>> [Kharbas, Kishor] I added some explanation in globals.hpp.
> >>>>
> >>>> Thanks,
> >>>> Kishor
> >>>>> ----------------------------------------------------------
> >>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.cpp
> >>>>> 26 #include "gc/g1/g1HeterogeneousHeapPolicy.hpp"
> >>>>> 27 #include "gc/g1/g1CollectedHeap.hpp"
> >>>>> - Wrong order for line 26 and 27.
> >>>>>
> >>>>>
> >>>>> ----------------------------------------------------------
> >>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.cpp
> >>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.hpp
> >>>>> - Need 1 more space at line 2 ~ 23.
> >>>>>
> >>>>>
> >>>>> ----------------------------------------------------------
> >>>>>
> test/hotspot/jtreg/gc/nvdimm/TestHumongousObjectsOnNvdimm.java
> >>>>> test/hotspot/jtreg/gc/nvdimm/TestOldObjectsOnNvdimm.java
> >>>>> test/hotspot/jtreg/gc/nvdimm/TestYoungObjectsOnDram.java
> >>>>> - You are missing ',' between 2018 and Oracle at line 2.
> >>>>>
> >>>>>
> >>>>> Thanks,
> >>>>> Sangheon
> >>>>>
> >>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: sangheon.kim at oracle.com
> >> [mailto:sangheon.kim at oracle.com]
> >>>>>>> Sent: Tuesday, December 11, 2018 2:18 PM
> >>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
> >>>>>>> <stefan.johansson 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,
> >>>>>>>
> >>>>>>> On 12/11/18 12:30 AM, Kharbas, Kishor wrote:
> >>>>>>>> Hi Stefan, Sangheon,
> >>>>>>>>
> >>>>>>>> I have an updated and re-based webrev which triggers a full gc
> >> when
> >>>> we
> >>>>>>> make available extra regions when we are out of old gc allocation
> >>>> regions.
> >>>>>>> This avoids changing code in copy_to_survivor()
> >>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.09/
> >>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.08_to_09/
> >>>>>>> Here are some my comments.
> >>>>>>> - I can't find newly added tests are excluded in this patch.
> >>>>>> [Kharbas, Kishor] Done.
> >>>>>>> - Some checking for not to exceed physical DRAM, Stefan also
> >>>> mentioned
> >>>>>>> this. Playing with NewSizePercentage and MaxNewSizePercentage,
> >> and
> >>>>> their
> >>>>>>> constraint functions as well. Make sure checking time should be
> after
> >>>>>>> ergonomics(AfterErgo).
> >>>>>> [Kharbas, Kishor] Added logic in G1 constraint functions.
> >>>>>>
> >>>>>>> - Test results(both this feature on/off) with suggested test groups.
> >>>>>>> - webrev.09 still contains ParallelGC patch.
> >>>>>>>
> >>>>>> [Kharbas, Kishor] removed in latest webrev.
> >>>>>>> ----------------------------------
> >>>>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.cpp
> >>>>>>> 36 _g1h = g1h;
> >>>>>>> - line 36 is redundant as G1Policy::init() does it. And a member of
> >> _g1h
> >>>>>>> here is also redundant.
> >>>>>>>
> >>>>>> [Kharbas, Kishor] Done.
> >>>>>>
> >>>>>>> 37
> >>>>>>> static_cast<HeterogeneousHeapRegionManager*>(g1h->hrm())-
> >>>>>>>> adjust_dram_regions((uint)young_list_target_length(),
> >>>>>>> _g1h->workers());
> >>>>>>> - You can add HeterogeneousHeapRegionManager* as a member of
> >>> this
> >>>>>>> policy
> >>>>>>> if want to avoid static_cast. All 4 methods defined in this class have
> >>>>>>> static_cast.
> >>>>>>>
> >>>>>> [Kharbas, Kishor] Done.
> >>>>>>> ----------------------------------
> >>>>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.hpp
> >>>>>>> 32 G1CollectedHeap* _g1h;
> >>>>>>> - G1Policy already has G1CollectedHeap* as a member.
> >>>>>>>
> >>>>>> [Kharbas, Kishor] Done.
> >>>>>>> Thanks,
> >>>>>>> Sangheon
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> I am running exhaustive jtreg tests, will report the results
> >> tomorrow.
> >>>>>>>> Thanks
> >>>>>>>> Kishor
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Kharbas, Kishor
> >>>>>>>>> Sent: Friday, December 7, 2018 11:24 AM
> >>>>>>>>> To: Stefan Johansson <stefan.johansson at oracle.com>;
> >>>>>>>>> 'sangheon.kim at oracle.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>;
> >>> Kharbas,
> >>>>>>> Kishor
> >>>>>>>>> <kishor.kharbas at intel.com>
> >>>>>>>>> Subject: RE: RFR(M): 8211425: Allocation of old generation of java
> >>>> heap
> >>>>> on
> >>>>>>>>> alternate memory devices - G1 GC
> >>>>>>>>>
> >>>>>>>>> Hi Stefan,
> >>>>>>>>> I used these commands to run jtreg. Thanks for starting the tests
> >> on
> >>>>> your
> >>>>>>>>> end.
> >>>>>>>>>
> >>>>>>>>> jtreg -jdk:/home/kishor/jdk-tip-nvdimm -retain:fail -conc:2 -
> >>>>>>>>> w:./JT_nvdimm_WithoutFlag -r:./JT_nvdimm_WithoutFlag
> >>>>>>>>> .:tier1_old_nvdimm
> >>>>>>>>>
> >>>>>>>>> jtreg -jdk:/home/kishor/jdk-tip-nvdimm -retain:fail -conc:2 -
> >>>>> javaoptions:"-
> >>>>>>>>> XX:+UnlockExperimentalVMOptions -XX:+UseG1GC -
> >>>>>>>>> XX:AllocateOldGenAt=/home/kishor" -
> >> w:./JT_nvdimm_WithoutFlag -
> >>>>>>>>> r:./JT_nvdimm_WithoutFlag .:tier1_old_nvdimm
> >>>>>>>>>
> >>>>>>>>> tier1_old_nvdimm = \
> >>>>>>>>> serviceability/ \
> >>>>>>>>> :tier1_gc \
> >>>>>>>>> :tier1_runtime \
> >>>>>>>>> :hotspot_appcds \
> >>>>>>>>> -gc/serial \
> >>>>>>>>> -gc/cms \
> >>>>>>>>> -gc/epsilon \
> >>>>>>>>> -gc/TestAllocateHeapAt.java \
> >>>>>>>>> -gc/TestAllocateHeapAtError.java \
> >>>>>>>>> -gc/TestAllocateHeapAtMultiple.java \
> >>>>>>>>> -gc/g1/TestFromCardCacheIndex.java \
> >>>>>>>>> -gc/metaspace/TestMetaspacePerfCounters.java \
> >>>>>>>>> -gc/metaspace/TestPerfCountersAndMemoryPools.java \
> >>>>>>>>> -runtime/Metaspace/PrintMetaspaceDcmd.java
> >>>>>>>>> -gc/g1/TestLargePageUseForAuxMemory.java
> >>>>>>>>>
> >>>>>>>>> Thanks,
> >>>>>>>>> Kishor
> >>>>>>>>>
> >>>>>>>>>> -----Original Message-----
> >>>>>>>>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
> >>>>>>>>>> Sent: Friday, December 7, 2018 6:14 AM
> >>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> >>>>>>>>>> 'sangheon.kim at oracle.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,
> >>>>>>>>>>
> >>>>>>>>>> On 2018-12-07 08:34, Kharbas, Kishor wrote:
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> JTreg regression testing shows 1 regression when we use
> >> patched
> >>>> JDK
> >>>>>>>>>> without flag
> >>>>>>>>>>> - runtime/appcds/cacheObject/ArchivedIntegerCacheTest.java
> >>>>>>>>>>>
> >>>>>>>>>>> I looked at the log, but don't seem to have a handle on this.
> Any
> >>>>>>> pointers
> >>>>>>>>>> on how appCDS is affected by the patch? Error is,
> >>>>>>>>>>> Exception in thread "main" java.lang.RuntimeException:
> >>>> FAILED.
> >>>>>>> Value
> >>>>>>>>>> expected to be archived: -128 of type java.lang.Byte
> >>>>>>>>>>> at
> >>
> CheckIntegerCacheApp.checkArchivedAsExpected(CheckIntegerCacheApp.jav
> >>>>>>>>>> a:109)
> >>>>>>>>>>> at
> >>>>> CheckIntegerCacheApp.main(CheckIntegerCacheApp.java:73)
> >>>>>>>>>>> ]
> >>>>>>>>>>> exitValue = 1
> >>>>>>>>>> I can't reproduce this failure locally, do you run with any extra
> >> flags
> >>>>>>>>>> or how exactly are you running the test? I don't see how AppCDS
> >> is
> >>>>>>>>>> affected when the feature is turned of, when turned on AppCDS
> >>> will
> >>>>> not
> >>>>>>>>>> be used since CompressedOops are not used.
> >>>>>>>>>>
> >>>>>>>>>> I've started some benchmarking run with the feature turned of
> as
> >>>> well
> >>>>> to
> >>>>>>>>>> make sure we don't see any regression in normal G1.
> >>>>>>>>>>
> >>>>>>>>>>> And 3 regressions when additional flag - AllocateOldGenAt is
> >>>>> specified. I
> >>>>>>>>>> have an idea what is going on here.
> >>>>>>>>>>> +gc/TestSoftReferencesBehaviorOnOOME.java
> >>>>>>>>>>> +gc/g1/TestGreyReclaimedHumongousObjects.java
> >>>>>>>>>>> +gc/g1/humongousObjects/TestNoAllocationsInHRegions.java
> >>>>>>>>>>>
> >>>>>>>>>> These should be addressed before the changes can be pushed. I
> >>> will
> >>>>> try
> >>>>>>>>>> to start some remote test jobs with the patch enabled over the
> >>>>> weekend.
> >>>>>>>>>> Thanks,
> >>>>>>>>>> Stefan
> >>>>>>>>>>
> >>>>>>>>>>> Thanks
> >>>>>>>>>>> Kishor
> >>>>>>>>>>>
> >>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>> From: Kharbas, Kishor
> >>>>>>>>>>>> Sent: Thursday, December 6, 2018 5:24 PM
> >>>>>>>>>>>> To: Stefan Johansson <stefan.johansson at oracle.com>;
> >>>>>>>>>>>> 'sangheon.kim at oracle.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>;
> >>>>> Kharbas,
> >>>>>>>>>> Kishor
> >>>>>>>>>>>> <kishor.kharbas at intel.com>
> >>>>>>>>>>>> Subject: RE: RFR(M): 8211425: Allocation of old generation of
> >>> java
> >>>>>>> heap
> >>>>>>>>> on
> >>>>>>>>>>>> alternate memory devices - G1 GC
> >>>>>>>>>>>>
> >>>>>>>>>>>> Hi Stefan, Sangheon,
> >>>>>>>>>>>>
> >>>>>>>>>>>> My replies are inline.
> >>>>>>>>>>>> Updated webrev -
> >>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.08/
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.07_to_08
> >>>>>>>>>>>> Thank you.
> >>>>>>>>>>>> Kishor
> >>>>>>>>>>>>
> >>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>> From: Stefan Johansson
> >> [mailto:stefan.johansson at oracle.com]
> >>>>>>>>>>>>> Sent: Thursday, December 6, 2018 6:03 AM
> >>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> >>>>>>>>>>>>> 'sangheon.kim at oracle.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,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On 2018-12-06 10:05, Kharbas, Kishor wrote:
> >>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I have new webrev (removed Parallel GC part, but couldn't
> >> get
> >>>> rid
> >>>>> of
> >>>>>>>>>> files
> >>>>>>>>>>>>> showing up with 0 lines changed) -
> >>>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.07
> >>>>>>>>>>>>>> Incremental webrev -
> >>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.06_to_07/
> >>>>>>>>>>>>> One new comment here and then the replies to your fixes
> are
> >>>>> below.
> >>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionManager.cpp
> >>>>>>>>>>>>> ---------------------------------------------
> >>>>>>>>>>>>> 71 HeapRegionManager*
> >>>>>>>>>>>>> HeapRegionManager::create_manager(G1CollectedHeap*
> >>> heap,
> >>>>>>>>>>>>> CollectorPolicy* policy) {
> >>>>>>>>>>>>> 72 if (static_cast<G1CollectorPolicy*>(policy)-
> >>>>>> is_hetero_heap()) {
> >>>>>>>>>>>>> 73 return new
> >>>>>>>>>>>>> HeterogeneousHeapRegionManager((uint)(policy-
> >>>>>>>>>>> max_heap_byte_size() /
> >>>>>>>>>>>>> HeapRegion::GrainBytes) /*heap size as num of regions*/);
> >>>>>>>>>>>>> 74 }
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> If you pass in a G1CollectorPolicy you can avoid the cast.
> >>>>>>>>>>>>>
> >>>>>>>>>>>> [Kharbas, Kishor] Thanks.
> >>>>>>>>>>>>>> I am replying to Sangheon's comments inline, and have
> >> copied
> >>>>>>> below
> >>>>>>>>>>>>> Stefan's comments from previous two emails below with my
> >>>>> replies.
> >>
> ================================================================
> >>> =====================================================
> >>>>>>>>>>>>>> Thanks for providing a separate webrev for this.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I'm not sure I understand why we need a special region
> >> type.
> >>>>> What
> >>>>>>> is
> >>>>>>>>>> the
> >>>>>>>>>>>>> problem of having Old regions in DRAM, if we have an
> >>>> evacuation
> >>>>>>>>>> failure?
> >>>>>>>>>>>>>> I understand that it is not what we want with this feature,
> >> but
> >>>> by
> >>>>>>>>>> allowing
> >>>>>>>>>>>>> to go over Xmx during the GC we should avoid most
> >>> occurrences
> >>>> of
> >>>>> it.
> >>>>>>> I
> >>>>>>>>>> can
> >>>>>>>>>>>>> happen during Mixed collections, but if it does we are
> >> probably
> >>>>> close
> >>>>>>> to
> >>>>>>>>>> run
> >>>>>>>>>>>>> into a Full GC.
> >>>>>>>>>>>>>>>> With the newer way (borrowing regions) we do not need
> >>>>> special
> >>>>>>>>>> region
> >>>>>>>>>>>>> type. I removed it from latest webrev.
> >>>>>>>>>>>>> Great, thanks.
> >>>>>>>>>>>>>
> >>
> ================================================================
> >>> =====================================================
> >>>>>>>>>>>>>> Here are my comments on the patch:
> >>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> >>>>>>>>>>>>>> -------------------------------------------
> >>>>>>>>>>>>>> 4558 if (new_alloc_region == NULL && is_hetero_heap()
> >> &&
> >>>>>>>>>> !is_survivor)
> >>>>>>>>>>>> {
> >>>>>>>>>>>>>> 4559 // if it is a heterogenous heap, we can always copy
> >>>> objects
> >>>>>>>>>>>>>> from young generation even if
> >>>>>>>>>>>>>> 4560 // old generation is full. We can borrow (or pre-
> >>> allocate)
> >>>>>>>>>>>>>> regions in nv-dimm to serve as allocation region.
> >>>>>>>>>>>>>> 4561 new_alloc_region =
> >>>>>>>>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(hrm())-
> >>>>>>>>>>>>>> borrow_old_region_for_gc();
> >>>>>>>>>>>>>> 4562 assert(new_alloc_region != NULL, "We should get a
> >>> non-
> >>>>> null
> >>>>>>>>>>>>>> region");
> >>>>>>>>>>>>>> 4563 }
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> This logic should live in
> >>>>>>>>>>>>>>
> HeterogeneousHeapRegionManager::allocate_free_region().
> >>>> This
> >>>>>>>>>>>> function
> >>>>>>>>>>>>> should return NULL only when we are really out of regions
> >> (that
> >>>> is
> >>>>> no
> >>>>>>>>>>>> regions
> >>>>>>>>>>>>> can be borrowed).
> >>>>>>>>>>>>>>>> allocate_free_region() is called by mutators too.
> >>> Humongous
> >>>>> calls
> >>>>>>>>>> this
> >>>>>>>>>>>>> function with 'old' = true. So we won't be able to
> differentiate
> >>>>>>> whether
> >>>>>>>>>> the
> >>>>>>>>>>>>> call is for gc old region allocation or for humongous.
> >>>>>>>>>>>>>>>> Is it a good idea to add a new hrm method to be called
> for
> >>> gc
> >>>>>>>>>> allocation
> >>>>>>>>>>>>> and override it in HeterogeneousHeapRegionManager?
> >>>>>>>>>>>>> I think instead changing G1CollectedHeap::new_region() and
> >>>>>>>>>>>>> HeapRegionManager::allocate_free_region() to take a
> >>>>>>> HeapRegionType
> >>>>>>>>>>>>> instead of a bool is better. Then the different callers can
> >> better
> >>>>> pass
> >>>>>>>>>>>>> in what they really are.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I think the HeapRegionType needs to be extended a bit to
> >>> handle
> >>>>>>> this,
> >>>>>>>>>>>>> I'm thinking something like:
> >>>>>>>>>>>>>
> >>> http://cr.openjdk.java.net/~sjohanss/8211425/HeapRegionType-
> >>>>> for-
> >>>>>>>>>>>>> new_region/
> >>>>>>>>>>>>>
> >>>>>>>>>>>> [Kharbas, Kishor] Thank you. I liked your idea and used it
> for
> >>> the
> >>>>>>>>>>>> implementation. Now there is no call to
> >>>>> borrow_old_region_for_gc()
> >>>>>>>>> from
> >>>>>>>>>>>> outside, allocate_free_region() handles this.
> >>>>>>>>>>>>
> >>
> ================================================================
> >>> =====================================================
> >>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
> >>>>>>>>>>>>>> ------------------------------------------------
> >>>>>>>>>>>>>> 227 // Third clause: if it is a heterogenous heap, we
> >>> borrow
> >>>>>>>>> regions
> >>>>>>>>>>>>>> from nv-dimm for old gc allocation.
> >>>>>>>>>>>>>> 228 // This region can only be used to evacate young
> >>>> objects,
> >>>>> so
> >>>>>>>>> we
> >>>>>>>>>>>>>> check the state of the object being copied.
> >>>>>>>>>>>>>> 229 if (_old_gen_is_full && dest_state.is_old() ||
> >>>>>>>>>>>>>> 230 (_g1h->is_hetero_heap() && state.is_old() &&
> >>>>>>>>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(_g1h-
> >>>>> hrm())-
> >>>>>>>>>>>>>> is_old_full())
> >>>>>>>>>>>>>> ) {
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Instead of doing this extra clause I think we should make
> >> sure
> >>>>>>>>>>>>> _old_gen_is_full is correct even for hetero heaps. I don't
> >> really
> >>>> see
> >>>>>>> why
> >>>>>>>>> it
> >>>>>>>>>>>>> shouldn't be already.
> >>>>>>>>>>>>>>>> 'this->_old_gen_is_full' is set true in
> >> allocate_in_next_plab()
> >>>>>>> which
> >>>>>>>>> is
> >>>>>>>>>>>> the
> >>>>>>>>>>>>> final attempt for allocation in default code path.
> >>>>>>>>>>>>>>>> However when hetero heap is enabled,
> >>>> allocate_in_next_plab()
> >>>>>>>>> may
> >>>>>>>>>>>> not
> >>>>>>>>>>>>> be called since we allocate plab from borrowed region when
> >>> old
> >>>> is
> >>>>>>> full.
> >>>>>>>>>>>>> Ok, I think I see your point now. If "borrowing" regions you
> >>> only
> >>>>> want
> >>>>>>>>>>>>> to use them for young objects. But adding the or clause here
> >>> will
> >>>>>>> mean
> >>>>>>>>>>>>> we will add extra checks for each evacuated object and I
> >> really
> >>>>> want
> >>>>>>> to
> >>>>>>>>>>>>> avoid that, since it will affect normal G1 quite a bit
> >> potentially.
> >>>>>>>>>>>> [Kharbas, Kishor] I am able to avoid this extra check on fast
> >> path
> >>>> by
> >>>>>>>>> setting
> >>>>>>>>>>>> _old_gen_is_full flag after allocate_direct_or_new_plab().
> >>>>>>>>>>>> After allocate_direct_or_new_plab(), we
> >>> check
> >>>>>>>>> whether
> >>>>>>>>>> we
> >>>>>>>>>>>> had to borrow any region for this allocation and set
> >>>> _old_gen_is_full
> >>>>>>>>>>>> accordingly.
> >>>>>>>>>>>> At the top, only when _old_gen_is_full is
> >>> true,
> >>>>> we
> >>>>>>>>> need
> >>>>>>>>>>>> additional check for whether source is young or old.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> An alternative to this case would be to allow borrowing for
> >> all,
> >>>> and
> >>>>> if
> >>>>>>>>>>>>> we after the GC realize we can't get down to Xmx number of
> >>>>> regions
> >>>>>>>>>>>>> trigger a Full GC to solve this.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> When talking about the Full GC, I think there might be
> >> potential
> >>>>>>>>>>>>> problems there. How do you make sure all objects are
> moved
> >>> to
> >>>>> old.
> >>>>>>>>> The
> >>>>>>>>>>>>> normal algorithm move stuff to the bottom of the heap by
> >>>>>>> compacting
> >>>>>>>>>>>>> chains of heap regions and tagging the regions as old after
> >> the
> >>>>>>>>>>>>> collection. For the nv-dimm case I can't see how you would
> >>>> handle
> >>>>>>>>>>>>> regions still in dram to move them into nv-dimm.
> >>>>>>>>>>>>>
> >>>>>>>>>>>> [Kharbas, Kishor] Yes, if we allow borrowing for all and
> >> complete
> >>>>> the
> >>>>>>> GC
> >>>>>>>>>> with
> >>>>>>>>>>>> extra regions, new allocations could increase the live set to
> >> more
> >>>>> than
> >>>>>>>>>> Xmx.
> >>>>>>>>>>>> Even a Full GC would not help.
> >>>>>>>>>>>>
> >>
> ================================================================
> >>> =====================================================
> >>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> >>>>>>>>>>>>>> -------------------------------------------
> >>>>>>>>>>>>>> 1493 _is_hetero_heap(AllocateOldGenAt != NULL),
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Does G1CollectedHeap really need to know this, can't we
> >>> always
> >>>>> just
> >>>>>>>>> as
> >>>>>>>>>>>> the
> >>>>>>>>>>>>> G1CollectorPolicy? As a matter of fact, I think most places
> >>> where
> >>>>> we
> >>>>>>>>>> check
> >>>>>>>>>>>>> is_hetro_heap() a better solution is to use virtual calls or
> >> other
> >>>>>>>>>> abstractions
> >>>>>>>>>>>>> to do the right thing.
> >>>>>>>>>>>>>>>> I moved this to G1CollectorPolicy. I got rid of few places
> >>>> where
> >>>>>>>>>>>>> is_hetero_heap() is needed by using abstractions
> (specialized
> >>>>>>>>> G1Policy).
> >>>>>>>>>>>>> Looks good.
> >>>>>>>>>>>>>
> >>
> ================================================================
> >>> =====================================================
> >>>>>>>>>>>>>> 1786 // Now we know the target length of young list. So
> >>> adjust
> >>>>> the
> >>>>>>>>>>>>>> heap to provision that many regions on dram.
> >>>>>>>>>>>>>> 1787 if (is_hetero_heap()) {
> >>>>>>>>>>>>>> 1788
> >>>>>>>>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(hrm())-
> >>>>>>>>>>>>>> adjust_dram_regions((uint)g1_policy()-
> >>>>>> young_list_target_length(),
> >>>>>>>>>>>>>> workers());
> >>>>>>>>>>>>>> 1789 }
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Here we have an example of what I mentioned above.
> >> Instead
> >>>> of
> >>>>>>>>> having
> >>>>>>>>>>>>> the check and the cast, is there a way we can make this
> >> happen
> >>>>> every
> >>>>>>>>>> time
> >>>>>>>>>>>>> we update the young_list_target_length(). I see that we
> >>> currently
> >>>>>>> only
> >>>>>>>>> do
> >>>>>>>>>>>> this
> >>>>>>>>>>>>> at initialization and after full collections, why don't we have
> >> to
> >>> do
> >>>>> this
> >>>>>>>>>> after
> >>>>>>>>>>>>> each GC?
> >>>>>>>>>>>>>> Same comment for lines 2532-2535.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> young_list_target_length is updated from other places
> >> such
> >>>> as
> >>>>>>>>>>>>> g1YoungRemSetSamplingThread. In which case we have to
> >>>> acquire
> >>>>>>>>> heap
> >>>>>>>>>>>> lock
> >>>>>>>>>>>>> or check whether it’s the VMThread and lock is owned.
> >>>>>>>>>>>>>>>> I had adjust_dram_regions() call here earlier, but later
> >>> moved
> >>>> it
> >>>>>>> out
> >>>>>>>>>>>>> based on comments.
> >>>>>>>>>>>>>>>> In latest webrev, I call adjust_dram_regions() from
> >>> G1Policy-
> >>>>>>>> init(),
> >>>>>>>>>>>>> record_collection_pause_end() and
> >>> record_full_collection_end()
> >>>>>>>>>>>>> I like this approach much better.
> >>>>>>>>>>>>>
> >>
> ================================================================
> >>> =====================================================
> >>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionManager.cpp
> >>>>>>>>>>>>>> ---------------------------------------------
> >>>>>>>>>>>>>> 71 HeapRegionManager*
> >>>>>>>>>>>>>> HeapRegionManager::create_manager(G1CollectedHeap*
> >>> heap,
> >>>>>>>>>>>>>> CollectorPolicy* policy) {
> >>>>>>>>>>>>>> 72 if (heap->is_hetero_heap()) {
> >>>>>>>>>>>>>> 73 return new
> >>>>>>>>>>>>>> HeterogeneousHeapRegionManager((uint)(policy-
> >>>>>>>>>>> max_heap_byte_size()
> >>>>>>>>>>>> /
> >>>>>>>>>>>>>> HeapRegion::GrainBytes) /*heap size as num of regions*/);
> >>>>>>>>>>>>>> 74 }
> >>>>>>>>>>>>>> 75 return new HeapRegionManager();
> >>>>>>>>>>>>>> 76 }
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> As I mentioned above, I think the is_hetero check should be
> >>>> done
> >>>>>>> on
> >>>>>>>>>> the
> >>>>>>>>>>>>> policy instead. Otherwise this is a place where this check is
> >>> really
> >>>>>>>>> needed.
> >>>>>>>>>>>>>>>> Done.
> >>>>>>>>>>>>> Nice.
> >>>>>>>>>>>>>
> >>
> ================================================================
> >>> =====================================================
> >>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionSet.hpp
> >>>>>>>>>>>>>> -----------------------------------------
> >>>>>>>>>>>>>> 198 uint num_of_regions_in_range(uint start, uint
> end)
> >>>>> const;
> >>>>>>>>>>>>>> Was looking at the usage of this method and it is only used
> >> to
> >>>> get
> >>>>>>> the
> >>>>>>>>>>>> length
> >>>>>>>>>>>>> of the dram free-list. Looking at the
> >>> HeteroHeapRegionManager
> >>>>>>>>>> wouldn't it
> >>>>>>>>>>>>> be possible to add a second free-list, so we have one for
> >> dram
> >>>> and
> >>>>>>> one
> >>>>>>>>>> for
> >>>>>>>>>>>>> nvdimm.
> >>>>>>>>>>>>>>>> I had given this a thought when I started the
> >>> implementation,
> >>>>> but
> >>>>>>> I
> >>>>>>>>>>>> chose
> >>>>>>>>>>>>> to keep a single list so that I can re-use base class
> >>>>>>>>> (HeapRegionManager)
> >>>>>>>>>>>>> functions. For example, insert_into_free_list(),
> >>>>>>>>>>>>> allocate_free_regions_starting_at(), etc.
> >>>>>>>>>>>>>> Since the regions in free_list are already sorted, I did
> >> not
> >>>>> have
> >>>>>>> to
> >>>>>>>>>>>> change
> >>>>>>>>>>>>> a lot in free list management.
> >>>>>>>>>>>>> Ok, I'm fine with this if you think it makes your
> >> implementation
> >>>>>>> easier.
> >>>>>>>
> >>
> ================================================================
> >>> =====================================================
> >>>>>>>>>>>>>> src/hotspot/share/runtime/arguments.cpp
> >>>>>>>>>>>>>> ---------------------------------------
> >>>>>>>>>>>>>> 1625 if (AllocateOldGenAt != NULL) {
> >>>>>>>>>>>>>> 1626 FLAG_SET_ERGO(bool, UseCompressedOops, false);
> >>>>>>>>>>>>>> 1627 return;
> >>>>>>>>>>>>>> 1628 }
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Any reason this can't be done in GCArguments::initialize()?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> I moved it to GCArguments.
> >>>>>>>>>>>>> Perfect.
> >>>>>>>>>>>>>
> >>
> ================================================================
> >>> =====================================================
> >>>>> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp
> >>>>>>>>>>>>>> ----------------------------------------------------------
> >>>>>>>>>>>>>> 85 // Empty constructor, we'll initialize it with the
> >>>> initialize()
> >>>>>>>>>>>>>> method.
> >>>>>>>>>>>>>> 86 HeterogeneousHeapRegionManager(uint
> >>> num_regions)
> >>>> :
> >>>>>>>>>>>>>> _max_regions(num_regions), _max_dram_regions(0),
> >>>>>>>>>>>>>> 87
> >>>>>>>>>>>>>> _max_nvdimm_regions(0), _start_index_of_nvdimm(0)
> >>>>>>>>>>>>>> 88 {}
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> The member _total_commited_before_full_gc is not
> >>> initialized.
> >>>>>>>>>>>>>>>> Fixed
> >>>>>>>>>>>>> Thumbs up.
> >>>>>>>>>>>>>
> >>
> ================================================================
> >>> ======================================================
> >>>>>>>>>>>>>> 98 // Adjust dram_set to provision
> >>>> 'expected_num_regions'
> >>>>>>>>> regions.
> >>>>>>>>>>>>>> 99 void adjust_dram_regions(uint
> >>> expected_num_regions,
> >>>>>>>>>>>> WorkGang*
> >>>>>>>>>>>>>> pretouch_workers);
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> As mentioned I would turn this into a virtual method if it
> >> can't
> >>>> be
> >>>>>>>>>>>>> abstracted in another way. The WorkGang can be obtained
> >> by
> >>>>> calling
> >>>>>>>>>>>>>> G1CollectedHeap::heap()->workers() so you don't need to
> >>> pass
> >>>> it
> >>>>> in.
> >>>>>>>>>> This
> >>>>>>>>>>>>> way we get a cleaner interface I think.
> >>>>>>>>>>>>>>>> Thanks.
> >>>>>>>>>>>>> Thanks for addressing my comments,
> >>>>>>>>>>>>> Stefan
> >>>>>>>>>>>>>
> >>
> ================================================================
> >>> ======================================================
> >>>>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>>>> From: sangheon.kim at oracle.com
> >>>>>>>>> [mailto:sangheon.kim at oracle.com]
> >>>>>>>>>>>>>>> Sent: Wednesday, December 5, 2018 3:02 PM
> >>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan
> >>>>>>> Johansson
> >>>>>>>>>>>>>>> <stefan.johansson 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,
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On 12/5/18 12:51 AM, Kharbas, Kishor wrote:
> >>>>>>>>>>>>>>>> Hi Stefan, Sangheon,
> >>>>>>>>>>>>>>>> I have another webrev update -
> >>>>>>>>>>>>>>>>
> >> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.06
> >>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05_to_06
> >>>>>>>>>>>>>>>> Changes are:
> >>>>>>>>>>>>>>>> 1. While testing the new evacuation handling
> mechanism,
> >> I
> >>>>>>>>> identified
> >>>>>>>>>>>>> some
> >>>>>>>>>>>>>>> issues and fixed them. The implementation has been
> >> tested
> >>> by
> >>>>>>>>> tracing
> >>>>>>>>>>>> the
> >>>>>>>>>>>>>>> code path through the debugger.
> >>>>>>>>>>>>>>>> Based on the fix, here is an explanation for
> >> evacuation
> >>>>> failure
> >>>>>>>>>>>>> handling -
> >>>>>>>>>>>>>>>> 1) The implementation is based on the axiom
> >> that
> >>>> for
> >>>>>>> every
> >>>>>>>>>>>>> region
> >>>>>>>>>>>>>>> in dram where is a unavailable (not 'committed') region in
> >>> nv-
> >>>>>>> dimm.
> >>>>>>>>>> By
> >>>>>>>>>>>>>>> borrowing such regions, we can avoid evacuation failure
> >> for
> >>>>> young
> >>>>>>>>>>>> objects
> >>>>>>>>>>>>>>> and thus avoid young->old region conversion.
> >>>>>>>>>>>>>>>> 2) We cannot use borrowed regions for old
> >>> objects.
> >>>>> Since
> >>>>>>> in
> >>>>>>>>>>>>> worst
> >>>>>>>>>>>>>>> case for evacuating 'n' young and 'm' old regions we need
> >> 'n
> >>> +
> >>>>> m'
> >>>>>>>>>>>>> destination
> >>>>>>>>>>>>>>> regions. And we may need to borrow 'n+m' from nv-
> dimm.
> >>> But
> >>>>> we
> >>>>>>>>> can
> >>>>>>>>>>>>> only
> >>>>>>>>>>>>>>> guarantee 'n' borrowed regions.
> >>>>>>>>>>>>>>>> So we use borrowed region only for copying
> >>>> young
> >>>>>>>>>> objects,
> >>>>>>>>>>>>> else
> >>>>>>>>>>>>>>> we cannot avoid young->old conversion and will need a
> >>> special
> >>>>>>>>> region
> >>>>>>>>>>>>> type
> >>>>>>>>>>>>>>> as in current implementation anyway.
> >>>>>>>>>>>>>>>> 3) When we cannot allocate a new region for
> >>>>>>>>>>>> OldGCAllocRegion,
> >>>>>>>>>>>>> we
> >>>>>>>>>>>>>>> borrow a region from hrm() to use as allocation region.
> We
> >>>>> release
> >>>>>>>>>> (un-
> >>>>>>>>>>>>>>> commit) that many number of regions in
> >>>> adjust_dram_regions()
> >>>>>>>>> which
> >>>>>>>>>> is
> >>>>>>>>>>>>>>> called at end of GC.
> >>>>>>>>>>>>>>>> 4) So we may be in situation where there is
> valid
> >>> old
> >>>>> plab
> >>>>>>>>> and
> >>>>>>>>>>>>>>> OldGCAllocRegion but we can only use them when source
> >> is
> >>>>> young.
> >>>>>>>>> So
> >>>>>>>>>>>> we
> >>>>>>>>>>>>>>> add another clause to the if() statement at the beginning
> of
> >>>>>>>>>>>>>>> copy_to_survivor_space() to check for this condition.
> >>>>>>>>>>>>>>>> For ease of review I extracted evacuation failure
> >>>> handling
> >>>>>>>>>>>>>>>> changes here -
> >>>>>>>>>>>>>>>>
> >> http://cr.openjdk.java.net/~kkharbas/8211425/webrev_EvacuationFailure
> >>>>>>>>>>>>>>> I have some comments.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> 1. Have you test enough for evacuation failure handling
> >>> case?
> >>>>> i.e. it
> >>>>>>>>>>>> would
> >>>>>>>>>>>>> be
> >>>>>>>>>>>>>>> better to have tests for these cases.
> >>>>>>>>>>>>>>> e.g. How can we guarantee the total heap will be less
> >>> than
> >>>>> Xmx
> >>>>>>>>>> after
> >>>>>>>>>>>>> GC if
> >>>>>>>>>>>>>>> we followed 'borrowing region path'.
> >>>>>>>>>>>>>>> Couldn't shrinking dram fail after borrowing a
> >> region?
> >>>>>>>>>>>>>>> (heterogeneousHeapRegionManager.cpp:65)
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] Since we only copy objects from young
> >>>>> generation,
> >>>>>>>>> we
> >>>>>>>>>>>>> will never borrow more than the length of young generation
> >> in
> >>>> the
> >>>>>>>>>>>> collection
> >>>>>>>>>>>>> set. Also we should completely evacuate the young
> >> generation,
> >>>> so
> >>>>>>>>> when
> >>>>>>>>>>>>> collection set is freed, we have that many free regions to
> >>> shrink.
> >>>>>>>>>>>>>>> 2. Do we still need pre-mature old type after introducing
> >>>>>>> 'borrowing
> >>>>>>>>>>>>>>> concept'? (Stefan also mentioned about this)
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] Done (I had kept it for removal later)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> 3. (Again) Please don't merge ParallelGC patch(8211424)
> >> into
> >>>> G1
> >>>>>>>>>>>>>>> Patch(8211425) as those should be committed separately.
> >>> And
> >>>>> this
> >>>>>>>>>> email
> >>>>>>>>>>>>>>> thread is to review G1. :) i.e. you should commit identical
> >>>>> reviewed
> >>>>>>>>>>>> patch,
> >>>>>>>>>>>>>>> not modifying it (to split Parallel part).
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] Done.
> >>>>>>>>>>>>>>> --------------------------------------
> >>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> >>>>>>>>>>>>>>> 4561 new_alloc_region =
> >>>>>>>>>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(hrm())-
> >>>>>>>>>>>>>>>> borrow_old_region_for_gc();
> >>>>>>>>>>>>>>> - This can be moved to hetero hrm. Stefan also mentioned
> >>>> this.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] I could not find an existing method in hrm
> >> to
> >>>>>>> handle
> >>>>>>>>>>>> this.
> >>>>>>>>>>>>> Two methods to allocate regions are allocate_free_region()
> >> and
> >>>>>>>>>>>>> allocate_free_regions_starting_at().
> >>>>>>>>>>>>>> Allocate_free_region() is also called for
> >>>>>>> humongous
> >>>>>>>>>>>>> allocation with 'old' parameter so I can't issue a borrowed
> >>> region
> >>>>> in
> >>>>>>>>>> every
> >>>>>>>>>>>>> call.
> >>>>>>>>>>>>>> Please let me know if you think add a
> >> new
> >>>>> hrm
> >>>>>>>>>> method
> >>>>>>>>>>>>> for use in this code and abstracting it, seems a good solution.
> >>>>>>>>>>>>>>> --------------------------------------
> >>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
> >>>>>>>>>>>>>>> 227 // Third clause: if it is a heterogenous heap, we
> >>>> borrow
> >>>>>>>>> regions
> >>>>>>>>>>>>>>> from nv-dimm for old gc allocation.
> >>>>>>>>>>>>>>> 228 // This region can only be used to evacate young
> >>>>> objects,
> >>>>>>> so
> >>>>>>>>> we
> >>>>>>>>>>>>>>> check the state of the object being copied.
> >>>>>>>>>>>>>>> - Something like this:
> >>>>>>>>>>>>>>> // Third clause: if it is a heterogenous heap, we can
> >>> borrow
> >>>>>>>>> regions
> >>>>>>>>>>>>>>> from nv-dimm for old region allocation. And these regions
> >>>>> should
> >>>>>>> be
> >>>>>>>>>>>> only
> >>>>>>>>>>>>>>> used to evacuate young objects.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] Done.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> 229 if (_old_gen_is_full && dest_state.is_old() ||
> >>>>>>>>>>>>>>> 230 (_g1h->is_hetero_heap() && state.is_old() &&
> >>>>>>>>>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(_g1h-
> >>>>>> hrm())-
> >>>>>>>>>>>>>>>> is_old_full())
> >>>>>>>>>>>>>>> ) {
> >>>>>>>>>>>>>>> - It would be better to have another parenthesis at line
> >> 229.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] Done.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> --------------------------------------
> >>>>>>>>>>>>>>>
> >>>>> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.cpp
> >>>>>>>>>>>>>>> 65 uint num =
> shrink_dram(_no_borrowed_regions);
> >>>>>>>>>>>>>>> 66 assert(num == _no_borrowed_regions, "Should
> >> be
> >>>> able
> >>>>> to
> >>>>>>>>>>>>>>> uncommit the number of regions borrowed for evacuation
> >>>>> failure
> >>>>>>>>>>>>> handling");
> >>>>>>>>>>>>>>> - Couldn't we fail at line 65?
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] Replied above.
> >>>>>>>>>>>>>>> --------------------------------------
> >>>>>>>>>>>>>>>
> >>>>> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp
> >>>>>>>>>>>>>>> 45 bool _is_old_full;
> >>>>>>>>>>>>>>> - This name looks mis-leading. It sets true when we
> >>>> temporarily
> >>>>>>>>>> borrow
> >>>>>>>>>>>> a
> >>>>>>>>>>>>>>> region from nvdimm during GC. To align with
> >>>>>>>>>> '_no_borrowed_regions',
> >>>>>>>>>>>>>>> something like 'has_borrowed_regions'?
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] Done.
> >>>>>>>>>>>>>>> 135 bool is_old_full();
> >>>>>>>>>>>>>>> - 'const'?
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] Replace this method with
> >>>>>>> has_borrowed_regions().
> >>>>>>>>>>>>>>>> 2. There is a fix for incorrect handling of humongous
> >>>>> allocations
> >>>>>>>>>> under
> >>>>>>>>>>>>>>> certain situations.
> >>>>>>>>>>>>>>>> 3. My jtreg tests are running, will provide the results
> when
> >>>> they
> >>>>>>>>>> finish.
> >>>>>>>>>>>>>>> I guess you are running serviceability as well?
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>> [Kharbas, Kishor] Yes I have added them.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>> Sangheon
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Let me know your feedback.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Thank you.
> >>>>>>>>>>>>>>>> Kishor
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>>>>>> From: Kharbas, Kishor
> >>>>>>>>>>>>>>>>> Sent: Monday, December 3, 2018 11:48 PM
> >>>>>>>>>>>>>>>>> To: sangheon.kim at oracle.com; Stefan Johansson
> >>>>>>>>>>>>>>>>> <stefan.johansson 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>;
> >>>>>>>>>> Kharbas,
> >>>>>>>>>>>>>>> Kishor
> >>>>>>>>>>>>>>>>> <kishor.kharbas at intel.com>
> >>>>>>>>>>>>>>>>> Subject: RE: RFR(M): 8211425: Allocation of old
> >> generation
> >>>> of
> >>>>>>> java
> >>>>>>>>>>>> heap
> >>>>>>>>>>>>>>> on
> >>>>>>>>>>>>>>>>> alternate memory devices - G1 GC
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Hi Sangheon and Stefan,
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> I have updated webrev for this feature,
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> 1. Webrev with Stefan's comments on Parallel GC
> >>>> (abstraction
> >>>>>>>>>> related
> >>>>>>>>>>>>>>>>> comments) :
> >>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04
> >>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03_to_04
> >>>>>>>>>>>>>>>>> 2. Webrev with evacuation failure handling :
> >>>>>>>>>>>>>>>>>
> >> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05
> >>>>>>>>>>>>>>>>>
> >>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04_to_05
> >>>>>>>>>>>>>>>>> Testing underway.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> 3. Explanation of evacuation failure handling:
> >>>>>>>>>>>>>>>>> 1) The implementation is based on the axiom
> >>> that
> >>>>> for
> >>>>>>>>> every
> >>>>>>>>>>>>> region
> >>>>>>>>>>>>>>> in
> >>>>>>>>>>>>>>>>> dram where is a unavailable (not 'committed') region in
> >>> nv-
> >>>>>>> dimm.
> >>>>>>>>>> By
> >>>>>>>>>>>>>>>>> borrowing such regions, we can avoid evacuation failure
> >>> for
> >>>>>>> young
> >>>>>>>>>>>>>>> objects
> >>>>>>>>>>>>>>>>> and thus avoid young->old region conversion.
> >>>>>>>>>>>>>>>>> 2) We cannot use borrowed regions for old
> >>>> objects.
> >>>>>>> Since
> >>>>>>>>> in
> >>>>>>>>>>>>> worst
> >>>>>>>>>>>>>>>>> case for evacuating 'n' young and 'm' old regions we
> >> need
> >>> 'n
> >>>> +
> >>>>>>> m'
> >>>>>>>>>>>>>>> destination
> >>>>>>>>>>>>>>>>> regions. And we may need to borrow 'n+m' from nv-
> >>> dimm.
> >>>>> But
> >>>>>>> we
> >>>>>>>>>>>> can
> >>>>>>>>>>>>>>> only
> >>>>>>>>>>>>>>>>> guarantee 'n' borrowed regions.
> >>>>>>>>>>>>>>>>> So we use borrowed region only for
> copying
> >>>>> young
> >>>>>>>>>> objects,
> >>>>>>>>>>>>> else
> >>>>>>>>>>>>>>> we
> >>>>>>>>>>>>>>>>> cannot avoid young->old conversion and will need a
> >>> special
> >>>>>>> region
> >>>>>>>>>>>> type
> >>>>>>>>>>>>> as
> >>>>>>>>>>>>>>> in
> >>>>>>>>>>>>>>>>> current implementation anyway.
> >>>>>>>>>>>>>>>>> 3) When we cannot allocate a new region for
> >>>>>>>>>>>> OldGCAllocRegion,
> >>>>>>>>>>>>>>> we
> >>>>>>>>>>>>>>>>> borrow a region from hrm() to use as allocation region.
> >>> We
> >>>>> sort
> >>>>>>> of
> >>>>>>>>>>>>>>> release
> >>>>>>>>>>>>>>>>> that many number of regions in adjust_dram_regions()
> >>>> which
> >>>>> is
> >>>>>>>>>> called
> >>>>>>>>>>>>> at
> >>>>>>>>>>>>>>> end
> >>>>>>>>>>>>>>>>> of GC.
> >>>>>>>>>>>>>>>>> 4) When G1ParScanThreadState:
> >> _old_gen_is_full
> >>>> is
> >>>>>>> true,
> >>>>>>>>>> we
> >>>>>>>>>>>>> only
> >>>>>>>>>>>>>>> let
> >>>>>>>>>>>>>>>>> young objects be allocated from the borrowed region.
> >>> There
> >>>>>>> might
> >>>>>>>>>> be
> >>>>>>>>>>>> a
> >>>>>>>>>>>>>>> race
> >>>>>>>>>>>>>>>>> condition here, when one thread enters plab_allocate()
> >>>> while
> >>>>>>>>> other
> >>>>>>>>>>>>> thread
> >>>>>>>>>>>>>>>>> determines and set _old_gen_is_full.
> >>>>>>>>>>>>>>>>> But we can tolerate this race condition.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> 4. I am testing all tier1 tests and appCDS tests with the
> >>>>> combined
> >>>>>>>>>>>> patch.
> >>>>>>>>>>>>>>> Will
> >>>>>>>>>>>>>>>>> provide an update soon.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>>>> Kishor
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>>>>>>> From: sangheon.kim at oracle.com
> >>>>>>>>>>>> [mailto:sangheon.kim at oracle.com]
> >>>>>>>>>>>>>>>>>> Sent: Sunday, December 2, 2018 2:49 PM
> >>>>>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> >> Stefan
> >>>>>>>>>> Johansson
> >>>>>>>>>>>>>>>>>> <stefan.johansson 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,
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> On 11/29/18 12:19 PM, Kharbas, Kishor wrote:
> >>>>>>>>>>>>>>>>>>> Hi Sangheon,
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> Thank you for reviewing the webrev.
> >>>>>>>>>>>>>>>>>>> 1. Based on your feedback I have newer webrev.
> >> There
> >>>> are
> >>>>>>>>> some
> >>>>>>>>>>>>>>>>>> comments inline about AppCDS (top of previous
> email).
> >>>>>>>>>>>>>>>>>>> 2. Webrev with your feedback on G1 GC patch :
> >>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.02/
> >> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01_to_02/
> >>>>>>>>>>>>>>>>>> Webrev.02 looks good.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> 3. I merged the Parallel GC patch with your latest
> >>>> comment,
> >>>>>>>>>>>>>>>>>>> consolidated jtreg tests and combined webrev is :
> >>>>>>>>>>>>>>>>>>>
> >>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03
> >>>>>>>>>>>>>>>>>> I meant to work based on Parallel GC patch so that
> your
> >>> G1
> >>>>>>> patch
> >>>>>>>>>>>>>>>>>> doesn't need to comment out Paralell GC test case at
> >>> newly
> >>>>>>>>> added
> >>>>>>>>>>>>> tests.
> >>>>>>>>>>>>>>>>>> I only checked commented out parts of Parallel GC are
> >>>>> enabled.
> >>>>>>>>>> This
> >>>>>>>>>>>>>>>>>> looks good too.
> >>>>>>>>>>>>>>>>>> I would expect that you completed running whole your
> >>>>>>>>> patches(G1
> >>>>>>>>>>>>> and
> >>>>>>>>>>>>>>>>>> Parallel GC) together without any failures.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> 4. I moved the CSR to finalized. Thanks for your
> >> review.
> >>>> We
> >>>>>>> will
> >>>>>>>>>>>>>>>>>>> wait to
> >>>>>>>>>>>>>>>>>> hear from CSR committee.
> >>>>>>>>>>>>>>>>>> Okay, I saw it is approved now.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> 5. I am working on Stefan's latest feedback on
> >> handling
> >>> of
> >>>>>>>>>>>>>>>>>>> evacuation
> >>>>>>>>>>>>>>>>>> failure.
> >>>>>>>>>>>>>>>>>> OK.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>>>>> Sangheon
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> Regards,
> >>>>>>>>>>>>>>>>>>> Kishor
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>>>>>>>>> From: sangheon.kim at oracle.com
> >>>>>>>>>>>>>>> [mailto:sangheon.kim at oracle.com]
> >>>>>>>>>>>>>>>>>>>> Sent: Tuesday, November 27, 2018 9:25 PM
> >>>>>>>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> >>> Stefan
> >>>>>>>>>>>> Johansson
> >>>>>>>>>>>>>>>>>>>> <stefan.johansson 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,
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> On 11/19/18 6:20 PM, Kharbas, Kishor wrote:
> >>>>>>>>>>>>>>>>>>>>> Hi Sangheon,
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> Here is the incremental webrev -
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>
> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00_to_01
> >>>>>>>>>>>>>>>>>>>> Thanks for the incremental webrev.
> >>>>>>>>>>>>>>>>>>>> Here are some comments for webrev.01.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 1. General comments:
> >>>>>>>>>>>>>>>>>>>> - Any update with AppCDS tests? (You mentioned
> >> this
> >>> on
> >>>>>>> your
> >>>>>>>>>>>>>>>>>>>> previous
> >>>>>>>>>>>>>>>>>> email)
> >>>>>>>>>>>>>>>>>>> [Kharbas, Kishor] I completed testing of AppCDS
> >>> with :
> >>>>> 1)
> >>>>>>>>>> original
> >>>>>>>>>>>>>>>>>>> code. 2)
> >>>>>>>>>>>>>>>>>> patch applied, but flag not used. 3) patch applied, flag
> >>> set.
> >>>>>>>>>>>>>>>>>>> Test results at :
> >>> http://cr.openjdk.java.net/~kkharbas/8211425/JT_AppCDS
> >>>>>>>>>>>>>>>>>>> 1) and 2) both show "Test
> >> results:
> >>>>>>>>>>>>>>>>>>> passed: 120; failed: 1;
> >>>>>>>>>>>>>>>>>> error: 2"
> >>>>>>>>>>>>>>>>>>> 3) No tests are run, all are
> >>>>>>>>>>>>>>>>>>> skipped. Do you know if
> >>>>>>>>>>>>>>>>>> appCDS does not work with certain flags. I use jtreg
> flag
> >>>>>>>>>>>>>>>>>> -javaoptions:"- XX:+UnlockExperimentalVMOptions -
> >>>>>>>>>>>>>>>>> XX:AllocateOldGenAt=/home/kishor".
> >>>>>>>>>>>>>>>>>>>> - Please test with ParallelGC patch added. i.e.
> Applied
> >>>> both
> >>>>>>>>>>>>>>>>>>>> ParallelGC and
> >>>>>>>>>>>>>>>>>> G1
> >>>>>>>>>>>>>>>>>>>> parts, so that you don't need to comment out
> >>> ParallelGC
> >>>>> on
> >>>>>>>>> new
> >>>>>>>>>>>>> tests.
> >>>>>>>>>>>>>>>>>>> [Kharbas, Kishor] The tests pass with merged patch,
> >>>> testing
> >>>>>>> both
> >>>>>>>>>>>>>>>>>>> ParallelGC
> >>>>>>>>>>>>>>>>>> and G1.
> >>>>>>>>>>>>>>>>>>>> 2. New tests related general comments:
> >>>>>>>>>>>>>>>>>>>> - As AllocateOldGenAt feature will not be tested
> >>>> regularly,
> >>>>>>> new
> >>>>>>>>>>>>>>>>>>>> tests
> >>>>>>>>>>>>>>>>>> should
> >>>>>>>>>>>>>>>>>>>> be excluded by default.
> >>>>>>>>>>>>>>>>>>> [Kharbas, Kishor] How do I do this? Should I remove
> >>> these
> >>>>> test
> >>>>>>>>>> from
> >>>>>>>>>>>>>>>>>> TEST.groups?
> >>>>>>>>>>>>>>>>>>>> - New tests need filters for other unsupported GCs.
> >>>>>>>>>>>>>>>>>>>> e.g. @requires vm.gc=="null" | vm.gc.G1 |
> >>>>>>> vm.gc.Parallel
> >>>>>>>>>>>>>>>>>>> [Kharbas, Kishor] Done.
> >>>>>>>>>>>>>>>>>>>> - Are there any reason for printing stdout in new
> >> tests?
> >>>> i.e.
> >>>>>>>>> looks
> >>>>>>>>>>>>>>>>>>>> like we don't need
> >>>>> 'System.out.println(output.getStdout());'
> >>>>>>>>>>>>>>>>>>> [Kharbas, Kishor] not really, I checked that output is
> >>>>> dumped
> >>>>>>> to
> >>>>>>>>>>>>>>>>>>> stdout of
> >>>>>>>>>>>>>>>>>> jtreg anyway. So I removed this.
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeapHeap.hpp
> >>>>>>>>>>>>>>>>>>>> 45 #include "gc/g1/heapRegionManager.hpp"
> >>>>>>>>>>>>>>>>>>>> 46 #include
> >>>>>>>>>> "gc/g1/heterogeneousHeapRegionManager.hpp"
> >>>>>>>>>>>>>>>>>>>> 47 #include "gc/g1/heapRegionSet.hpp"
> >>>>>>>>>>>>>>>>>>>> - Line 46 should be below line 47.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectorPolicy.hpp
> >>>>>>>>>>>>>>>>>>>> 41 virtual size_t
> >> heap_reservation_size_bytes();
> >>>>>>>>>>>>>>>>>>>> - const?
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 45 size_t _heap_reservation_size_bytes;
> >>>>>>>>>>>>>>>>>>>> - Do we really need this variable? Just returning
> >>>>>>>>>>>>>>>>>>>> 2*_max_heap_byte_size seems enough. In this case,
> >>>>> adding
> >>>>>>>>>>>> virtual
> >>>>>>>>>>>>>>>>>>>> size_t
> >>>>>>>>>>>>>>>>>>>> heap_reservation_size_bytes() seems enough.
> >>>>>>>>>>>>>>>>>>>> - Regarding naming, how about
> >>>>> heap_reserved_size_bytes()?
> >>>>>>>>>>>>>>>>>>>> 51 size_t heap_reservation_size_bytes();
> >>>>>>>>>>>>>>>>>>>> - I would prefer to have 'virtual' for readability. I saw
> >>> you
> >>>>>>>>>>>>>>>>>>>> reflected this
> >>>>>>>>>>>>>>>>>> kind
> >>>>>>>>>>>>>>>>>>>> of my comment in other places so asking here too.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>>
> >>> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
> >>>>>>>>>>>>>>>>>>>> 25 #include "logging/log.hpp"
> >>>>>>>>>>>>>>>>>>>> - precompiled.hpp comes first
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 180 return false;
> >>>>>>>>>>>>>>>>>>>> 181 log_error(gc, init)("Could not create file
> >> for
> >>>> Old
> >>>>>>>>>>>>>>>>>>>> generation at location %s", AllocateOldGenAt);
> >>>>>>>>>>>>>>>>>>>> - We never reach line 181 because of line 180? :)
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 196
> >>>>>>>>>>>>>>>>>>>>
> >>>
> G1RegionToHeteroSpaceMapper::G1RegionToHeteroSpaceMapper(Reserve
> >>>>>>>>>>>>>>>>>>>> dSpace rs,
> >>>>>>>>>>>>>>>>>>>> - My previous comment was about splitting
> >>> initialization
> >>>>>>>>>>>> part(which
> >>>>>>>>>>>>>>>>>>>> does actual file-heap mapping) from ctor, so that we
> >>> can
> >>>>>>> avoid
> >>>>>>>>>>>>>>>>>>>> failure during construction. After construction, call
> >>>>> initialize
> >>>>>>>>>>>>>>>>>>>> method to do file mapping and then report any
> >>> failures
> >>>> if
> >>>>>>>>> have.
> >>>>>>>>>>>>>>>>>>>> 196
> >>>>>>>>>>>>>>>>>>>>
> >>>
> G1RegionToHeteroSpaceMapper::G1RegionToHeteroSpaceMapper(Reserve
> >>>>>>>>>>>>>>>>>>>> dSpace rs,
> >>>>>>>>>>>>>>>>>>>> 197 size_t actual_size,
> >>>>>>>>>>>>>>>>>>>> 198 size_t page_size,
> >>>>>>>>>>>>>>>>>>>> 199 size_t alloc_granularity,
> >>>>>>>>>>>>>>>>>>>> 200 size_t commit_factor,
> >>>>>>>>>>>>>>>>>>>> 201 MemoryType type) :
> >>>>>>>>>>>>>>>>>>>> 202 G1RegionToSpaceMapper(rs, actual_size,
> >>>>>>> page_size,
> >>>>>>>>>>>>>>>>>>>> alloc_granularity, commit_factor, type),
> >>>>>>>>>>>>>>>>>>>> 203 _num_committed_dram(0),
> >>>>>>>>>>>> _num_committed_nvdimm(0),
> >>>>>>>>>>>>>>>>>>>> _success(false) {
> >>>>>>>>>>>>>>>>>>>> - Looks like a bad alignment. I think parameters and
> >>>>>>>>> initialization
> >>>>>>>>>>>>>>>>>>>> list(parental class as well) should be distinguishable.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 295 delete mapper;
> >>>>>>>>>>>>>>>>>>>> - Existing issue(no action needed): current mapper
> >>> code
> >>>>>>>>> doesn't
> >>>>>>>>>>>>>>>>>>>> have a destructor.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionManager.cpp
> >>>>>>>>>>>>>>>>>>>> 29 #include
> >>> "gc/g1/heapRegionManager.inline.hpp"
> >>>>>>>>>>>>>>>>>>>> 30 #include
> >>>>>>>>>> "gc/g1/heterogeneousHeapRegionManager.hpp"
> >>>>>>>>>>>>>>>>>>>> 31 #include "gc/g1/heapRegionSet.inline.hpp"
> >>>>>>>>>>>>>>>>>>>> - Move line 30 after line 31.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionManager.hpp
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 117 FreeRegionList _free_list;
> >>>>>>>>>>>>>>>>>>>> 118 void make_regions_available(uint index,
> >> uint
> >>>>>>>>>> num_regions
> >>>>>>>>>>>> =
> >>>>>>>>>>>>>>>>>>>> 1,
> >>>>>>>>>>>>>>>>>>>> WorkGang* pretouch_gang = NULL);
> >>>>>>>>>>>>>>>>>>>> - New line between 117 and 118 please.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 130 static HeapRegionManager*
> >>>>>>>>>>>>>>> create_manager(G1CollectedHeap*
> >>>>>>>>>>>>>>>>>>>> heap,
> >>>>>>>>>>>>>>>>>>>> CollectorPolicy* policy);
> >>>>>>>>>>>>>>>>>>>> - I see build failure in solaris.
> >>>>>>>>>>>>>>>>>>>>
> >>>>> open/src/hotspot/share/gc/g1/heapRegionManager.hpp",
> >>>>>>> line
> >>>>>>>>>>>> 129:
> >>>>>>>>>>>>>>>>> Error:
> >>>>>>>>>>>>>>>>>>>> CollectorPolicy is not defined.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionType.cpp
> >>>>>>>>>>>>>>>>>>>> - Copyright update
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/shared/gcArguments.cpp
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 68 "AllocateOldGenAt not supported for
> >>>> selected
> >>>>>>>>>> GC.\n");
> >>>>>>>>>>>>>>>>>>>> - "AllocateOldGenAt *is* not supported ..."
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/whitebox.cpp
> >>>>>>>>>>>>>>>>>>>> 510 return (jlong)(Universe::heap()->base()
> +
> >>>>>>>>> start_region
> >>>>>>>>>>>>>>>>>>>> * HeapRegion::GrainBytes);
> >>>>>>>>>>>>>>>>>>>> - g1h->base() is same, isn't it? There are more lines
> >>> that
> >>>>>>> using
> >>>>>>>>>>>>>>>>>>>> Universe::heap()->base().
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 524 }
> >>>>>>>>>>>>>>>>>>>> 525 else {
> >>>>>>>>>>>>>>>>>>>> and
> >>>>>>>>>>>>>>>>>>>> 551 }
> >>>>>>>>>>>>>>>>>>>> 552 else {
> >>>>>>>>>>>>>>>>>>>> - Make at one line. i.e. '} else {'
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/arguments.cpp
> >>>>>>>>>>>>>>>>>>>> 1625 if (AllocateOldGenAt != NULL) {
> >>>>>>>>>>>>>>>>>>>> 1626 FLAG_SET_ERGO(bool, UseCompressedOops,
> >>>> false);
> >>>>>>>>>>>>>>>>>>>> 1627 return;
> >>>>>>>>>>>>>>>>>>>> 1628 }
> >>>>>>>>>>>>>>>>>>>> - Wrong alignment. Needs 2 spaces for line
> >> 1625~1628.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/globals.hpp
> >>>>>>>>>>>>>>>>>>>> 2606 experimental(ccstr, AllocateOldGenAt, NULL,
> >>>>>>>>>>>>>>>>>>>> \
> >>>>>>>>>>>>>>>>>>>> 2607 "Directory to use for allocating old
> >>>>>>>>>>>>>>>>>>>> generation")
> >>>>>>>>>>>>>>>>>>>> - How about moving AllocateOldGenAt around
> >>>>>>> AllocateHeapAt
> >>>>>>>>>>>>> option.
> >>>>>>>>>>>>>>>>>>>> - Change the description similar to AllocateHeapAt
> as
> >> it
> >>>>>>>>> explains
> >>>>>>>>>>>>>>> better.
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>>
> >> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.cpp
> >>>>>>>>>>>>>>>>>>>> 29 #include
> >>> "gc/g1/heapRegionManager.inline.hpp"
> >>>>>>>>>>>>>>>>>>>> 30 #include
> >>>>>>>>>> "gc/g1/heterogeneousHeapRegionManager.hpp"
> >>>>>>>>>>>>>>>>>>>> 31 #include "gc/g1/heapRegionSet.inline.hpp"
> >>>>>>>>>>>>>>>>>>>> - Line 30 should be located after line 31.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>>
> >> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp
> >>>>>>>>>>>>>>>>>>>> #ifndef
> >> SHARE_VM_GC_G1_HeterogeneousHeapRegionManager_HPP
> >>>>>>>>>>>>>>>>>>>> - Should be all uppercase letters.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> ----------------------
> >>>>>>>>>>>>>>>>>>>>
> >> test/hotspot/jtreg/gc/8202286/TestAllocateOldGenAtMultiple.java
> >>>>>>>>>>>>>>>>>>>> 56 "-Xmx4g -Xms4g", // 4.
> >>>>>>>>>>>>>>>>>>>> With larger heap size (UnscaledNarrowOop not
> >>>> possible).
> >>>>>>>>>>>>>>>>>>>> 57 "-Xmx4g -Xms4g -
> >>>> XX:+UseLargePages", //
> >>>>> 5.
> >>>>>>>>>>>>>>>>>>>> Set UseLargePages.
> >>>>>>>>>>>>>>>>>>>> 58 "-Xmx4g -Xms4g -
> >>> XX:+UseNUMA" //
> >>>> 6.
> >>>>>>> Set
> >>>>>>>>>>>>>>> UseNUMA.
> >>>>>>>>>>>>>>>>>>>> - It would be better to use smaller heap for test
> >>>>>>> stability(even
> >>>>>>>>>>>>>>>>>>>> though this will not be tested regularly) unless you
> >>> have
> >>>>> any
> >>>>>>>>>> good
> >>>>>>>>>>>>>>>>> reason for it.
> >>>>>>>>>>>>>>>>>>>>> I have also filed a CSR at
> >>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-
> >>>>>>>>>>>>>>>>>>>> 8214081
> >>>>>>>>>>>>>>>>>>>> I reviewed this CSR as well.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>>>>>>> Sangheon
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> Thank you,
> >>>>>>>>>>>>>>>>>>>>> Kishor
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>>>>>>>>>>> From: sangheon.kim at oracle.com
> >>>>>>>>>>>>>>>>> [mailto:sangheon.kim at oracle.com]
> >>>>>>>>>>>>>>>>>>>>>> Sent: Sunday, November 18, 2018 11:14 AM
> >>>>>>>>>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
> >>>> Stefan
> >>>>>>>>>>>>> Johansson
> >>>>>>>>>>>>>>>>>>>>>> <stefan.johansson at oracle.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>
> >>>>>>>>>>>>>>>>>>>>>> Cc: 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,
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> Could you give us incremental webrev? It will help
> >> a
> >>>> lot
> >>>>> for
> >>>>>>>>>>>>>>> reviewers.
> >>>>>>>>>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>>>>>>>>> Sangheon
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> On 11/16/18 6:35 PM, Kharbas, Kishor wrote:
> >>>>>>>>>>>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> I have an update to the webrev at :
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01
> >>>>>>>>>>>>>>>>>>>>>>> 1. Most of the comments from Sangheon and
> >>> Stefan
> >>>>>>> have
> >>>>>>>>>>>> been
> >>>>>>>>>>>>>>>>>>>> addressed.
> >>>>>>>>>>>>>>>>>>>>>> For ease of reading, I am copy-pasting below the
> >>>>>>> comments
> >>>>>>>>>> not
> >>>>>>>>>>>>>>>>>>>>>> fixed
> >>>>>>>>>>>>>>>>>> or
> >>>>>>>>>>>>>>>>>>>> for
> >>>>>>>>>>>>>>>>>>>>>> which I have follow up.
> >>>>>>>>>>>>>>>>>>>>>>> 2. I also ran jtreg tests, with and without flag
> >> using
> >>>> test
> >>>>>>>>>>>>>>>>>>>>>>> group shown
> >>>>>>>>>>>>>>>>>>>> below
> >>>>>>>>>>>>>>>>>>>>>> (removed tests incompatible with this flag. Also,
> >> for
> >>>>>>> testing,
> >>>>>>>>> I
> >>>>>>>>>>>>>>>>>>>>>> made - XX:+AllocateOldGenAt a no-op instead of
> >>> error
> >>>>>>> since
> >>>>>>>>>>>> many
> >>>>>>>>>>>>>>>>>>>>>> tests have
> >>>>>>>>>>>>>>>>>> sub-
> >>>>>>>>>>>>>>>>>>>>>> tests for all different GCs)
> >>>>>>>>>>>>>>>>>>>>>>> I see same number of failures (12) for
> >>> original,
> >>>>>>> patch
> >>>>>>>>>>>>>>>>>>>>>>> without flag and with flag (no new failures) 3.
> >> Tasks
> >>>> in
> >>>>>>>>>>>> progress
> >>>>>>>>>>>>>>> are :
> >>>>>>>>>>>>>>>>>>>>>>> - AppCDS tests.
> >>>>>>>>>>>>>>>>>>>>>>> - CSR filing.
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> tier1_old_nvdimm = \
> >>>>>>>>>>>>>>>>>>>>>>> serviceability/ \
> >>>>>>>>>>>>>>>>>>>>>>> :tier1_gc \
> >>>>>>>>>>>>>>>>>>>>>>> :tier1_runtime \
> >>>>>>>>>>>>>>>>>>>>>>> -gc/serial \
> >>>>>>>>>>>>>>>>>>>>>>> -gc/parallel \
> >>>>>>>>>>>>>>>>>>>>>>> -gc/cms \
> >>>>>>>>>>>>>>>>>>>>>>> -gc/epsilon \
> >>>>>>>>>>>>>>>>>>>>>>> -gc/TestAllocateHeapAt.java \
> >>>>>>>>>>>>>>>>>>>>>>> -gc/TestAllocateHeapAtError.java \
> >>>>>>>>>>>>>>>>>>>>>>> -gc/TestAllocateHeapAtMultiple.java \
> >>>>>>>>>>>>>>>>>>>>>>> -gc/g1/TestFromCardCacheIndex.java \
> >>>>>>>>>>>>>>>>>>>>>>> -
> >>>> gc/metaspace/TestMetaspacePerfCounters.java
> >>>>> \
> >>>>>>>>>>>>>>>>>>>>>>> -
> >>>>>>>>> gc/metaspace/TestPerfCountersAndMemoryPools.java
> >>>>>>>>>> \
> >>>>>>>>>>>>>>>>>>>>>>> -
> >>>> runtime/Metaspace/PrintMetaspaceDcmd.java
> >>>>>>>>>>>>>>>>>>>>>>>
> >> ========================================================
> >>>>>>>>>>>>>>>>>>>>>>> Comments from Sangheon:
> >>>>>>>>>>>>>>>>>>>>>>>
> >> ========================================================
> >>>>> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
> >>>>>>>>>>>>>>>>>>>>>>> 173 void
> >>>>> G1RegionToHeteroSpaceMapper::map_nvdimm_space(ReservedSpace
> >>>>>>>>>>>>>>>>>>>>>>> rs) {
> >>>>>>>>>>>>>>>>>>>>>>> - Your previous change of adding AllocateHeapAt
> >>>>> created
> >>>>>>>>> nv
> >>>>>>>>>>>> file
> >>>>>>>>>>>>>>>>>>>>>>> inside
> >>>>>>>>>>>>>>>>>> of
> >>>>>>>>>>>>>>>>>>>>>> ReservedHeapSpace but this-AllocateOldGenAt
> >>> create
> >>>>>>> inside
> >>>>>>>>>> of
> >>>>>>>>>>>>>>>>>> mapper.
> >>>>>>>>>>>>>>>>>>>>>> Couldn't old gen also create its file near there? I
> >> feel
> >>>> like
> >>>>>>>>>>>>>>>>>>>>>> creating here
> >>>>>>>>>>>>>>>>>>>> seems
> >>>>>>>>>>>>>>>>>>>>>> un-natural.
> >>>>>>>>>>>>>>>>>>>>>>> - This is just an idea. Instead of adding
> >>>>> AllocateOldGenAt,
> >>>>>>>>>>>>>>>>>>>>>>> couldn't re-
> >>>>>>>>>>>>>>>>>> use
> >>>>>>>>>>>>>>>>>>>>>> AllocateHeapAt and add type option? e.g. 'all' or
> >>> 'old'
> >>>> or
> >>>>>>>>> 'off'.
> >>>>>>>>>>>>>>>>>>>>>>>>> If I do this in ReservedHeapSpace, all of the
> >>>> reserved
> >>>>>>>>>>>> memory
> >>>>>>>>>>>>>>>>>>>>>>>>> will
> >>>>>>>>>>>>>>>>>> be
> >>>>>>>>>>>>>>>>>>>>>> mapped to nv-dimm. Since here I want part of
> >>>> reserved
> >>>>>>>>>>>> memory
> >>>>>>>>>>>>> in
> >>>>>>>>>>>>>>>>>>>>>> nv-
> >>>>>>>>>>>>>>>>>>>> dimm,
> >>>>>>>>>>>>>>>>>>>>>> I can't use this directly, or easily modify it for our
> >>>> need.
> >>>>>>>>>>>>>>>>>>>>>>>>> I like the idea of re-using AllocateHeapAt. I
> >>>> however
> >>>>>>> not
> >>>>>>>>>>>> sure
> >>>>>>>>>>>>>>>>>>>>>>>>> how can a '-XX option handle extra option.( I
> >> am
> >>>>>>> thinking
> >>>>>>>>>> of
> >>>>>>>>>>>>>>>>>>>>>>>>> something similar to -Xbootclasspath/a(p))
> >> ========================================================
> >>>> src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.cpp
> >>>>>>>>>>>>>>>>>>>>>>> 34 // expand_by() is called to grow the heap. We
> >>>> grow
> >>>>>>> into
> >>>>>>>>>>>>>>>>>>>>>>> nvdimm
> >>>>>>>>>>>>>>>>>>>> now.
> >>>>>>>>>>>>>>>>>>>>>>> 35 // Dram regions are committed later as
> >> needed
> >>>>> during
> >>>>>>>>>>>>> mutator
> >>>>>>>>>>>>>>>>>> region
> >>>>>>>>>>>>>>>>>>>>>>> allocation or
> >>>>>>>>>>>>>>>>>>>>>>> 36 // when young list target length is determined
> >>>> after
> >>>>> gc
> >>>>>>>>>> cycle.
> >>>>>>>>>>>>>>>>>>>>>>> 37 uint
> >>>>>>>>> HeapRegionManagerForHeteroHeap::expand_by(uint
> >>>>>>>>>>>>>>>>>>>> num_regions,
> >>>>>>>>>>>>>>>>>>>>>>> WorkGang* pretouch_workers) {
> >>>>>>>>>>>>>>>>>>>>>>> 38 uint num_expanded =
> >>>>>>>>>>>> expand_nvdimm(MIN2(num_regions,
> >>>>>>>>>>>>>>>>>>>>>>> max_expandable_length() -
> >>>>> total_regions_committed()),
> >>>>>>>>>>>>>>>>>>>>>>> pretouch_workers);
> >>>>>>>>>>>>>>>>>>>>>>> 39 assert(total_regions_committed() <=
> >>>>>>>>>>>>>>>>>>>>>>> max_expandable_length(), "must be");
> >>>>>>>>>>>>>>>>>>>>>>> 40 return num_expanded;
> >>>>>>>>>>>>>>>>>>>>>>> 41 }
> >>>>>>>>>>>>>>>>>>>>>>> - I guess the only reason of not adjusting dram
> >> here
> >>>> is
> >>>>>>> that
> >>>>>>>>>> we
> >>>>>>>>>>>>>>>>>>>>>>> don't have information of 'is_old'? Looks a bit
> >>> weired
> >>>>> but
> >>>>>>> I
> >>>>>>>>>>>>>>>>>>>>>>> don't have any suggestion. (see more below at
> >>>>>>>>>>>>>>>>>>>>>>> allocate_free_region)
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> Yes, expand_by() is used to grow the heap at
> >>>>>>>>> initialization
> >>>>>>>>>> or
> >>>>>>>>>>>>>>>>>>>>>>>>> after
> >>>>>>>>>>>>>>>>>> full
> >>>>>>>>>>>>>>>>>>>>>> GC. Since young generation target length is
> >> decided
> >>> at
> >>>> a
> >>>>>>> later
> >>>>>>>>>>>>>>>>>>>>>> point, I
> >>>>>>>>>>>>>>>>>>>> expand
> >>>>>>>>>>>>>>>>>>>>>> in nv-dimm space here.
> >>>>>>>>>>>>>>>>>>>>>>>>> Later when young generation target length is
> >>>>>>>>> determined,
> >>>>>>>>>>>>>>>>>>>>>> adjust_dram_regions() is called to provision the
> >>> target
> >>>>>>>>>> number
> >>>>>>>>>>>> of
> >>>>>>>>>>>>>>>>>> regions.
> >> ========================================================
> >>>>>>>>>>>>>>>>>>>>>>> Comments from Stefan
> >>>>>>>>>>>>>>>>>>>>>>>
> >> ========================================================
> >>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> >>>>>>>>>>>>>>>>>>>>>>> -------------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>> 1642 if (AllocateOldGenAt != NULL) {
> >>>>>>>>>>>>>>>>>>>>>>> 1643 _is_hetero_heap = true;
> >>>>>>>>>>>>>>>>>>>>>>> 1644 max_byte_size *= 2;
> >>>>>>>>>>>>>>>>>>>>>>> 1645 }
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> I would like this to be abstracted out of
> >>>>> G1CollectedHeap,
> >>>>>>>>>> not
> >>>>>>>>>>>>>>>>>>>>>>> entirely sure how but I'm thinking something like
> >>>>> adding
> >>>>>>> a
> >>>>>>>>>>>>>>>>>>>>>>> G1HeteroCollectorPolicy which answers correctly
> >>> on
> >>>>> how
> >>>>>>>>>>>> much
> >>>>>>>>>>>>>>>>>> memory
> >>>>>>>>>>>>>>>>>>>>>>> needs to be reserved and how big the heap
> >> actually
> >>>> is.
> >>>>>>>>>>>>>>>>>>>>>>>>> I abstracted out this in
> >> G1HeteroCollectorPolicy.
> >>>>>>>>>>>>>>>>>>>>>>> 1668
> >>>>>>>>>>>> G1RegionToSpaceMapper::create_heap_mapper(g1_rs,
> >>>>>>>>>>>>>>>>>>>>>>> 1669 g1_rs.size(),
> >>>>>>>>>>>>>>>>>>>>>>> 1670 page_size,
> >>>>>>>>>>>>>>>>>>>>>>> 1671
> >>> HeapRegion::GrainBytes,
> >>>>>>>>>>>>>>>>>>>>>>> 1672 1,
> >>>>>>>>>>>>>>>>>>>>>>> 1673 mtJavaHeap);
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> This function could then also make use of the
> >> new
> >>>>> policy,
> >>>>>>>>>>>>>>>>>>>>>>> something
> >>>>>>>>>>>>>>>>>> like:
> >>>>>>>>>>>>>>>>>>>>>>> create_heap_mapper(g1_rs, _collector_policy,
> >>>>>>> page_size);
> >>>>>>>>>>>>>>>>>>>>>>>>> Since the mapper needs to map the reserved
> >>>>> memory,
> >>>>>>>>>> Since
> >>>>>>>>>>>>>>>>>>>>>> create_heap_mapper() does not need anything
> >> from
> >>>>>>>>>>>>>>>>>>>>>> collector_policy, I chose to keep the call
> >> unchanged.
> >>>>>>>>>>>>>>>>>>>>>>> 3925 if(g1h->is_hetero_heap()) {
> >>>>>>>>>>>>>>>>>>>>>>> 3926 if(!r->is_old()) {
> >>>>>>>>>>>>>>>>>>>>>>> ....
> >>>>>>>>>>>>>>>>>>>>>>> 3929 r->set_premature_old();
> >>>>>>>>>>>>>>>>>>>>>>> 3930 }
> >>>>>>>>>>>>>>>>>>>>>>> 3931 } else {
> >>>>>>>>>>>>>>>>>>>>>>> 3932 r->set_old();
> >>>>>>>>>>>>>>>>>>>>>>> 3933 }
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> So if I understand this correctly, premature_old
> is
> >>>> used
> >>>>>>>>> when
> >>>>>>>>>>>> we
> >>>>>>>>>>>>>>>>>>>>>>> get evac failures and we use it to force these
> >>> regions
> >>>> to
> >>>>>>> be
> >>>>>>>>>>>>>>>>>>>>>>> included in the next Mixed collection. I'm not
> >> sure
> >>>> this
> >>>>> is
> >>>>>>>>>>>>>>>>>>>>>>> needed, in fact I think one of the cool things with
> >>> nv-
> >>>>>>> dimm
> >>>>>>>>> is
> >>>>>>>>>>>>>>>>>>>>>>> that we can avoid evac failures. G1 normally
> >> needs
> >>> to
> >>>>>>> stop
> >>>>>>>>>>>>>>>>>>>>>>> handing out regions to promote to when there
> >> are
> >>>> no
> >>>>>>>>>> regions
> >>>>>>>>>>>>>>>>>>>>>>> left, but when using nv-dimm the old
> >>>>>>>>>>>>>>>>>> regions
> >>>>>>>>>>>>>>>>>>>>>>> come from another pool and we should be able
> >> to
> >>>>> avoid
> >>>>>>>>> this
> >>>>>>>>>>>>> case.
> >>>>>>>>>>>>>>>>>>>>>>>>> Yes! I had given this a thought. We can always
> >>>>> procure
> >>>>>>> a
> >>>>>>>>>>>> free
> >>>>>>>>>>>>>>>>>>>>>>>>> region
> >>>>>>>>>>>>>>>>>> in
> >>>>>>>>>>>>>>>>>>>>>> nv-dimm. However if we do this, during this GC
> >>> phase
> >>>>>>> there
> >>>>>>>>>>>> could
> >>>>>>>>>>>>>>>>>>>>>> be
> >>>>>>>>>>>>>>>>>> more
> >>>>>>>>>>>>>>>>>>>>>> regions committed in memory than Xmx.
> >>>>>>>>>>>>>>>>>>>>>>>>> This would mean heap memory usage
> >> increases
> >>> to
> >>>>>>> more
> >>>>>>>>>>>> than
> >>>>>>>>>>>>>>>>> 'Xmx'
> >>>>>>>>>>>>>>>>>>>>>> during some gc phases. Will this be ok?
> >>>>>>>>>>>>>>>>>>>>>>>>> But again in current implementation memory
> >>>> usage
> >>>>> is
> >>>>>>>>>> more
> >>>>>>>>>>>>>>> than
> >>>>>>>>>>>>>>>>>> Xmx
> >>>>>>>>>>>>>>>>>>>>>>>>> anyway, since I allocate Xmx memory in nv-
> >>> dimm
> >>>> at
> >>>>>>>>> start.
> >>>>>>>>>> I
> >>>>>>>>>>>> do
> >>>>>>>>>>>>>>>>>>>>>>>>> this
> >>>>>>>>>>>>>>>>>>>>>> because allocating in nv-dimm involves mapping
> to
> >> a
> >>>> file
> >>>>>>>>>>>> system,
> >>>>>>>>>>>>>>>>>>>>>> which becomes complicated if you expand/shrink
> >>> file
> >>>>>>>>>> whenever
> >>>>>>>>>>>>> you
> >>>>>>>>>>>>>>>>>>>>>> commit/uncommit regions. But I chose to keep
> this
> >>>> only
> >>>>> in
> >>>>>>>>>>>>>>>>>>>>>> G1regionToSpaceMapper and not design rest of
> >> the
> >>>>> code
> >>>>>>>>>> based
> >>>>>>>>>>>>> on
> >>>>>>>>>>>>>>>>> this.
> >>>>>>>>>>>>>>>>>>>>>>>>> I had to make an exception for Full GC where I
> >>>>> increase
> >>>>>>>>>> the
> >>>>>>>>>>>>>>>>>> committed
> >>>>>>>>>>>>>>>>>>>>>> regions in nv-dimm before start of GC, since we
> >>> need
> >>>> to
> >>>>>>>>> move
> >>>>>>>>>>>> all
> >>>>>>>>>>>>>>>>>> objects
> >>>>>>>>>>>>>>>>>>>> to
> >>>>>>>>>>>>>>>>>>>>>> old generation.
> >>>>>>>>>>>>>>>>>>>>>>>>> Let me know what you think, I like your idea
> >>> since
> >>>>> we
> >>>>>>>>>> don't
> >>>>>>>>>>>>>>>>>>>>>>>>> need to
> >>>>>>>>>>>>>>>>>>>> add
> >>>>>>>>>>>>>>>>>>>>>> more complexity for handling premature old
> >>> regions.
> >>>>>>>>>>>>>>>>>>>>>>> Thanks and regards,
> >>>>>>>>>>>>>>>>>>>>>>> Kishor
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>>>>>>>>>>>>> From: sangheon.kim at oracle.com
> >>>>>>>>>>>>>>>>>> [mailto:sangheon.kim at oracle.com]
> >>>>>>>>>>>>>>>>>>>>>>>> Sent: Monday, November 5, 2018 9:39 PM
> >>>>>>>>>>>>>>>>>>>>>>>> To: Kharbas, Kishor
> >> <kishor.kharbas at intel.com>;
> >>>>> Stefan
> >>>>>>>>>>>>>>>>>>>>>>>> Johansson <stefan.johansson at oracle.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>
> >>>>>>>>>>>>>>>>>>>>>>>> Cc: 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,
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> On 11/2/18 2:48 PM, Kharbas, Kishor wrote:
> >>>>>>>>>>>>>>>>>>>>>>>>> Thanks Stefan and Sangheon for your
> >>> comments.
> >>>>>>>>>>>>>>>>>>>>>>>>> As suggested, I ran jtreg for serviceability
> tests.
> >> I
> >>>> see
> >>>>>>>>>>>>>>>>>>>>>>>>> failures even without
> >>>>>>>>>>>>>>>>>>>>>>>> using the new flag.
> >>>>>>>>>>>>>>>>>>>>>>>>> Looks like things are breaking in mirror classes
> >>>>>>>>>>>>>>>>>>>>>>>> (jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.*)
> >>>> because
> >>>>> of
> >>>>>>>>>> new
> >>>>>>>>>>>>>>>>>>>>>>>> fields and/or method changes in JVM classes.
> >>>>>>>>>>>>>>>>>>>>>>>>> Is there a bkm for changing these mirror Java
> >>>> classes
> >>>>>>>>> when
> >>>>>>>>>>>> we
> >>>>>>>>>>>>>>>>>> change
> >>>>>>>>>>>>>>>>>>>>>>>>> JVM
> >>>>>>>>>>>>>>>>>>>>>>>> classes?
> >>>>>>>>>>>>>>>>>>>>>>>> Sorry I'm not aware of any better known
> >> method
> >>>>> than
> >>>>>>>>>> fixing
> >>>>>>>>>>>>>>>>>>>>>>>> one-by-
> >>>>>>>>>>>>>>>>>>>> one.
> >>>>>>>>>>>>>>>>>>>>>>>>> I am working on addressing your comments
> on
> >>> the
> >>>>>>> patch
> >>>>>>>>>>>> (had
> >>>>>>>>>>>>>>>>>>>>>>>>> been
> >>>>>>>>>>>>>>>>>>>>>>>> dragged into another project, so I am behind
> on
> >>>> this).
> >>>>>>>>>>>>>>>>>>>>>>>> Thanks for your update.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>>>>>>>>>>> Sangheon
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> Thanks
> >>>>>>>>>>>>>>>>>>>>>>>>> Kishor
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>>>>>>>>>>>>>>> From: Stefan Johansson
> >>>>>>>>>>>>>>> [mailto:stefan.johansson at oracle.com]
> >>>>>>>>>>>>>>>>>>>>>>>>>> Sent: Friday, October 26, 2018 7:32 AM
> >>>>>>>>>>>>>>>>>>>>>>>>>> To: Kharbas, Kishor
> >>> <kishor.kharbas 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>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Cc: 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'll start with a few general observations and
> >>> then
> >>>>>>> there
> >>>>>>>>>> are
> >>>>>>>>>>>>>>>>>>>>>>>>>> some specific code comments below.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> I think the general design looks promising,
> not
> >>>>> having
> >>>>>>>>> to
> >>>>>>>>>> set
> >>>>>>>>>>>>>>>>>>>>>>>>>> a fixed limit for what can end up on NV-
> dimm
> >> is
> >>>>>>> great.
> >>>>>>>>>>>>> Having
> >>>>>>>>>>>>>>>>>>>>>>>>>> a separate HeapRegionManager looks like a
> >>> good
> >>>>>>>>>>>> abstraction
> >>>>>>>>>>>>>>>>>>>>>>>>>> and
> >>>>>>>>>>>>>>>>>>>> when
> >>>>>>>>>>>>>>>>>>>>>>>>>> adding larger features like this that is really
> >>>>>>> important.
> >>>>>>>>>>>>>>>>>>>>>>>>>> I also agree with Sangheon that you should
> do
> >>>>> more
> >>>>>>>>>>>> testing,
> >>>>>>>>>>>>>>>>>>>>>>>>>> both with and without the featured turned
> >> on. I
> >>>>> also
> >>>>>>>>>>>>>>>>>>>>>>>>>> recommend you to build with pre- compiled
> >>>>> headers
> >>>>>>>>>>>>> disabled,
> >>>>>>>>>>>>>>>>>>>>>>>>>> to find places where includes have been
> >>>> forgotten.
> >>>>> To
> >>>>>>>>>>>>>>>>>>>>>>>>>> configure such build add --disable-
> >> precompiled-
> >>>>>>>>> headers
> >>>>>>>>>> to
> >>>>>>>>>>>>>>> your
> >>>>>>>>>>>>>>>>> configure call.
> >>>>>>>>>>>>>>>>>>>>>>>>>> On 2018-10-04 04:08, Kharbas, Kishor wrote:
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Hi all,
> >>>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Requesting review of the patch for
> allocating
> >>> old
> >>>>>>>>>>>> generation
> >>>>>>>>>>>>>>>>>>>>>>>>>>> of
> >>>>>>>>>>>>>>>>>> g1
> >>>>>>>>>>>>>>>>>>>>>>>>>>> gc on alternate memory devices such nv-
> >>> dimm.
> >>>>>>>>>>>>>>>>>>>>>>>>>>> The design of this implementation is
> >> explained
> >>>> on
> >>>>>>> the
> >>>>>>>>>> bug
> >>>>>>>>>>>>>>>>>>>>>>>>>>> page -
> >>>>>>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-
> >>>>> 8211425
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Please follow the parent issue here :
> >>>>>>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-
> >>>>>>> 8202286.
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Webrev:
> >>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00
> >>>> <http://cr.openjdk.java.net/%7Ekkharbas/8211425/webrev.00>
> >> src/hotspot/share/gc/g1/g1Allocator.inline.hpp
> >>>>>>>>>>>>>>>>>>>>>>>>>> ----------------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>> 100 size_t length = static_cast
> >>>>>>>>>>>>>>>>>>>>>>>>>> <G1CollectedHeap*>(Universe::heap())-
> >>>>>>>>>>>>>>>>>> max_reserved_capacity()
> >>>>>>>>>>>>>>>>>>>>>>>>>> ;
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> You can avoid the cast by using
> >>>>>>>>> G1CollectedHeap::heap()
> >>>>>>>>>>>>>>>>>>>>>>>>>> instead
> >>>>>>>>>>>>>>>>>> of
> >>>>>>>>>>>>>>>>>>>>>>>>>> Universe::heap().
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> >>>>>>>>>>>>>>>>>>>>>>>>>> -------------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1642 if (AllocateOldGenAt != NULL) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1643 _is_hetero_heap = true;
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1644 max_byte_size *= 2;
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1645 }
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> I would like this to be abstracted out of
> >>>>>>>>> G1CollectedHeap,
> >>>>>>>>>>>>>>>>>>>>>>>>>> not entirely sure how but I'm thinking
> >>> something
> >>>>> like
> >>>>>>>>>>>> adding
> >>>>>>>>>>>>>>>>>>>>>>>>>> a G1HeteroCollectorPolicy which answers
> >>>> correctly
> >>>>> on
> >>>>>>>>>> how
> >>>>>>>>>>>>>>>>> much
> >>>>>>>>>>>>>>>>>>>>>> memory
> >>>>>>>>>>>>>>>>>>>>>>>>>> needs to be reserved and how big the heap
> >>>>> actually
> >>>>>>> is.
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1668
> >>>>>>>>>>>>>>> G1RegionToSpaceMapper::create_heap_mapper(g1_rs,
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1669 g1_rs.size(),
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1670 page_size,
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1671
> >>>>> HeapRegion::GrainBytes,
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1672 1,
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1673 mtJavaHeap);
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> This function could then also make use of the
> >>>> new
> >>>>>>>>> policy,
> >>>>>>>>>>>>>>>>>> something
> >>>>>>>>>>>>>>>>>>>>>> like:
> >>>>>>>>>>>>>>>>>>>>>>>>>> create_heap_mapper(g1_rs,
> >> _collector_policy,
> >>>>>>>>>> page_size);
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1704 if (is_hetero_heap()) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1705 _hrm = new
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>> HeapRegionManagerForHeteroHeap((uint)((max_byte_size
> >>>>>>>>>>>>>>>>>>>>>>>>>> / 2) / HeapRegion::GrainBytes /*heap size as
> >>>> num
> >>>>> of
> >>>>>>>>>>>>>>> regions*/));
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1706 }
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1707 else {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1708 _hrm = new HeapRegionManager();
> >>>>>>>>>>>>>>>>>>>>>>>>>> 1709 }
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> This code could then become something like:
> >>>>>>>>>>>>>>>>>>>>>>>>>> _hrm =
> >>>>>>>>>>>>>>>>>
> HeapRegionManager::create_manager(_collector_policy)
> >>>>>>>>>>>>>>>>>>>>>>>>>> 3925 if(g1h->is_hetero_heap()) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 3926 if(!r->is_old()) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> ....
> >>>>>>>>>>>>>>>>>>>>>>>>>> 3929 r->set_premature_old();
> >>>>>>>>>>>>>>>>>>>>>>>>>> 3930 }
> >>>>>>>>>>>>>>>>>>>>>>>>>> 3931 } else {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 3932 r->set_old();
> >>>>>>>>>>>>>>>>>>>>>>>>>> 3933 }
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> So if I understand this correctly,
> >> premature_old
> >>> is
> >>>>>>> used
> >>>>>>>>>>>> when
> >>>>>>>>>>>>>>>>>>>>>>>>>> we get evac failures and we use it to force
> >> these
> >>>>>>> regions
> >>>>>>>>>> to
> >>>>>>>>>>>>>>>>>>>>>>>>>> be included in the next Mixed collection. I'm
> >>> not
> >>>>> sure
> >>>>>>>>> this
> >>>>>>>>>>>>>>>>>>>>>>>>>> is needed, in fact I think one of the cool
> things
> >>>> with
> >>>>>>>>>>>>>>>>>>>>>>>>>> nv-dimm is that we can avoid evac failures.
> >> G1
> >>>>>>> normally
> >>>>>>>>>>>>> needs
> >>>>>>>>>>>>>>>>>>>>>>>>>> to stop handing out regions to promote to
> >>> when
> >>>>>>> there
> >>>>>>>>>> are
> >>>>>>>>>>>>> no
> >>>>>>>>>>>>>>>>>>>>>>>>>> regions left, but when using nv-dimm the old
> >>>>> regions
> >>>>>>>>>> come
> >>>>>>>>>>>>>>>>>>>>>>>>>> from another pool and we should
> >>>>>>>>>>>>>>>>>> be
> >>>>>>>>>>>>>>>>>>>>>> able to avoid this case.
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1FullCollector.cpp
> >>>>>>>>>>>>>>>>>>>>>>>>>> -------------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>> 169 if (_heap->is_hetero_heap()) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 170 static_cast
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >> <HeapRegionManagerForHeteroHeap*>(_heap-
> >>>>>>>>>> _hrm)-
> >>>>>>>>>>>>>>>>>>>>>>>>>>> prepare_for_full_collection_start();
> >>>>>>>>>>>>>>>>>>>>>>>>>> 171 }
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Move this to:
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>> G1CollectedHeap::prepare_heap_for_full_collection()
> >>>>>>>>>>>>>>>>>>>>>>>>>> 185 if (_heap->is_hetero_heap()) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 186 static_cast
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >> <HeapRegionManagerForHeteroHeap*>(_heap-
> >>>>>>>>>> _hrm)-
> >>>>>>>>>>>>>>>>>>>>>>>>>>> prepare_for_full_collection_end();
> >>>>>>>>>>>>>>>>>>>>>>>>>> 187 }
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Move this to:
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>> G1CollectedHeap::prepare_heap_for_mutators()
> >>>>>>>>>>>>>>>>>>>>>>>>>> And if you make the prepare-functions
> virtual
> >>>> and
> >>>>>>> not
> >>>>>>>>>>>> doing
> >>>>>>>>>>>>>>>>>>>>>>>>>> anything on HeapRegionManager we can
> >> avoid
> >>>> the
> >>>>>>>>>> checks.
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1Policy.cpp
> >>>>>>>>>>>>>>>>>>>>>>>>>> ------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>> 223 if(_g1h->is_hetero_heap() &&
> >>>>>>>>>> (Thread::current()-
> >>>>>>>>>>>>>>>>>>>>>>> is_VM_thread()
> >>>>>>>>>>>>>>>>>>>>>>>>>> || Heap_lock->owned_by_self())) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 224 static_cast
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >> <HeapRegionManagerForHeteroHeap*>(_g1h-
> >>>>>>>> hrm())-
> >>> resize_dram_regions(_young_list_target_length,
> >>>>>>>>>>>>>>>>>>>>>>>>>> _g1h->workers());
> >>>>>>>>>>>>>>>>>>>>>>>>>> 225 }
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Feels like this code should be part of the
> >>>>>>>>>>>>>>>>>>>>>>>>>> prepare_for_full_collection_end() above, but
> >>> the
> >>>>>>>>>>>>>>>>>>>>>>>>>> _young_list_target_length isn't updated until
> >>>> right
> >>>>>>> after
> >>>>>>>>>>>>>>>>>>>>>>>>>> prepare_heap_for_mutators() so you might
> >>> need
> >>>>> to
> >>>>>>>>>>>>>>> restructure
> >>>>>>>>>>>>>>>>>> the
> >>>>>>>>>>>>>>>>>>>>>>>>>> code a bit more to make it fit.
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
> >>>>>>>>>>>>>>>>>>>>>>>>>> -------------------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>> Had to add these two includes to make it
> >>>> compile.
> >>>>>>>>>>>>>>>>>>>>>>>>>> #include "runtime/java.hpp"
> >>>>>>>>>>>>>>>>>>>>>>>>>> #include "utilities/formatBuffer.hpp"
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Please also clean out all code commented
> out.
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>> src/hotspot/share/gc/g1/heapRegionManager.hpp
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---------------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>> I agree with Sangheon, please group the
> >>>> members
> >>>>>>> that
> >>>>>>>>>>>>> should
> >>>>>>>>>>>>>>>>>>>>>>>>>> be protected and remember to update the
> >> init-
> >>>> list
> >>>>> for
> >>>>>>>>> the
> >>>>>>>>>>>>>>>>>> constructor.
> >>>>>>>>>>>>>>>>>>>>>>>>>> Also agree that the #else on #ifdef ASSERT
> >>> should
> >>>>> be
> >>>>>>>>>>>>> removed.
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionSet.cpp
> >>>>>>>>>>>>>>>>>>>>>>>>>> -----------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>> 240 bool started = false;
> >>>>>>>>>>>>>>>>>>>>>>>>>> 241 while (cur != NULL && cur-
> >>>>> hrm_index()
> >>>>> <=
> >>>>>>>>>> end) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 242 if (!started && cur->hrm_index()
> >>> =
> >>>>> start)
> >>>>>>> {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 243 started = true;
> >>>>>>>>>>>>>>>>>>>>>>>>>> 244 }
> >>>>>>>>>>>>>>>>>>>>>>>>>> 245 if(started) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 246 num++;
> >>>>>>>>>>>>>>>>>>>>>>>>>> 247 }
> >>>>>>>>>>>>>>>>>>>>>>>>>> 248 cur = cur->next();
> >>>>>>>>>>>>>>>>>>>>>>>>>> 249 }
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> To me this looks more complex than it is
> >>> because
> >>>>> of
> >>>>>>> the
> >>>>>>>>>>>>>>>>>>>>>>>>>> multiple conditions, what do you think
> about:
> >>>>>>>>>>>>>>>>>>>>>>>>>> while (cur != NULL) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> uint index = cur->hrm_index();
> >>>>>>>>>>>>>>>>>>>>>>>>>> if (index > end) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> break;
> >>>>>>>>>>>>>>>>>>>>>>>>>> } else if (index >= start) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> num++;
> >>>>>>>>>>>>>>>>>>>>>>>>>> }
> >>>>>>>>>>>>>>>>>>>>>>>>>> cur = cur->next();
> >>>>>>>>>>>>>>>>>>>>>>>>>> }
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/arguments.cpp
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2072 if(!FLAG_IS_DEFAULT(AllocateHeapAt)
> >>> &&
> >>>>>>>>>>>>>>>>>>>>>>>>>> !FLAG_IS_DEFAULT(AllocateOldGenAt)) {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2073
> >>> jio_fprintf(defaultStream::error_stream(),
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2074 "AllocateHeapAt and
> >>>>> AllocateOldGenAt
> >>>>>>>>>>>> cannot
> >>>>>>>>>>>>>>> be
> >>>>>>>>>>>>>>>>>> used
> >>>>>>>>>>>>>>>>>>>>>>>>>> together.\n");
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2075 status = false;
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2076 }
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2077
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2078 if
> >> (!FLAG_IS_DEFAULT(AllocateOldGenAt)
> >>>> &&
> >>>>>>>>>>>>>>> (UseSerialGC
> >>>>>>>>>>>>>>>>>> ||
> >>>>>>>>>>>>>>>>>>>>>>>>>> UseConcMarkSweepGC || UseEpsilonGC ||
> >>>>> UseZGC))
> >>>>>>> {
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2079
> >>> jio_fprintf(defaultStream::error_stream(),
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2080 "AllocateOldGenAt not supported
> for
> >>>>>>> selected
> >>>>>>>>>>>>>>> GC.\n");
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2081 status = false;
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2082 }
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> This code would fit much better in
> >>> GCArguments.
> >>>>>>> There
> >>>>>>>>>> is
> >>>>>>>>>>>>>>>>>>>>>>>>>> currently no method handling this case but
> >>>> please
> >>>>>>> add
> >>>>>>>>>>>>>>>>>>>> check_args_consistency().
> >>>>>>>>>>>>>>>>>>>>>>>>>> You can look at
> >>>>>>>>> CompilerConfig::check_args_consistency
> >>>>>>>>>>>> for
> >>>>>>>>>>>>>>>>>>>>>> inspiration.
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/globals.hpp
> >>>>>>>>>>>>>>>>>>>>>>>>>> -------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>> 2612 experimental(uintx,
> >>>>>>>>> G1YoungExpansionBufferPerc,
> >>>>>>>>>>>> 10,
> >>>>>>>>>>>>>>>>>>>>>>>>>> Move to g1_globals.hpp and call it
> >>>>>>>>>>>>>>>>>> G1YoungExpansionBufferPercent.
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>> src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.*pp
> >>>>>>>>>>>>>>>>>>>>>>>>>> ----------------------------------------------------------
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Haven't reviewed this code in detail yet but
> >> will
> >>>> do
> >>>>> in
> >>>>>>> a
> >>>>>>>>>>>>>>>>>>>>>>>>>> later
> >>>>>>>>>>>>>>>>>> review.
> >>>>>>>>>>>>>>>>>>>>>>>>>> One thought I have already is about the
> >> name,
> >>>>> what
> >>>>>>> do
> >>>>>>>>>>>> you
> >>>>>>>>>>>>>>>>>>>>>>>>>> think
> >>>>>>>>>>>>>>>>>>>>>> about:
> >>>>>>>>>>>>>>>>>>>>>>>>>> HeterogeneousHeapRegionManager
> >>>>>>>>>>>>>>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>>>>>>>>>>>>> Stefan
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Testing : Passed tier1_gc and tier1_runtime
> >>> jtret
> >>>>>>> tests.
> >>>>>>>>>>>>>>>>>>>>>>>>>>> I added extra options
> >>>>>>>>>>>>>>>>>>>>>>>>>>> "-XX:+UnlockExperimentalVMOptions -
> >>>>>>>>>>>>>>>>>>>>>>>>>> XX:AllocateOldGenAt=/home/Kishor"
> >>>>>>>>>>>>>>>>>>>>>>>>>>> to each test. There are some tests which I
> >> had
> >>> to
> >>>>>>>>>> exclude
> >>>>>>>>>>>>>>>>>>>>>>>>>>> since they won't work with this flag.
> >> Example:
> >>>>> tests
> >>>>>>> in
> >>>>>>>>>>>>>>>>>>>> ConcMarkSweepGC
> >>>>>>>>>>>>>>>>>>>>>>>>>>> which does not support this flag, tests
> >>> requiring
> >>>>>>>>>>>>>>>>>>>>>>>>>>> CompressedOops to be enabled,
> >>>>>>>>>>>>>>>>>>>>>>>>>> etc.
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks
> >>>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Kishor
> >>>>>>>>>>>>>>>>>>>>>>>>>>>
More information about the hotspot-gc-dev
mailing list