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 01:23:30 UTC 2018
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