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