RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Stefan Johansson
stefan.johansson at oracle.com
Tue Dec 11 11:46:46 UTC 2018
Hi Kishor,
Comments on the webrev below. I'm still missing the code we discussed
yesterday that makes sure the DRAM part doesn't grow over the system
available RAM. I think this part is really important to make sure the
feature is robust.
On 2018-12-11 09:30, Kharbas, Kishor wrote:
> Hi Stefan, Sangheon,
>
> I have an updated and re-based webrev which triggers a full gc when we make available extra regions when we are out of old gc allocation regions. This avoids changing code in copy_to_survivor()
>
> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.09/
src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.cpp
-----------------------------------------------------
I would really like to avoid having all the static_cast calls in here
and to follow a pattern often used in HotSpot, I propose you add a
static manager() function in HeterogeneousHeapRegionManager which return
a casted version of the HRM. You can fetch the hrm by calling:
G1CollectedHeap::heap()->hrm();
You could then either use that in the policy or also add a member that
keeps the HeterogeneousHeapRegionManager and set it up in the init call.
Then you can use that in the policy like:
_manager->adjust_dram_regions((uint)young_list_target_length(),
_g1h->workers());
src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.hpp
-----------------------------------------------------
44 virtual bool need_to_perform_full_collection_after_partial();
I think a better name would be force_upgrade_to_full_gc().
src/hotspot/share/gc/g1/g1VMOperations.cpp
------------------------------------------
141 bool should_upgrade_to_full =
(!g1h->should_do_concurrent_full_gc(_gc_cause) &&
142
!g1h->has_regions_left_for_allocation()) ||
143
g1h->g1_policy()->need_to_perform_full_collection_after_partial();
To simplify reading this break it out to a separate method on
G1CollectedHeap called should_upgrade_to_full_gc(). And in that method
keep the conditions as simple as possible, basically something like:
if (policy->force_full)
return true;
else if (should_do_conc)
return false;
else if (has_regions)
return false;
return true;
Please double check my logic here.
src/hotspot/share/prims/whitebox.cpp
------------------------------------
You can use the new HeterogeneousHeapRegionManager::manager() here as well.
src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.cpp
----------------------------------------------------------
244 if(type.is_eden() || type.is_humongous()) {
245 if(has_borrowed_regions()) {
246 return NULL;
247 }
248 }
Add a comment that we want to prevent mutator allocations when we have
borrowed regions but the GC should still be able to allocate.
src/hotspot/share/gc/g1/g1FullCollector.cpp
-------------------------------------------
There is an added line here, please remove that one.
src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
-------------------------------------------------
26 #include "logging/log.hpp"
Not sorted appropriately. Please check all your files to make sure the
includes are sorted.
Thanks,
Stefan
> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.08_to_09/
>
> I am running exhaustive jtreg tests, will report the results tomorrow.
>
> Thanks
> Kishor
>
>> -----Original Message-----
>> From: Kharbas, Kishor
>> Sent: Friday, December 7, 2018 11:24 AM
>> To: Stefan Johansson <stefan.johansson at oracle.com>;
>> 'sangheon.kim at oracle.com' <sangheon.kim at oracle.com>
>> Cc: 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>;
>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Kharbas, Kishor
>> <kishor.kharbas at intel.com>
>> Subject: RE: RFR(M): 8211425: Allocation of old generation of java heap on
>> alternate memory devices - G1 GC
>>
>> Hi Stefan,
>> I used these commands to run jtreg. Thanks for starting the tests on your
>> end.
>>
>> jtreg -jdk:/home/kishor/jdk-tip-nvdimm -retain:fail -conc:2 -
>> w:./JT_nvdimm_WithoutFlag -r:./JT_nvdimm_WithoutFlag
>> .:tier1_old_nvdimm
>>
>> jtreg -jdk:/home/kishor/jdk-tip-nvdimm -retain:fail -conc:2 -javaoptions:"-
>> XX:+UnlockExperimentalVMOptions -XX:+UseG1GC -
>> XX:AllocateOldGenAt=/home/kishor" -w:./JT_nvdimm_WithoutFlag -
>> r:./JT_nvdimm_WithoutFlag .:tier1_old_nvdimm
>>
>> tier1_old_nvdimm = \
>> serviceability/ \
>> :tier1_gc \
>> :tier1_runtime \
>> :hotspot_appcds \
>> -gc/serial \
>> -gc/cms \
>> -gc/epsilon \
>> -gc/TestAllocateHeapAt.java \
>> -gc/TestAllocateHeapAtError.java \
>> -gc/TestAllocateHeapAtMultiple.java \
>> -gc/g1/TestFromCardCacheIndex.java \
>> -gc/metaspace/TestMetaspacePerfCounters.java \
>> -gc/metaspace/TestPerfCountersAndMemoryPools.java \
>> -runtime/Metaspace/PrintMetaspaceDcmd.java
>> -gc/g1/TestLargePageUseForAuxMemory.java
>>
>> Thanks,
>> Kishor
>>
>>> -----Original Message-----
>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>>> Sent: Friday, December 7, 2018 6:14 AM
>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>> 'sangheon.kim at oracle.com' <sangheon.kim at oracle.com>
>>> Cc: 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-
>> dev at openjdk.java.net>;
>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java heap on
>>> alternate memory devices - G1 GC
>>>
>>> Hi Kishor,
>>>
>>> On 2018-12-07 08:34, Kharbas, Kishor wrote:
>>>> Hi,
>>>>
>>>> JTreg regression testing shows 1 regression when we use patched JDK
>>> without flag
>>>> - runtime/appcds/cacheObject/ArchivedIntegerCacheTest.java
>>>>
>>>> I looked at the log, but don't seem to have a handle on this. Any pointers
>>> on how appCDS is affected by the patch? Error is,
>>>>
>>>> Exception in thread "main" java.lang.RuntimeException: FAILED. Value
>>> expected to be archived: -128 of type java.lang.Byte
>>>> at
>>>
>> CheckIntegerCacheApp.checkArchivedAsExpected(CheckIntegerCacheApp.jav
>>> a:109)
>>>> at CheckIntegerCacheApp.main(CheckIntegerCacheApp.java:73)
>>>> ]
>>>> exitValue = 1
>>> I can't reproduce this failure locally, do you run with any extra flags
>>> or how exactly are you running the test? I don't see how AppCDS is
>>> affected when the feature is turned of, when turned on AppCDS will not
>>> be used since CompressedOops are not used.
>>>
>>> I've started some benchmarking run with the feature turned of as well to
>>> make sure we don't see any regression in normal G1.
>>>
>>>>
>>>> And 3 regressions when additional flag - AllocateOldGenAt is specified. I
>>> have an idea what is going on here.
>>>> +gc/TestSoftReferencesBehaviorOnOOME.java
>>>> +gc/g1/TestGreyReclaimedHumongousObjects.java
>>>> +gc/g1/humongousObjects/TestNoAllocationsInHRegions.java
>>>>
>>>
>>> These should be addressed before the changes can be pushed. I will try
>>> to start some remote test jobs with the patch enabled over the weekend.
>>>
>>> Thanks,
>>> Stefan
>>>
>>>> Thanks
>>>> Kishor
>>>>
>>>>> -----Original Message-----
>>>>> From: Kharbas, Kishor
>>>>> Sent: Thursday, December 6, 2018 5:24 PM
>>>>> To: Stefan Johansson <stefan.johansson at oracle.com>;
>>>>> 'sangheon.kim at oracle.com' <sangheon.kim at oracle.com>
>>>>> Cc: 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-
>>> dev at openjdk.java.net>;
>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>>>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Kharbas,
>>> Kishor
>>>>> <kishor.kharbas at intel.com>
>>>>> Subject: RE: RFR(M): 8211425: Allocation of old generation of java heap
>> on
>>>>> alternate memory devices - G1 GC
>>>>>
>>>>> Hi Stefan, Sangheon,
>>>>>
>>>>> My replies are inline.
>>>>> Updated webrev -
>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.08/
>>>>>
>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.07_to_08
>>>>>
>>>>> Thank you.
>>>>> Kishor
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>>>>>> Sent: Thursday, December 6, 2018 6:03 AM
>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>>>>> 'sangheon.kim at oracle.com' <sangheon.kim at oracle.com>
>>>>>> Cc: 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-
>>>>> dev at openjdk.java.net>;
>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>>>>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java heap
>>> on
>>>>>> alternate memory devices - G1 GC
>>>>>>
>>>>>> Hi Kishor,
>>>>>>
>>>>>> On 2018-12-06 10:05, Kharbas, Kishor wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I have new webrev (removed Parallel GC part, but couldn't get rid of
>>> files
>>>>>> showing up with 0 lines changed) -
>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.07
>>>>>>> Incremental webrev -
>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.06_to_07/
>>>>>> One new comment here and then the replies to your fixes are below.
>>>>>> src/hotspot/share/gc/g1/heapRegionManager.cpp
>>>>>> ---------------------------------------------
>>>>>> 71 HeapRegionManager*
>>>>>> HeapRegionManager::create_manager(G1CollectedHeap* heap,
>>>>>> CollectorPolicy* policy) {
>>>>>> 72 if (static_cast<G1CollectorPolicy*>(policy)->is_hetero_heap()) {
>>>>>> 73 return new
>>>>>> HeterogeneousHeapRegionManager((uint)(policy-
>>>> max_heap_byte_size() /
>>>>>> HeapRegion::GrainBytes) /*heap size as num of regions*/);
>>>>>> 74 }
>>>>>>
>>>>>> If you pass in a G1CollectorPolicy you can avoid the cast.
>>>>>>
>>>>> [Kharbas, Kishor] Thanks.
>>>>>>>
>>>>>>> I am replying to Sangheon's comments inline, and have copied below
>>>>>> Stefan's comments from previous two emails below with my replies.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> =====================================================
>>>>>>> Thanks for providing a separate webrev for this.
>>>>>>>
>>>>>>> I'm not sure I understand why we need a special region type. What is
>>> the
>>>>>> problem of having Old regions in DRAM, if we have an evacuation
>>> failure?
>>>>>>> I understand that it is not what we want with this feature, but by
>>> allowing
>>>>>> to go over Xmx during the GC we should avoid most occurrences of it. I
>>> can
>>>>>> happen during Mixed collections, but if it does we are probably close to
>>> run
>>>>>> into a Full GC.
>>>>>>>
>>>>>>>>> With the newer way (borrowing regions) we do not need special
>>> region
>>>>>> type. I removed it from latest webrev.
>>>>>> Great, thanks.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> =====================================================
>>>>>>> Here are my comments on the patch:
>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>>>>>>> -------------------------------------------
>>>>>>> 4558 if (new_alloc_region == NULL && is_hetero_heap() &&
>>> !is_survivor)
>>>>> {
>>>>>>> 4559 // if it is a heterogenous heap, we can always copy objects
>>>>>>> from young generation even if
>>>>>>> 4560 // old generation is full. We can borrow (or pre-allocate)
>>>>>>> regions in nv-dimm to serve as allocation region.
>>>>>>> 4561 new_alloc_region =
>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(hrm())-
>>>>>>> borrow_old_region_for_gc();
>>>>>>> 4562 assert(new_alloc_region != NULL, "We should get a non-null
>>>>>>> region");
>>>>>>> 4563 }
>>>>>>>
>>>>>>> This logic should live in
>>>>>>> HeterogeneousHeapRegionManager::allocate_free_region(). This
>>>>> function
>>>>>> should return NULL only when we are really out of regions (that is no
>>>>> regions
>>>>>> can be borrowed).
>>>>>>>
>>>>>>>>> allocate_free_region() is called by mutators too. Humongous calls
>>> this
>>>>>> function with 'old' = true. So we won't be able to differentiate whether
>>> the
>>>>>> call is for gc old region allocation or for humongous.
>>>>>>>>> Is it a good idea to add a new hrm method to be called for gc
>>> allocation
>>>>>> and override it in HeterogeneousHeapRegionManager?
>>>>>> I think instead changing G1CollectedHeap::new_region() and
>>>>>> HeapRegionManager::allocate_free_region() to take a HeapRegionType
>>>>>> instead of a bool is better. Then the different callers can better pass
>>>>>> in what they really are.
>>>>>>
>>>>>> I think the HeapRegionType needs to be extended a bit to handle this,
>>>>>> I'm thinking something like:
>>>>>> http://cr.openjdk.java.net/~sjohanss/8211425/HeapRegionType-for-
>>>>>> new_region/
>>>>>>
>>>>> [Kharbas, Kishor] Thank you. I liked your idea and used it for the
>>>>> implementation. Now there is no call to borrow_old_region_for_gc()
>> from
>>>>> outside, allocate_free_region() handles this.
>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> =====================================================
>>>>>>> src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
>>>>>>> ------------------------------------------------
>>>>>>> 227 // Third clause: if it is a heterogenous heap, we borrow
>> regions
>>>>>>> from nv-dimm for old gc allocation.
>>>>>>> 228 // This region can only be used to evacate young objects, so
>> we
>>>>>>> check the state of the object being copied.
>>>>>>> 229 if (_old_gen_is_full && dest_state.is_old() ||
>>>>>>> 230 (_g1h->is_hetero_heap() && state.is_old() &&
>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(_g1h->hrm())-
>>>>>>> is_old_full())
>>>>>>> ) {
>>>>>>>
>>>>>>> Instead of doing this extra clause I think we should make sure
>>>>>> _old_gen_is_full is correct even for hetero heaps. I don't really see why
>> it
>>>>>> shouldn't be already.
>>>>>>>
>>>>>>>>> 'this->_old_gen_is_full' is set true in allocate_in_next_plab() which
>> is
>>>>> the
>>>>>> final attempt for allocation in default code path.
>>>>>>>>> However when hetero heap is enabled, allocate_in_next_plab()
>> may
>>>>> not
>>>>>> be called since we allocate plab from borrowed region when old is full.
>>>>>>
>>>>>> Ok, I think I see your point now. If "borrowing" regions you only want
>>>>>> to use them for young objects. But adding the or clause here will mean
>>>>>> we will add extra checks for each evacuated object and I really want to
>>>>>> avoid that, since it will affect normal G1 quite a bit potentially.
>>>>>>
>>>>> [Kharbas, Kishor] I am able to avoid this extra check on fast path by
>> setting
>>>>> _old_gen_is_full flag after allocate_direct_or_new_plab().
>>>>> After allocate_direct_or_new_plab(), we check
>> whether
>>> we
>>>>> had to borrow any region for this allocation and set _old_gen_is_full
>>>>> accordingly.
>>>>> At the top, only when _old_gen_is_full is true, we
>> need
>>>>> additional check for whether source is young or old.
>>>>>
>>>>>> An alternative to this case would be to allow borrowing for all, and if
>>>>>> we after the GC realize we can't get down to Xmx number of regions
>>>>>> trigger a Full GC to solve this.
>>>>>>
>>>>>> When talking about the Full GC, I think there might be potential
>>>>>> problems there. How do you make sure all objects are moved to old.
>> The
>>>>>> normal algorithm move stuff to the bottom of the heap by compacting
>>>>>> chains of heap regions and tagging the regions as old after the
>>>>>> collection. For the nv-dimm case I can't see how you would handle
>>>>>> regions still in dram to move them into nv-dimm.
>>>>>>
>>>>> [Kharbas, Kishor] Yes, if we allow borrowing for all and complete the GC
>>> with
>>>>> extra regions, new allocations could increase the live set to more than
>>> Xmx.
>>>>> Even a Full GC would not help.
>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> =====================================================
>>>>>>>
>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>>>>>>> -------------------------------------------
>>>>>>> 1493 _is_hetero_heap(AllocateOldGenAt != NULL),
>>>>>>>
>>>>>>> Does G1CollectedHeap really need to know this, can't we always just
>> as
>>>>> the
>>>>>> G1CollectorPolicy? As a matter of fact, I think most places where we
>>> check
>>>>>> is_hetro_heap() a better solution is to use virtual calls or other
>>> abstractions
>>>>>> to do the right thing.
>>>>>>>
>>>>>>>>> I moved this to G1CollectorPolicy. I got rid of few places where
>>>>>> is_hetero_heap() is needed by using abstractions (specialized
>> G1Policy).
>>>>>> Looks good.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> =====================================================
>>>>>>> 1786 // Now we know the target length of young list. So adjust the
>>>>>>> heap to provision that many regions on dram.
>>>>>>> 1787 if (is_hetero_heap()) {
>>>>>>> 1788
>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(hrm())-
>>>>>>> adjust_dram_regions((uint)g1_policy()->young_list_target_length(),
>>>>>>> workers());
>>>>>>> 1789 }
>>>>>>>
>>>>>>> Here we have an example of what I mentioned above. Instead of
>> having
>>>>>> the check and the cast, is there a way we can make this happen every
>>> time
>>>>>> we update the young_list_target_length(). I see that we currently only
>> do
>>>>> this
>>>>>> at initialization and after full collections, why don't we have to do this
>>> after
>>>>>> each GC?
>>>>>>>
>>>>>>> Same comment for lines 2532-2535.
>>>>>>>
>>>>>>>>> young_list_target_length is updated from other places such as
>>>>>> g1YoungRemSetSamplingThread. In which case we have to acquire
>> heap
>>>>> lock
>>>>>> or check whether it’s the VMThread and lock is owned.
>>>>>>>>> I had adjust_dram_regions() call here earlier, but later moved it out
>>>>>> based on comments.
>>>>>>>>> In latest webrev, I call adjust_dram_regions() from G1Policy->init(),
>>>>>> record_collection_pause_end() and record_full_collection_end()
>>>>>>
>>>>>> I like this approach much better.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> =====================================================
>>>>>>> src/hotspot/share/gc/g1/heapRegionManager.cpp
>>>>>>> ---------------------------------------------
>>>>>>> 71 HeapRegionManager*
>>>>>>> HeapRegionManager::create_manager(G1CollectedHeap* heap,
>>>>>>> CollectorPolicy* policy) {
>>>>>>> 72 if (heap->is_hetero_heap()) {
>>>>>>> 73 return new
>>>>>>> HeterogeneousHeapRegionManager((uint)(policy-
>>>> max_heap_byte_size()
>>>>> /
>>>>>>> HeapRegion::GrainBytes) /*heap size as num of regions*/);
>>>>>>> 74 }
>>>>>>> 75 return new HeapRegionManager();
>>>>>>> 76 }
>>>>>>>
>>>>>>> As I mentioned above, I think the is_hetero check should be done on
>>> the
>>>>>> policy instead. Otherwise this is a place where this check is really
>> needed.
>>>>>>>
>>>>>>>>> Done.
>>>>>> Nice.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> =====================================================
>>>>>>>
>>>>>>> src/hotspot/share/gc/g1/heapRegionSet.hpp
>>>>>>> -----------------------------------------
>>>>>>> 198 uint num_of_regions_in_range(uint start, uint end) const;
>>>>>>>
>>>>>>> Was looking at the usage of this method and it is only used to get the
>>>>> length
>>>>>> of the dram free-list. Looking at the HeteroHeapRegionManager
>>> wouldn't it
>>>>>> be possible to add a second free-list, so we have one for dram and one
>>> for
>>>>>> nvdimm.
>>>>>>>
>>>>>>>>> I had given this a thought when I started the implementation, but I
>>>>> chose
>>>>>> to keep a single list so that I can re-use base class
>> (HeapRegionManager)
>>>>>> functions. For example, insert_into_free_list(),
>>>>>> allocate_free_regions_starting_at(), etc.
>>>>>>> Since the regions in free_list are already sorted, I did not have to
>>>>> change
>>>>>> a lot in free list management.
>>>>>> Ok, I'm fine with this if you think it makes your implementation easier.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> =====================================================
>>>>>>> src/hotspot/share/runtime/arguments.cpp
>>>>>>> ---------------------------------------
>>>>>>> 1625 if (AllocateOldGenAt != NULL) {
>>>>>>> 1626 FLAG_SET_ERGO(bool, UseCompressedOops, false);
>>>>>>> 1627 return;
>>>>>>> 1628 }
>>>>>>>
>>>>>>> Any reason this can't be done in GCArguments::initialize()?
>>>>>>>
>>>>>>>>> I moved it to GCArguments.
>>>>>> Perfect.
>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> =====================================================
>>>>>>>
>>>>>>> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp
>>>>>>> ----------------------------------------------------------
>>>>>>> 85 // Empty constructor, we'll initialize it with the initialize()
>>>>>>> method.
>>>>>>> 86 HeterogeneousHeapRegionManager(uint num_regions) :
>>>>>>> _max_regions(num_regions), _max_dram_regions(0),
>>>>>>> 87
>>>>>>> _max_nvdimm_regions(0), _start_index_of_nvdimm(0)
>>>>>>> 88 {}
>>>>>>>
>>>>>>> The member _total_commited_before_full_gc is not initialized.
>>>>>>>
>>>>>>>>> Fixed
>>>>>> Thumbs up.
>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> ======================================================
>>>>>>> 98 // Adjust dram_set to provision 'expected_num_regions'
>> regions.
>>>>>>> 99 void adjust_dram_regions(uint expected_num_regions,
>>>>> WorkGang*
>>>>>>> pretouch_workers);
>>>>>>>
>>>>>>> As mentioned I would turn this into a virtual method if it can't be
>>>>>> abstracted in another way. The WorkGang can be obtained by calling
>>>>>>> G1CollectedHeap::heap()->workers() so you don't need to pass it in.
>>> This
>>>>>> way we get a cleaner interface I think.
>>>>>>>
>>>>>>>>> Thanks.
>>>>>> Thanks for addressing my comments,
>>>>>> Stefan
>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>> ================================================================
>>>>>> ======================================================
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: sangheon.kim at oracle.com
>> [mailto:sangheon.kim at oracle.com]
>>>>>>>> Sent: Wednesday, December 5, 2018 3:02 PM
>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
>>>>>>>> <stefan.johansson at oracle.com>
>>>>>>>> Cc: 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-
>>>>>> dev at openjdk.java.net>;
>>>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>>>>>>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java
>>> heap
>>>>> on
>>>>>>>> alternate memory devices - G1 GC
>>>>>>>>
>>>>>>>> Hi Kishor,
>>>>>>>>
>>>>>>>> On 12/5/18 12:51 AM, Kharbas, Kishor wrote:
>>>>>>>>> Hi Stefan, Sangheon,
>>>>>>>>> I have another webrev update -
>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.06
>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05_to_06
>>>>>>>>>
>>>>>>>>> Changes are:
>>>>>>>>> 1. While testing the new evacuation handling mechanism, I
>> identified
>>>>>> some
>>>>>>>> issues and fixed them. The implementation has been tested by
>> tracing
>>>>> the
>>>>>>>> code path through the debugger.
>>>>>>>>> Based on the fix, here is an explanation for evacuation failure
>>>>>> handling -
>>>>>>>>> 1) The implementation is based on the axiom that for every
>>>>>> region
>>>>>>>> in dram where is a unavailable (not 'committed') region in nv-dimm.
>>> By
>>>>>>>> borrowing such regions, we can avoid evacuation failure for young
>>>>> objects
>>>>>>>> and thus avoid young->old region conversion.
>>>>>>>>> 2) We cannot use borrowed regions for old objects. Since in
>>>>>> worst
>>>>>>>> case for evacuating 'n' young and 'm' old regions we need 'n + m'
>>>>>> destination
>>>>>>>> regions. And we may need to borrow 'n+m' from nv-dimm. But we
>> can
>>>>>> only
>>>>>>>> guarantee 'n' borrowed regions.
>>>>>>>>> So we use borrowed region only for copying young
>>> objects,
>>>>>> else
>>>>>>>> we cannot avoid young->old conversion and will need a special
>> region
>>>>>> type
>>>>>>>> as in current implementation anyway.
>>>>>>>>> 3) When we cannot allocate a new region for
>>>>> OldGCAllocRegion,
>>>>>> we
>>>>>>>> borrow a region from hrm() to use as allocation region. We release
>>> (un-
>>>>>>>> commit) that many number of regions in adjust_dram_regions()
>> which
>>> is
>>>>>>>> called at end of GC.
>>>>>>>>> 4) So we may be in situation where there is valid old plab
>> and
>>>>>>>> OldGCAllocRegion but we can only use them when source is young.
>> So
>>>>> we
>>>>>>>> add another clause to the if() statement at the beginning of
>>>>>>>> copy_to_survivor_space() to check for this condition.
>>>>>>>>> For ease of review I extracted evacuation failure handling
>>>>>>>>> changes here -
>>>>>>>>>
>>>>>>
>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev_EvacuationFailure
>>>>>>>> I have some comments.
>>>>>>>>
>>>>>>>> 1. Have you test enough for evacuation failure handling case? i.e. it
>>>>> would
>>>>>> be
>>>>>>>> better to have tests for these cases.
>>>>>>>> e.g. How can we guarantee the total heap will be less than Xmx
>>> after
>>>>>> GC if
>>>>>>>> we followed 'borrowing region path'.
>>>>>>>> Couldn't shrinking dram fail after borrowing a region?
>>>>>>>> (heterogeneousHeapRegionManager.cpp:65)
>>>>>>>>
>>>>>>> [Kharbas, Kishor] Since we only copy objects from young generation,
>> we
>>>>>> will never borrow more than the length of young generation in the
>>>>> collection
>>>>>> set. Also we should completely evacuate the young generation, so
>> when
>>>>>> collection set is freed, we have that many free regions to shrink.
>>>>>>>
>>>>>>>> 2. Do we still need pre-mature old type after introducing 'borrowing
>>>>>>>> concept'? (Stefan also mentioned about this)
>>>>>>>>
>>>>>>> [Kharbas, Kishor] Done (I had kept it for removal later)
>>>>>>>
>>>>>>>> 3. (Again) Please don't merge ParallelGC patch(8211424) into G1
>>>>>>>> Patch(8211425) as those should be committed separately. And this
>>> email
>>>>>>>> thread is to review G1. :) i.e. you should commit identical reviewed
>>>>> patch,
>>>>>>>> not modifying it (to split Parallel part).
>>>>>>>>
>>>>>>> [Kharbas, Kishor] Done.
>>>>>>>> --------------------------------------
>>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>>>>>>>> 4561 new_alloc_region =
>>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(hrm())-
>>>>>>>>> borrow_old_region_for_gc();
>>>>>>>> - This can be moved to hetero hrm. Stefan also mentioned this.
>>>>>>>>
>>>>>>>>
>>>>>>> [Kharbas, Kishor] I could not find an existing method in hrm to handle
>>>>> this.
>>>>>> Two methods to allocate regions are allocate_free_region() and
>>>>>> allocate_free_regions_starting_at().
>>>>>>> Allocate_free_region() is also called for humongous
>>>>>> allocation with 'old' parameter so I can't issue a borrowed region in
>>> every
>>>>>> call.
>>>>>>> Please let me know if you think add a new hrm
>>> method
>>>>>> for use in this code and abstracting it, seems a good solution.
>>>>>>>> --------------------------------------
>>>>>>>> src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
>>>>>>>> 227 // Third clause: if it is a heterogenous heap, we borrow
>> regions
>>>>>>>> from nv-dimm for old gc allocation.
>>>>>>>> 228 // This region can only be used to evacate young objects, so
>> we
>>>>>>>> check the state of the object being copied.
>>>>>>>> - Something like this:
>>>>>>>> // Third clause: if it is a heterogenous heap, we can borrow
>> regions
>>>>>>>> from nv-dimm for old region allocation. And these regions should be
>>>>> only
>>>>>>>> used to evacuate young objects.
>>>>>>>>
>>>>>>> [Kharbas, Kishor] Done.
>>>>>>>
>>>>>>>> 229 if (_old_gen_is_full && dest_state.is_old() ||
>>>>>>>> 230 (_g1h->is_hetero_heap() && state.is_old() &&
>>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(_g1h->hrm())-
>>>>>>>>> is_old_full())
>>>>>>>> ) {
>>>>>>>> - It would be better to have another parenthesis at line 229.
>>>>>>>>
>>>>>>>>
>>>>>>> [Kharbas, Kishor] Done.
>>>>>>>
>>>>>>>> --------------------------------------
>>>>>>>> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.cpp
>>>>>>>> 65 uint num = shrink_dram(_no_borrowed_regions);
>>>>>>>> 66 assert(num == _no_borrowed_regions, "Should be able to
>>>>>>>> uncommit the number of regions borrowed for evacuation failure
>>>>>> handling");
>>>>>>>> - Couldn't we fail at line 65?
>>>>>>>>
>>>>>>> [Kharbas, Kishor] Replied above.
>>>>>>>>
>>>>>>>> --------------------------------------
>>>>>>>> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp
>>>>>>>> 45 bool _is_old_full;
>>>>>>>> - This name looks mis-leading. It sets true when we temporarily
>>> borrow
>>>>> a
>>>>>>>> region from nvdimm during GC. To align with
>>> '_no_borrowed_regions',
>>>>>>>> something like 'has_borrowed_regions'?
>>>>>>>>
>>>>>>> [Kharbas, Kishor] Done.
>>>>>>>> 135 bool is_old_full();
>>>>>>>> - 'const'?
>>>>>>>>
>>>>>>> [Kharbas, Kishor] Replace this method with has_borrowed_regions().
>>>>>>>>
>>>>>>>>> 2. There is a fix for incorrect handling of humongous allocations
>>> under
>>>>>>>> certain situations.
>>>>>>>>> 3. My jtreg tests are running, will provide the results when they
>>> finish.
>>>>>>>> I guess you are running serviceability as well?
>>>>>>>>
>>>>>>> [Kharbas, Kishor] Yes I have added them.
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Sangheon
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Let me know your feedback.
>>>>>>>>>
>>>>>>>>> Thank you.
>>>>>>>>> Kishor
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Kharbas, Kishor
>>>>>>>>>> Sent: Monday, December 3, 2018 11:48 PM
>>>>>>>>>> To: sangheon.kim at oracle.com; Stefan Johansson
>>>>>>>>>> <stefan.johansson at oracle.com>
>>>>>>>>>> Cc: 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-
>>>>>>>> dev at openjdk.java.net>;
>>>>>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>>>>>>>>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>;
>>> Kharbas,
>>>>>>>> Kishor
>>>>>>>>>> <kishor.kharbas at intel.com>
>>>>>>>>>> Subject: RE: RFR(M): 8211425: Allocation of old generation of java
>>>>> heap
>>>>>>>> on
>>>>>>>>>> alternate memory devices - G1 GC
>>>>>>>>>>
>>>>>>>>>> Hi Sangheon and Stefan,
>>>>>>>>>>
>>>>>>>>>> I have updated webrev for this feature,
>>>>>>>>>>
>>>>>>>>>> 1. Webrev with Stefan's comments on Parallel GC (abstraction
>>> related
>>>>>>>>>> comments) :
>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03_to_04
>>>>>>>>>>
>>>>>>>>>> 2. Webrev with evacuation failure handling :
>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.05
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.04_to_05
>>>>>>>>>> Testing underway.
>>>>>>>>>>
>>>>>>>>>> 3. Explanation of evacuation failure handling:
>>>>>>>>>> 1) The implementation is based on the axiom that for
>> every
>>>>>> region
>>>>>>>> in
>>>>>>>>>> dram where is a unavailable (not 'committed') region in nv-dimm.
>>> By
>>>>>>>>>> borrowing such regions, we can avoid evacuation failure for young
>>>>>>>> objects
>>>>>>>>>> and thus avoid young->old region conversion.
>>>>>>>>>> 2) We cannot use borrowed regions for old objects. Since
>> in
>>>>>> worst
>>>>>>>>>> case for evacuating 'n' young and 'm' old regions we need 'n + m'
>>>>>>>> destination
>>>>>>>>>> regions. And we may need to borrow 'n+m' from nv-dimm. But we
>>>>> can
>>>>>>>> only
>>>>>>>>>> guarantee 'n' borrowed regions.
>>>>>>>>>> So we use borrowed region only for copying young
>>> objects,
>>>>>> else
>>>>>>>> we
>>>>>>>>>> cannot avoid young->old conversion and will need a special region
>>>>> type
>>>>>> as
>>>>>>>> in
>>>>>>>>>> current implementation anyway.
>>>>>>>>>> 3) When we cannot allocate a new region for
>>>>> OldGCAllocRegion,
>>>>>>>> we
>>>>>>>>>> borrow a region from hrm() to use as allocation region. We sort of
>>>>>>>> release
>>>>>>>>>> that many number of regions in adjust_dram_regions() which is
>>> called
>>>>>> at
>>>>>>>> end
>>>>>>>>>> of GC.
>>>>>>>>>> 4) When G1ParScanThreadState: _old_gen_is_full is true,
>>> we
>>>>>> only
>>>>>>>> let
>>>>>>>>>> young objects be allocated from the borrowed region. There might
>>> be
>>>>> a
>>>>>>>> race
>>>>>>>>>> condition here, when one thread enters plab_allocate() while
>> other
>>>>>> thread
>>>>>>>>>> determines and set _old_gen_is_full.
>>>>>>>>>> But we can tolerate this race condition.
>>>>>>>>>>
>>>>>>>>>> 4. I am testing all tier1 tests and appCDS tests with the combined
>>>>> patch.
>>>>>>>> Will
>>>>>>>>>> provide an update soon.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Kishor
>>>>>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: sangheon.kim at oracle.com
>>>>> [mailto:sangheon.kim at oracle.com]
>>>>>>>>>>> Sent: Sunday, December 2, 2018 2:49 PM
>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan
>>> Johansson
>>>>>>>>>>> <stefan.johansson at oracle.com>
>>>>>>>>>>> Cc: 'hotspot-gc-dev at openjdk.java.net'
>>>>>>>>>>> <hotspot-gc-dev at openjdk.java.net>;
>>>>>>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>>>>>>>>>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>>>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java
>>>>>>>>>>> heap on alternate memory devices - G1 GC
>>>>>>>>>>>
>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>
>>>>>>>>>>> On 11/29/18 12:19 PM, Kharbas, Kishor wrote:
>>>>>>>>>>>> Hi Sangheon,
>>>>>>>>>>>>
>>>>>>>>>>>> Thank you for reviewing the webrev.
>>>>>>>>>>>> 1. Based on your feedback I have newer webrev. There are
>> some
>>>>>>>>>>> comments inline about AppCDS (top of previous email).
>>>>>>>>>>>> 2. Webrev with your feedback on G1 GC patch :
>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.02/
>>>>>>>>>>>>
>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01_to_02/
>>>>>>>>>>> Webrev.02 looks good.
>>>>>>>>>>>
>>>>>>>>>>>> 3. I merged the Parallel GC patch with your latest comment,
>>>>>>>>>>>> consolidated jtreg tests and combined webrev is :
>>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.03
>>>>>>>>>>> I meant to work based on Parallel GC patch so that your G1 patch
>>>>>>>>>>> doesn't need to comment out Paralell GC test case at newly
>> added
>>>>>> tests.
>>>>>>>>>>>
>>>>>>>>>>> I only checked commented out parts of Parallel GC are enabled.
>>> This
>>>>>>>>>>> looks good too.
>>>>>>>>>>> I would expect that you completed running whole your
>> patches(G1
>>>>>> and
>>>>>>>>>>> Parallel GC) together without any failures.
>>>>>>>>>>>
>>>>>>>>>>>> 4. I moved the CSR to finalized. Thanks for your review. We will
>>>>>>>>>>>> wait to
>>>>>>>>>>> hear from CSR committee.
>>>>>>>>>>> Okay, I saw it is approved now.
>>>>>>>>>>>
>>>>>>>>>>>> 5. I am working on Stefan's latest feedback on handling of
>>>>>>>>>>>> evacuation
>>>>>>>>>>> failure.
>>>>>>>>>>> OK.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Sangheon
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Kishor
>>>>>>>>>>>>
>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>> From: sangheon.kim at oracle.com
>>>>>>>> [mailto:sangheon.kim at oracle.com]
>>>>>>>>>>>>> Sent: Tuesday, November 27, 2018 9:25 PM
>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan
>>>>> Johansson
>>>>>>>>>>>>> <stefan.johansson at oracle.com>
>>>>>>>>>>>>> Cc: 'hotspot-gc-dev at openjdk.java.net' <hotspot-gc-
>>>>>>>>>>> dev at openjdk.java.net>;
>>>>>>>>>>>>> 'Hotspot dev runtime' <hotspot-runtime-
>>> dev at openjdk.java.net>;
>>>>>>>>>>>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>>>>>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation of
>>> java
>>>>>>>>>>>>> heap
>>>>>>>>>>> on
>>>>>>>>>>>>> alternate memory devices - G1 GC
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 11/19/18 6:20 PM, Kharbas, Kishor wrote:
>>>>>>>>>>>>>> Hi Sangheon,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Here is the incremental webrev -
>>>>>>>>>>>>>>
>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00_to_01
>>>>>>>>>>>>> Thanks for the incremental webrev.
>>>>>>>>>>>>> Here are some comments for webrev.01.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. General comments:
>>>>>>>>>>>>> - Any update with AppCDS tests? (You mentioned this on your
>>>>>>>>>>>>> previous
>>>>>>>>>>> email)
>>>>>>>>>>>> [Kharbas, Kishor] I completed testing of AppCDS with : 1)
>>> original
>>>>>>>>>>>> code. 2)
>>>>>>>>>>> patch applied, but flag not used. 3) patch applied, flag set.
>>>>>>>>>>>> Test results at :
>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/JT_AppCDS
>>>>>>>>>>>> 1) and 2) both show "Test results:
>>>>>>>>>>>> passed: 120; failed: 1;
>>>>>>>>>>> error: 2"
>>>>>>>>>>>> 3) No tests are run, all are
>>>>>>>>>>>> skipped. Do you know if
>>>>>>>>>>> appCDS does not work with certain flags. I use jtreg flag
>>>>>>>>>>> -javaoptions:"- XX:+UnlockExperimentalVMOptions -
>>>>>>>>>> XX:AllocateOldGenAt=/home/kishor".
>>>>>>>>>>>>> - Please test with ParallelGC patch added. i.e. Applied both
>>>>>>>>>>>>> ParallelGC and
>>>>>>>>>>> G1
>>>>>>>>>>>>> parts, so that you don't need to comment out ParallelGC on
>> new
>>>>>> tests.
>>>>>>>>>>>>>
>>>>>>>>>>>> [Kharbas, Kishor] The tests pass with merged patch, testing both
>>>>>>>>>>>> ParallelGC
>>>>>>>>>>> and G1.
>>>>>>>>>>>>> 2. New tests related general comments:
>>>>>>>>>>>>> - As AllocateOldGenAt feature will not be tested regularly, new
>>>>>>>>>>>>> tests
>>>>>>>>>>> should
>>>>>>>>>>>>> be excluded by default.
>>>>>>>>>>>> [Kharbas, Kishor] How do I do this? Should I remove these test
>>> from
>>>>>>>>>>> TEST.groups?
>>>>>>>>>>>>> - New tests need filters for other unsupported GCs.
>>>>>>>>>>>>> e.g. @requires vm.gc=="null" | vm.gc.G1 | vm.gc.Parallel
>>>>>>>>>>>> [Kharbas, Kishor] Done.
>>>>>>>>>>>>> - Are there any reason for printing stdout in new tests? i.e.
>> looks
>>>>>>>>>>>>> like we don't need 'System.out.println(output.getStdout());'
>>>>>>>>>>>>>
>>>>>>>>>>>> [Kharbas, Kishor] not really, I checked that output is dumped to
>>>>>>>>>>>> stdout of
>>>>>>>>>>> jtreg anyway. So I removed this.
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeapHeap.hpp
>>>>>>>>>>>>> 45 #include "gc/g1/heapRegionManager.hpp"
>>>>>>>>>>>>> 46 #include
>>> "gc/g1/heterogeneousHeapRegionManager.hpp"
>>>>>>>>>>>>> 47 #include "gc/g1/heapRegionSet.hpp"
>>>>>>>>>>>>> - Line 46 should be below line 47.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectorPolicy.hpp
>>>>>>>>>>>>> 41 virtual size_t heap_reservation_size_bytes();
>>>>>>>>>>>>> - const?
>>>>>>>>>>>>>
>>>>>>>>>>>>> 45 size_t _heap_reservation_size_bytes;
>>>>>>>>>>>>> - Do we really need this variable? Just returning
>>>>>>>>>>>>> 2*_max_heap_byte_size seems enough. In this case, adding
>>>>> virtual
>>>>>>>>>>>>> size_t
>>>>>>>>>>>>> heap_reservation_size_bytes() seems enough.
>>>>>>>>>>>>> - Regarding naming, how about heap_reserved_size_bytes()?
>>>>>>>>>>>>>
>>>>>>>>>>>>> 51 size_t heap_reservation_size_bytes();
>>>>>>>>>>>>> - I would prefer to have 'virtual' for readability. I saw you
>>>>>>>>>>>>> reflected this
>>>>>>>>>>> kind
>>>>>>>>>>>>> of my comment in other places so asking here too.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
>>>>>>>>>>>>> 25 #include "logging/log.hpp"
>>>>>>>>>>>>> - precompiled.hpp comes first
>>>>>>>>>>>>>
>>>>>>>>>>>>> 180 return false;
>>>>>>>>>>>>> 181 log_error(gc, init)("Could not create file for Old
>>>>>>>>>>>>> generation at location %s", AllocateOldGenAt);
>>>>>>>>>>>>> - We never reach line 181 because of line 180? :)
>>>>>>>>>>>>>
>>>>>>>>>>>>> 196
>>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>> G1RegionToHeteroSpaceMapper::G1RegionToHeteroSpaceMapper(Reserve
>>>>>>>>>>>>> dSpace rs,
>>>>>>>>>>>>> - My previous comment was about splitting initialization
>>>>> part(which
>>>>>>>>>>>>> does actual file-heap mapping) from ctor, so that we can avoid
>>>>>>>>>>>>> failure during construction. After construction, call initialize
>>>>>>>>>>>>> method to do file mapping and then report any failures if
>> have.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 196
>>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>> G1RegionToHeteroSpaceMapper::G1RegionToHeteroSpaceMapper(Reserve
>>>>>>>>>>>>> dSpace rs,
>>>>>>>>>>>>> 197 size_t actual_size,
>>>>>>>>>>>>> 198 size_t page_size,
>>>>>>>>>>>>> 199 size_t alloc_granularity,
>>>>>>>>>>>>> 200 size_t commit_factor,
>>>>>>>>>>>>> 201 MemoryType type) :
>>>>>>>>>>>>> 202 G1RegionToSpaceMapper(rs, actual_size, page_size,
>>>>>>>>>>>>> alloc_granularity, commit_factor, type),
>>>>>>>>>>>>> 203 _num_committed_dram(0),
>>>>> _num_committed_nvdimm(0),
>>>>>>>>>>>>> _success(false) {
>>>>>>>>>>>>> - Looks like a bad alignment. I think parameters and
>> initialization
>>>>>>>>>>>>> list(parental class as well) should be distinguishable.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 295 delete mapper;
>>>>>>>>>>>>> - Existing issue(no action needed): current mapper code
>> doesn't
>>>>>>>>>>>>> have a destructor.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionManager.cpp
>>>>>>>>>>>>> 29 #include "gc/g1/heapRegionManager.inline.hpp"
>>>>>>>>>>>>> 30 #include
>>> "gc/g1/heterogeneousHeapRegionManager.hpp"
>>>>>>>>>>>>> 31 #include "gc/g1/heapRegionSet.inline.hpp"
>>>>>>>>>>>>> - Move line 30 after line 31.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionManager.hpp
>>>>>>>>>>>>>
>>>>>>>>>>>>> 117 FreeRegionList _free_list;
>>>>>>>>>>>>> 118 void make_regions_available(uint index, uint
>>> num_regions
>>>>> =
>>>>>>>>>>>>> 1,
>>>>>>>>>>>>> WorkGang* pretouch_gang = NULL);
>>>>>>>>>>>>> - New line between 117 and 118 please.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 130 static HeapRegionManager*
>>>>>>>> create_manager(G1CollectedHeap*
>>>>>>>>>>>>> heap,
>>>>>>>>>>>>> CollectorPolicy* policy);
>>>>>>>>>>>>> - I see build failure in solaris.
>>>>>>>>>>>>> open/src/hotspot/share/gc/g1/heapRegionManager.hpp", line
>>>>> 129:
>>>>>>>>>> Error:
>>>>>>>>>>>>> CollectorPolicy is not defined.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionType.cpp
>>>>>>>>>>>>> - Copyright update
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/gc/shared/gcArguments.cpp
>>>>>>>>>>>>>
>>>>>>>>>>>>> 68 "AllocateOldGenAt not supported for selected
>>> GC.\n");
>>>>>>>>>>>>> - "AllocateOldGenAt *is* not supported ..."
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/prims/whitebox.cpp
>>>>>>>>>>>>> 510 return (jlong)(Universe::heap()->base() +
>> start_region
>>>>>>>>>>>>> * HeapRegion::GrainBytes);
>>>>>>>>>>>>> - g1h->base() is same, isn't it? There are more lines that using
>>>>>>>>>>>>> Universe::heap()->base().
>>>>>>>>>>>>>
>>>>>>>>>>>>> 524 }
>>>>>>>>>>>>> 525 else {
>>>>>>>>>>>>> and
>>>>>>>>>>>>> 551 }
>>>>>>>>>>>>> 552 else {
>>>>>>>>>>>>> - Make at one line. i.e. '} else {'
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/runtime/arguments.cpp
>>>>>>>>>>>>> 1625 if (AllocateOldGenAt != NULL) {
>>>>>>>>>>>>> 1626 FLAG_SET_ERGO(bool, UseCompressedOops, false);
>>>>>>>>>>>>> 1627 return;
>>>>>>>>>>>>> 1628 }
>>>>>>>>>>>>> - Wrong alignment. Needs 2 spaces for line 1625~1628.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>> src/hotspot/share/runtime/globals.hpp
>>>>>>>>>>>>> 2606 experimental(ccstr, AllocateOldGenAt, NULL,
>>>>>>>>>>>>> \
>>>>>>>>>>>>> 2607 "Directory to use for allocating old
>>>>>>>>>>>>> generation")
>>>>>>>>>>>>> - How about moving AllocateOldGenAt around AllocateHeapAt
>>>>>> option.
>>>>>>>>>>>>> - Change the description similar to AllocateHeapAt as it
>> explains
>>>>>>>> better.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>>
>>> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.cpp
>>>>>>>>>>>>> 29 #include "gc/g1/heapRegionManager.inline.hpp"
>>>>>>>>>>>>> 30 #include
>>> "gc/g1/heterogeneousHeapRegionManager.hpp"
>>>>>>>>>>>>> 31 #include "gc/g1/heapRegionSet.inline.hpp"
>>>>>>>>>>>>> - Line 30 should be located after line 31.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>>
>>> src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.hpp
>>>>>>>>>>>>>
>>>>>>>>>>>>> #ifndef
>>>>>> SHARE_VM_GC_G1_HeterogeneousHeapRegionManager_HPP
>>>>>>>>>>>>> - Should be all uppercase letters.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ----------------------
>>>>>>>>>>>>>
>>> test/hotspot/jtreg/gc/8202286/TestAllocateOldGenAtMultiple.java
>>>>>>>>>>>>> 56 "-Xmx4g -Xms4g", // 4.
>>>>>>>>>>>>> With larger heap size (UnscaledNarrowOop not possible).
>>>>>>>>>>>>> 57 "-Xmx4g -Xms4g -XX:+UseLargePages", // 5.
>>>>>>>>>>>>> Set UseLargePages.
>>>>>>>>>>>>> 58 "-Xmx4g -Xms4g -XX:+UseNUMA" // 6. Set
>>>>>>>> UseNUMA.
>>>>>>>>>>>>> - It would be better to use smaller heap for test stability(even
>>>>>>>>>>>>> though this will not be tested regularly) unless you have any
>>> good
>>>>>>>>>> reason for it.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> I have also filed a CSR at
>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-
>>>>>>>>>>>>> 8214081
>>>>>>>>>>>>> I reviewed this CSR as well.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Sangheon
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thank you,
>>>>>>>>>>>>>> Kishor
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>> From: sangheon.kim at oracle.com
>>>>>>>>>> [mailto:sangheon.kim at oracle.com]
>>>>>>>>>>>>>>> Sent: Sunday, November 18, 2018 11:14 AM
>>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan
>>>>>> Johansson
>>>>>>>>>>>>>>> <stefan.johansson at oracle.com>; 'hotspot-gc-
>>>>>>>> dev at openjdk.java.net'
>>>>>>>>>>>>>>> <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev runtime'
>>>>>>>>>>>>>>> <hotspot- runtime-dev at openjdk.java.net>
>>>>>>>>>>>>>>> Cc: Viswanathan, Sandhya
>>> <sandhya.viswanathan at intel.com>
>>>>>>>>>>>>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation
>> of
>>>>>>>>>>>>>>> java heap
>>>>>>>>>>>>> on
>>>>>>>>>>>>>>> alternate memory devices - G1 GC
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Could you give us incremental webrev? It will help a lot for
>>>>>>>> reviewers.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Sangheon
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 11/16/18 6:35 PM, Kharbas, Kishor wrote:
>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I have an update to the webrev at :
>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.01
>>>>>>>>>>>>>>>> 1. Most of the comments from Sangheon and Stefan have
>>>>> been
>>>>>>>>>>>>> addressed.
>>>>>>>>>>>>>>> For ease of reading, I am copy-pasting below the comments
>>> not
>>>>>>>>>>>>>>> fixed
>>>>>>>>>>> or
>>>>>>>>>>>>> for
>>>>>>>>>>>>>>> which I have follow up.
>>>>>>>>>>>>>>>> 2. I also ran jtreg tests, with and without flag using test
>>>>>>>>>>>>>>>> group shown
>>>>>>>>>>>>> below
>>>>>>>>>>>>>>> (removed tests incompatible with this flag. Also, for testing,
>> I
>>>>>>>>>>>>>>> made - XX:+AllocateOldGenAt a no-op instead of error since
>>>>> many
>>>>>>>>>>>>>>> tests have
>>>>>>>>>>> sub-
>>>>>>>>>>>>>>> tests for all different GCs)
>>>>>>>>>>>>>>>> I see same number of failures (12) for original, patch
>>>>>>>>>>>>>>>> without flag and with flag (no new failures) 3. Tasks in
>>>>> progress
>>>>>>>> are :
>>>>>>>>>>>>>>>> - AppCDS tests.
>>>>>>>>>>>>>>>> - CSR filing.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> tier1_old_nvdimm = \
>>>>>>>>>>>>>>>> serviceability/ \
>>>>>>>>>>>>>>>> :tier1_gc \
>>>>>>>>>>>>>>>> :tier1_runtime \
>>>>>>>>>>>>>>>> -gc/serial \
>>>>>>>>>>>>>>>> -gc/parallel \
>>>>>>>>>>>>>>>> -gc/cms \
>>>>>>>>>>>>>>>> -gc/epsilon \
>>>>>>>>>>>>>>>> -gc/TestAllocateHeapAt.java \
>>>>>>>>>>>>>>>> -gc/TestAllocateHeapAtError.java \
>>>>>>>>>>>>>>>> -gc/TestAllocateHeapAtMultiple.java \
>>>>>>>>>>>>>>>> -gc/g1/TestFromCardCacheIndex.java \
>>>>>>>>>>>>>>>> -gc/metaspace/TestMetaspacePerfCounters.java \
>>>>>>>>>>>>>>>> -
>> gc/metaspace/TestPerfCountersAndMemoryPools.java
>>> \
>>>>>>>>>>>>>>>> -runtime/Metaspace/PrintMetaspaceDcmd.java
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>
>> ========================================================
>>>>>>>>>>>>>>>> Comments from Sangheon:
>>>>>>>>>>>>>>>>
>>>>>>>>>>
>> ========================================================
>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 173 void
>>>>>>>>>>
>>>>> G1RegionToHeteroSpaceMapper::map_nvdimm_space(ReservedSpace
>>>>>>>>>>>>>>>> rs) {
>>>>>>>>>>>>>>>> - Your previous change of adding AllocateHeapAt created
>> nv
>>>>> file
>>>>>>>>>>>>>>>> inside
>>>>>>>>>>> of
>>>>>>>>>>>>>>> ReservedHeapSpace but this-AllocateOldGenAt create inside
>>> of
>>>>>>>>>>> mapper.
>>>>>>>>>>>>>>> Couldn't old gen also create its file near there? I feel like
>>>>>>>>>>>>>>> creating here
>>>>>>>>>>>>> seems
>>>>>>>>>>>>>>> un-natural.
>>>>>>>>>>>>>>>> - This is just an idea. Instead of adding AllocateOldGenAt,
>>>>>>>>>>>>>>>> couldn't re-
>>>>>>>>>>> use
>>>>>>>>>>>>>>> AllocateHeapAt and add type option? e.g. 'all' or 'old' or
>> 'off'.
>>>>>>>>>>>>>>>>>> If I do this in ReservedHeapSpace, all of the reserved
>>>>> memory
>>>>>>>>>>>>>>>>>> will
>>>>>>>>>>> be
>>>>>>>>>>>>>>> mapped to nv-dimm. Since here I want part of reserved
>>>>> memory
>>>>>> in
>>>>>>>>>>>>>>> nv-
>>>>>>>>>>>>> dimm,
>>>>>>>>>>>>>>> I can't use this directly, or easily modify it for our need.
>>>>>>>>>>>>>>>>>> I like the idea of re-using AllocateHeapAt. I however not
>>>>> sure
>>>>>>>>>>>>>>>>>> how can a '-XX option handle extra option.( I am thinking
>>> of
>>>>>>>>>>>>>>>>>> something similar to -Xbootclasspath/a(p))
>>>>>>>>>>
>> ========================================================
>>>>>>>>>>>>>>>>
>>>>>> src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.cpp
>>>>>>>>>>>>>>>> 34 // expand_by() is called to grow the heap. We grow into
>>>>>>>>>>>>>>>> nvdimm
>>>>>>>>>>>>> now.
>>>>>>>>>>>>>>>> 35 // Dram regions are committed later as needed during
>>>>>> mutator
>>>>>>>>>>> region
>>>>>>>>>>>>>>>> allocation or
>>>>>>>>>>>>>>>> 36 // when young list target length is determined after gc
>>> cycle.
>>>>>>>>>>>>>>>> 37 uint
>> HeapRegionManagerForHeteroHeap::expand_by(uint
>>>>>>>>>>>>> num_regions,
>>>>>>>>>>>>>>>> WorkGang* pretouch_workers) {
>>>>>>>>>>>>>>>> 38 uint num_expanded =
>>>>> expand_nvdimm(MIN2(num_regions,
>>>>>>>>>>>>>>>> max_expandable_length() - total_regions_committed()),
>>>>>>>>>>>>>>>> pretouch_workers);
>>>>>>>>>>>>>>>> 39 assert(total_regions_committed() <=
>>>>>>>>>>>>>>>> max_expandable_length(), "must be");
>>>>>>>>>>>>>>>> 40 return num_expanded;
>>>>>>>>>>>>>>>> 41 }
>>>>>>>>>>>>>>>> - I guess the only reason of not adjusting dram here is that
>>> we
>>>>>>>>>>>>>>>> don't have information of 'is_old'? Looks a bit weired but I
>>>>>>>>>>>>>>>> don't have any suggestion. (see more below at
>>>>>>>>>>>>>>>> allocate_free_region)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Yes, expand_by() is used to grow the heap at
>> initialization
>>> or
>>>>>>>>>>>>>>>>>> after
>>>>>>>>>>> full
>>>>>>>>>>>>>>> GC. Since young generation target length is decided at a later
>>>>>>>>>>>>>>> point, I
>>>>>>>>>>>>> expand
>>>>>>>>>>>>>>> in nv-dimm space here.
>>>>>>>>>>>>>>>>>> Later when young generation target length is
>> determined,
>>>>>>>>>>>>>>> adjust_dram_regions() is called to provision the target
>>> number
>>>>> of
>>>>>>>>>>> regions.
>>>>>>>>>>
>> ========================================================
>>>>>>>>>>>>>>>> Comments from Stefan
>>>>>>>>>>>>>>>>
>>>>>>>>>>
>> ========================================================
>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>>>>>>>>>>>>>>>> -------------------------------------------
>>>>>>>>>>>>>>>> 1642 if (AllocateOldGenAt != NULL) {
>>>>>>>>>>>>>>>> 1643 _is_hetero_heap = true;
>>>>>>>>>>>>>>>> 1644 max_byte_size *= 2;
>>>>>>>>>>>>>>>> 1645 }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I would like this to be abstracted out of G1CollectedHeap,
>>> not
>>>>>>>>>>>>>>>> entirely sure how but I'm thinking something like adding a
>>>>>>>>>>>>>>>> G1HeteroCollectorPolicy which answers correctly on how
>>>>> much
>>>>>>>>>>> memory
>>>>>>>>>>>>>>>> needs to be reserved and how big the heap actually is.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I abstracted out this in G1HeteroCollectorPolicy.
>>>>>>>>>>>>>>>> 1668
>>>>> G1RegionToSpaceMapper::create_heap_mapper(g1_rs,
>>>>>>>>>>>>>>>> 1669 g1_rs.size(),
>>>>>>>>>>>>>>>> 1670 page_size,
>>>>>>>>>>>>>>>> 1671 HeapRegion::GrainBytes,
>>>>>>>>>>>>>>>> 1672 1,
>>>>>>>>>>>>>>>> 1673 mtJavaHeap);
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This function could then also make use of the new policy,
>>>>>>>>>>>>>>>> something
>>>>>>>>>>> like:
>>>>>>>>>>>>>>>> create_heap_mapper(g1_rs, _collector_policy, page_size);
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Since the mapper needs to map the reserved memory,
>>> Since
>>>>>>>>>>>>>>> create_heap_mapper() does not need anything from
>>>>>>>>>>>>>>> collector_policy, I chose to keep the call unchanged.
>>>>>>>>>>>>>>>> 3925 if(g1h->is_hetero_heap()) {
>>>>>>>>>>>>>>>> 3926 if(!r->is_old()) {
>>>>>>>>>>>>>>>> ....
>>>>>>>>>>>>>>>> 3929 r->set_premature_old();
>>>>>>>>>>>>>>>> 3930 }
>>>>>>>>>>>>>>>> 3931 } else {
>>>>>>>>>>>>>>>> 3932 r->set_old();
>>>>>>>>>>>>>>>> 3933 }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So if I understand this correctly, premature_old is used
>> when
>>>>> we
>>>>>>>>>>>>>>>> get evac failures and we use it to force these regions to be
>>>>>>>>>>>>>>>> included in the next Mixed collection. I'm not sure this is
>>>>>>>>>>>>>>>> needed, in fact I think one of the cool things with nv-dimm
>> is
>>>>>>>>>>>>>>>> that we can avoid evac failures. G1 normally needs to stop
>>>>>>>>>>>>>>>> handing out regions to promote to when there are no
>>> regions
>>>>>>>>>>>>>>>> left, but when using nv-dimm the old
>>>>>>>>>>> regions
>>>>>>>>>>>>>>>> come from another pool and we should be able to avoid
>> this
>>>>>> case.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Yes! I had given this a thought. We can always procure a
>>>>> free
>>>>>>>>>>>>>>>>>> region
>>>>>>>>>>> in
>>>>>>>>>>>>>>> nv-dimm. However if we do this, during this GC phase there
>>>>> could
>>>>>>>>>>>>>>> be
>>>>>>>>>>> more
>>>>>>>>>>>>>>> regions committed in memory than Xmx.
>>>>>>>>>>>>>>>>>> This would mean heap memory usage increases to more
>>>>> than
>>>>>>>>>> 'Xmx'
>>>>>>>>>>>>>>> during some gc phases. Will this be ok?
>>>>>>>>>>>>>>>>>> But again in current implementation memory usage is
>>> more
>>>>>>>> than
>>>>>>>>>>> Xmx
>>>>>>>>>>>>>>>>>> anyway, since I allocate Xmx memory in nv-dimm at
>> start.
>>> I
>>>>> do
>>>>>>>>>>>>>>>>>> this
>>>>>>>>>>>>>>> because allocating in nv-dimm involves mapping to a file
>>>>> system,
>>>>>>>>>>>>>>> which becomes complicated if you expand/shrink file
>>> whenever
>>>>>> you
>>>>>>>>>>>>>>> commit/uncommit regions. But I chose to keep this only in
>>>>>>>>>>>>>>> G1regionToSpaceMapper and not design rest of the code
>>> based
>>>>>> on
>>>>>>>>>> this.
>>>>>>>>>>>>>>>>>> I had to make an exception for Full GC where I increase
>>> the
>>>>>>>>>>> committed
>>>>>>>>>>>>>>> regions in nv-dimm before start of GC, since we need to
>> move
>>>>> all
>>>>>>>>>>> objects
>>>>>>>>>>>>> to
>>>>>>>>>>>>>>> old generation.
>>>>>>>>>>>>>>>>>> Let me know what you think, I like your idea since we
>>> don't
>>>>>>>>>>>>>>>>>> need to
>>>>>>>>>>>>> add
>>>>>>>>>>>>>>> more complexity for handling premature old regions.
>>>>>>>>>>>>>>>> Thanks and regards,
>>>>>>>>>>>>>>>> Kishor
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>>>> From: sangheon.kim at oracle.com
>>>>>>>>>>> [mailto:sangheon.kim at oracle.com]
>>>>>>>>>>>>>>>>> Sent: Monday, November 5, 2018 9:39 PM
>>>>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan
>>>>>>>>>>>>>>>>> Johansson <stefan.johansson at oracle.com>; 'hotspot-gc-
>>>>>>>>>>> dev at openjdk.java.net'
>>>>>>>>>>>>>>>>> <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev
>> runtime'
>>>>>>>>>>> <hotspot-
>>>>>>>>>>>>>>>>> runtime-dev at openjdk.java.net>
>>>>>>>>>>>>>>>>> Cc: Viswanathan, Sandhya
>>>>> <sandhya.viswanathan at intel.com>
>>>>>>>>>>>>>>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation
>>> of
>>>>>>>>>>>>>>>>> java heap on alternate memory devices - G1 GC
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 11/2/18 2:48 PM, Kharbas, Kishor wrote:
>>>>>>>>>>>>>>>>>> Thanks Stefan and Sangheon for your comments.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> As suggested, I ran jtreg for serviceability tests. I see
>>>>>>>>>>>>>>>>>> failures even without
>>>>>>>>>>>>>>>>> using the new flag.
>>>>>>>>>>>>>>>>>> Looks like things are breaking in mirror classes
>>>>>>>>>>>>>>>>> (jdk.hotspot.agent/sun.jvm.hotspot.gc.g1.*) because of
>>> new
>>>>>>>>>>>>>>>>> fields and/or method changes in JVM classes.
>>>>>>>>>>>>>>>>>> Is there a bkm for changing these mirror Java classes
>> when
>>>>> we
>>>>>>>>>>> change
>>>>>>>>>>>>>>>>>> JVM
>>>>>>>>>>>>>>>>> classes?
>>>>>>>>>>>>>>>>> Sorry I'm not aware of any better known method than
>>> fixing
>>>>>>>>>>>>>>>>> one-by-
>>>>>>>>>>>>> one.
>>>>>>>>>>>>>>>>>> I am working on addressing your comments on the patch
>>>>> (had
>>>>>>>>>>>>>>>>>> been
>>>>>>>>>>>>>>>>> dragged into another project, so I am behind on this).
>>>>>>>>>>>>>>>>> Thanks for your update.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> Sangheon
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>> Kishor
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>>>>>>>> From: Stefan Johansson
>>>>>>>> [mailto:stefan.johansson at oracle.com]
>>>>>>>>>>>>>>>>>>> Sent: Friday, October 26, 2018 7:32 AM
>>>>>>>>>>>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>> 'hotspot-
>>>>>> gc-
>>>>>>>>>>>>>>>>>>> dev at openjdk.java.net' <hotspot-gc-
>>>>> dev at openjdk.java.net>;
>>>>>>>>>>>>> 'Hotspot
>>>>>>>>>>>>>>>>> dev
>>>>>>>>>>>>>>>>>>> runtime' <hotspot-runtime-dev at openjdk.java.net>
>>>>>>>>>>>>>>>>>>> Cc: Viswanathan, Sandhya
>>>>>> <sandhya.viswanathan at intel.com>
>>>>>>>>>>>>>>>>>>> Subject: Re: RFR(M): 8211425: Allocation of old
>>> generation
>>>>> of
>>>>>>>>>>>>>>>>>>> java heap on alternate memory devices - G1 GC
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Hi Kishor,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I'll start with a few general observations and then there
>>> are
>>>>>>>>>>>>>>>>>>> some specific code comments below.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I think the general design looks promising, not having
>> to
>>> set
>>>>>>>>>>>>>>>>>>> a fixed limit for what can end up on NV-dimm is great.
>>>>>> Having
>>>>>>>>>>>>>>>>>>> a separate HeapRegionManager looks like a good
>>>>> abstraction
>>>>>>>>>>>>>>>>>>> and
>>>>>>>>>>>>> when
>>>>>>>>>>>>>>>>>>> adding larger features like this that is really important.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I also agree with Sangheon that you should do more
>>>>> testing,
>>>>>>>>>>>>>>>>>>> both with and without the featured turned on. I also
>>>>>>>>>>>>>>>>>>> recommend you to build with pre- compiled headers
>>>>>> disabled,
>>>>>>>>>>>>>>>>>>> to find places where includes have been forgotten. To
>>>>>>>>>>>>>>>>>>> configure such build add --disable-precompiled-
>> headers
>>> to
>>>>>>>> your
>>>>>>>>>> configure call.
>>>>>>>>>>>>>>>>>>> On 2018-10-04 04:08, Kharbas, Kishor wrote:
>>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Requesting review of the patch for allocating old
>>>>> generation
>>>>>>>>>>>>>>>>>>>> of
>>>>>>>>>>> g1
>>>>>>>>>>>>>>>>>>>> gc on alternate memory devices such nv-dimm.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> The design of this implementation is explained on the
>>> bug
>>>>>>>>>>>>>>>>>>>> page -
>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8211425
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Please follow the parent issue here :
>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8202286.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Webrev:
>>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00
>>>>>>>>>>>>>>>>>>>>
>>>>>>>> <http://cr.openjdk.java.net/%7Ekkharbas/8211425/webrev.00>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1Allocator.inline.hpp
>>>>>>>>>>>>>>>>>>> ----------------------------------------------
>>>>>>>>>>>>>>>>>>> 100 size_t length = static_cast
>>>>>>>>>>>>>>>>>>> <G1CollectedHeap*>(Universe::heap())-
>>>>>>>>>>> max_reserved_capacity()
>>>>>>>>>>>>>>>>>>> ;
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> You can avoid the cast by using
>> G1CollectedHeap::heap()
>>>>>>>>>>>>>>>>>>> instead
>>>>>>>>>>> of
>>>>>>>>>>>>>>>>>>> Universe::heap().
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>>>>>>>>>>>>>>>>>>> -------------------------------------------
>>>>>>>>>>>>>>>>>>> 1642 if (AllocateOldGenAt != NULL) {
>>>>>>>>>>>>>>>>>>> 1643 _is_hetero_heap = true;
>>>>>>>>>>>>>>>>>>> 1644 max_byte_size *= 2;
>>>>>>>>>>>>>>>>>>> 1645 }
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I would like this to be abstracted out of
>> G1CollectedHeap,
>>>>>>>>>>>>>>>>>>> not entirely sure how but I'm thinking something like
>>>>> adding
>>>>>>>>>>>>>>>>>>> a G1HeteroCollectorPolicy which answers correctly on
>>> how
>>>>>>>>>> much
>>>>>>>>>>>>>>> memory
>>>>>>>>>>>>>>>>>>> needs to be reserved and how big the heap actually is.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 1668
>>>>>>>> G1RegionToSpaceMapper::create_heap_mapper(g1_rs,
>>>>>>>>>>>>>>>>>>> 1669 g1_rs.size(),
>>>>>>>>>>>>>>>>>>> 1670 page_size,
>>>>>>>>>>>>>>>>>>> 1671 HeapRegion::GrainBytes,
>>>>>>>>>>>>>>>>>>> 1672 1,
>>>>>>>>>>>>>>>>>>> 1673 mtJavaHeap);
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> This function could then also make use of the new
>> policy,
>>>>>>>>>>> something
>>>>>>>>>>>>>>> like:
>>>>>>>>>>>>>>>>>>> create_heap_mapper(g1_rs, _collector_policy,
>>> page_size);
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 1704 if (is_hetero_heap()) {
>>>>>>>>>>>>>>>>>>> 1705 _hrm = new
>>>>>>>>>>>>>>>>>>>
>>> HeapRegionManagerForHeteroHeap((uint)((max_byte_size
>>>>>>>>>>>>>>>>>>> / 2) / HeapRegion::GrainBytes /*heap size as num of
>>>>>>>> regions*/));
>>>>>>>>>>>>>>>>>>> 1706 }
>>>>>>>>>>>>>>>>>>> 1707 else {
>>>>>>>>>>>>>>>>>>> 1708 _hrm = new HeapRegionManager();
>>>>>>>>>>>>>>>>>>> 1709 }
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> This code could then become something like:
>>>>>>>>>>>>>>>>>>> _hrm =
>>>>>>>>>> HeapRegionManager::create_manager(_collector_policy)
>>>>>>>>>>>>>>>>>>> 3925 if(g1h->is_hetero_heap()) {
>>>>>>>>>>>>>>>>>>> 3926 if(!r->is_old()) {
>>>>>>>>>>>>>>>>>>> ....
>>>>>>>>>>>>>>>>>>> 3929 r->set_premature_old();
>>>>>>>>>>>>>>>>>>> 3930 }
>>>>>>>>>>>>>>>>>>> 3931 } else {
>>>>>>>>>>>>>>>>>>> 3932 r->set_old();
>>>>>>>>>>>>>>>>>>> 3933 }
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> So if I understand this correctly, premature_old is used
>>>>> when
>>>>>>>>>>>>>>>>>>> we get evac failures and we use it to force these regions
>>> to
>>>>>>>>>>>>>>>>>>> be included in the next Mixed collection. I'm not sure
>> this
>>>>>>>>>>>>>>>>>>> is needed, in fact I think one of the cool things with
>>>>>>>>>>>>>>>>>>> nv-dimm is that we can avoid evac failures. G1 normally
>>>>>> needs
>>>>>>>>>>>>>>>>>>> to stop handing out regions to promote to when there
>>> are
>>>>>> no
>>>>>>>>>>>>>>>>>>> regions left, but when using nv-dimm the old regions
>>> come
>>>>>>>>>>>>>>>>>>> from another pool and we should
>>>>>>>>>>> be
>>>>>>>>>>>>>>> able to avoid this case.
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1FullCollector.cpp
>>>>>>>>>>>>>>>>>>> -------------------------------------------
>>>>>>>>>>>>>>>>>>> 169 if (_heap->is_hetero_heap()) {
>>>>>>>>>>>>>>>>>>> 170 static_cast
>>>>>>>>>>>>>>>>>>> <HeapRegionManagerForHeteroHeap*>(_heap-
>>> _hrm)-
>>>>>>>>>>>>>>>>>>>> prepare_for_full_collection_start();
>>>>>>>>>>>>>>>>>>> 171 }
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Move this to:
>>>>>>>>>>>>>>>>>>> G1CollectedHeap::prepare_heap_for_full_collection()
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 185 if (_heap->is_hetero_heap()) {
>>>>>>>>>>>>>>>>>>> 186 static_cast
>>>>>>>>>>>>>>>>>>> <HeapRegionManagerForHeteroHeap*>(_heap-
>>> _hrm)-
>>>>>>>>>>>>>>>>>>>> prepare_for_full_collection_end();
>>>>>>>>>>>>>>>>>>> 187 }
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Move this to:
>>>>>>>>>>>>>>>>>>> G1CollectedHeap::prepare_heap_for_mutators()
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> And if you make the prepare-functions virtual and not
>>>>> doing
>>>>>>>>>>>>>>>>>>> anything on HeapRegionManager we can avoid the
>>> checks.
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1Policy.cpp
>>>>>>>>>>>>>>>>>>> ------------------------------------
>>>>>>>>>>>>>>>>>>> 223 if(_g1h->is_hetero_heap() &&
>>> (Thread::current()-
>>>>>>>>>>>>>>>> is_VM_thread()
>>>>>>>>>>>>>>>>>>> || Heap_lock->owned_by_self())) {
>>>>>>>>>>>>>>>>>>> 224 static_cast
>>>>>>>>>>>>>>>>>>> <HeapRegionManagerForHeteroHeap*>(_g1h->hrm())-
>>>>>>>>>>>>>>>>>>>> resize_dram_regions(_young_list_target_length,
>>>>>>>>>>>>>>>>>>> _g1h->workers());
>>>>>>>>>>>>>>>>>>> 225 }
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Feels like this code should be part of the
>>>>>>>>>>>>>>>>>>> prepare_for_full_collection_end() above, but the
>>>>>>>>>>>>>>>>>>> _young_list_target_length isn't updated until right after
>>>>>>>>>>>>>>>>>>> prepare_heap_for_mutators() so you might need to
>>>>>>>> restructure
>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>> code a bit more to make it fit.
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
>>>>>>>>>>>>>>>>>>> -------------------------------------------------
>>>>>>>>>>>>>>>>>>> Had to add these two includes to make it compile.
>>>>>>>>>>>>>>>>>>> #include "runtime/java.hpp"
>>>>>>>>>>>>>>>>>>> #include "utilities/formatBuffer.hpp"
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Please also clean out all code commented out.
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionManager.hpp
>>>>>>>>>>>>>>>>>>> ---------------------------------------------
>>>>>>>>>>>>>>>>>>> I agree with Sangheon, please group the members that
>>>>>> should
>>>>>>>>>>>>>>>>>>> be protected and remember to update the init-list for
>> the
>>>>>>>>>>> constructor.
>>>>>>>>>>>>>>>>>>> Also agree that the #else on #ifdef ASSERT should be
>>>>>> removed.
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionSet.cpp
>>>>>>>>>>>>>>>>>>> -----------------------------------------
>>>>>>>>>>>>>>>>>>> 240 bool started = false;
>>>>>>>>>>>>>>>>>>> 241 while (cur != NULL && cur->hrm_index() <=
>>> end) {
>>>>>>>>>>>>>>>>>>> 242 if (!started && cur->hrm_index() >= start) {
>>>>>>>>>>>>>>>>>>> 243 started = true;
>>>>>>>>>>>>>>>>>>> 244 }
>>>>>>>>>>>>>>>>>>> 245 if(started) {
>>>>>>>>>>>>>>>>>>> 246 num++;
>>>>>>>>>>>>>>>>>>> 247 }
>>>>>>>>>>>>>>>>>>> 248 cur = cur->next();
>>>>>>>>>>>>>>>>>>> 249 }
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> To me this looks more complex than it is because of the
>>>>>>>>>>>>>>>>>>> multiple conditions, what do you think about:
>>>>>>>>>>>>>>>>>>> while (cur != NULL) {
>>>>>>>>>>>>>>>>>>> uint index = cur->hrm_index();
>>>>>>>>>>>>>>>>>>> if (index > end) {
>>>>>>>>>>>>>>>>>>> break;
>>>>>>>>>>>>>>>>>>> } else if (index >= start) {
>>>>>>>>>>>>>>>>>>> num++;
>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>> cur = cur->next();
>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/arguments.cpp
>>>>>>>>>>>>>>>>>>> ---------------------------------------
>>>>>>>>>>>>>>>>>>> 2072 if(!FLAG_IS_DEFAULT(AllocateHeapAt) &&
>>>>>>>>>>>>>>>>>>> !FLAG_IS_DEFAULT(AllocateOldGenAt)) {
>>>>>>>>>>>>>>>>>>> 2073 jio_fprintf(defaultStream::error_stream(),
>>>>>>>>>>>>>>>>>>> 2074 "AllocateHeapAt and AllocateOldGenAt
>>>>> cannot
>>>>>>>> be
>>>>>>>>>>> used
>>>>>>>>>>>>>>>>>>> together.\n");
>>>>>>>>>>>>>>>>>>> 2075 status = false;
>>>>>>>>>>>>>>>>>>> 2076 }
>>>>>>>>>>>>>>>>>>> 2077
>>>>>>>>>>>>>>>>>>> 2078 if (!FLAG_IS_DEFAULT(AllocateOldGenAt) &&
>>>>>>>> (UseSerialGC
>>>>>>>>>>> ||
>>>>>>>>>>>>>>>>>>> UseConcMarkSweepGC || UseEpsilonGC || UseZGC)) {
>>>>>>>>>>>>>>>>>>> 2079 jio_fprintf(defaultStream::error_stream(),
>>>>>>>>>>>>>>>>>>> 2080 "AllocateOldGenAt not supported for selected
>>>>>>>> GC.\n");
>>>>>>>>>>>>>>>>>>> 2081 status = false;
>>>>>>>>>>>>>>>>>>> 2082 }
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> This code would fit much better in GCArguments. There
>>> is
>>>>>>>>>>>>>>>>>>> currently no method handling this case but please add
>>>>>>>>>>>>> check_args_consistency().
>>>>>>>>>>>>>>>>>>> You can look at
>> CompilerConfig::check_args_consistency
>>>>> for
>>>>>>>>>>>>>>> inspiration.
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/runtime/globals.hpp
>>>>>>>>>>>>>>>>>>> -------------------------------------
>>>>>>>>>>>>>>>>>>> 2612 experimental(uintx,
>> G1YoungExpansionBufferPerc,
>>>>> 10,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Move to g1_globals.hpp and call it
>>>>>>>>>>> G1YoungExpansionBufferPercent.
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>> src/hotspot/share/gc/g1/heapRegionManagerForHeteroHeap.*pp
>>>>>>>>>>>>>>>>>>> ----------------------------------------------------------
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Haven't reviewed this code in detail yet but will do in a
>>>>>>>>>>>>>>>>>>> later
>>>>>>>>>>> review.
>>>>>>>>>>>>>>>>>>> One thought I have already is about the name, what do
>>>>> you
>>>>>>>>>>>>>>>>>>> think
>>>>>>>>>>>>>>> about:
>>>>>>>>>>>>>>>>>>> HeterogeneousHeapRegionManager
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>> Stefan
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Testing : Passed tier1_gc and tier1_runtime jtret tests.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> I added extra options
>>>>>>>>>>>>>>>>>>>> "-XX:+UnlockExperimentalVMOptions -
>>>>>>>>>>>>>>>>>>> XX:AllocateOldGenAt=/home/Kishor"
>>>>>>>>>>>>>>>>>>>> to each test. There are some tests which I had to
>>> exclude
>>>>>>>>>>>>>>>>>>>> since they won't work with this flag. Example: tests in
>>>>>>>>>>>>> ConcMarkSweepGC
>>>>>>>>>>>>>>>>>>>> which does not support this flag, tests requiring
>>>>>>>>>>>>>>>>>>>> CompressedOops to be enabled,
>>>>>>>>>>>>>>>>>>> etc.
>>>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Kishor
>>>>>>>>>>>>>>>>>>>>
>>>>>>>
More information about the hotspot-gc-dev
mailing list