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 02:06:58 UTC 2018
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