RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Kharbas, Kishor
kishor.kharbas at intel.com
Wed Dec 12 08:49:37 UTC 2018
Hi Sangheon,
I have worked on your comments in webrev.10. Replying inline just to formally check all points as done.
> -----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