RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC

Stefan Johansson stefan.johansson at oracle.com
Fri Dec 7 14:13:30 UTC 2018


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.java: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