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