RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Kharbas, Kishor
kishor.kharbas at intel.com
Thu Dec 13 09:14:11 UTC 2018
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