RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
sangheon.kim at oracle.com
sangheon.kim at oracle.com
Thu Dec 13 22:30:58 UTC 2018
Hi Kishor,
On 12/13/18 2:14 AM, Kharbas, Kishor wrote:
> Missed one change earlier -
> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.13/
> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.12_to_13/
It was a bit confusing to review webrev.11, 12 and 13.
Here are some comments.
-----------------------------------------------
src/hotspot/share/gc/g1/g1Policy.cpp
86 delete _ihop_control;
- Need to add _young_gen_sizer as well here?
188 printf("Young gen sizer is %p\n", _young_gen_sizer);
- Remove this line.
-----------------------------------------------
src/hotspot/share/gc/g1/g1YoungGenSizer.cpp
- Copyright update
133 G1YoungGenSizer*
G1YoungGenSizer::create_gen_sizer(G1CollectorPolicy* policy) {
- I think you wanted to initialize _young_gen_sizer at initialization
list and use create_gen_sizer() method. You can do that if you pass
G1CollectorPolicy* when constructing G1Policy.
G1Policy::G1Policy(G1CollectorPolicy* policy, STWGCTimer* gc_timer) :
...
_young_gen_sizer(G1YoungGenSizer::create_gen_sizer(policy)),
...
G1Policy* G1Policy::create_policy(G1CollectorPolicy* policy, STWGCTimer*
gc_timer_stw) {
if (policy->is_hetero_heap()) {
return new G1HeterogeneousHeapPolicy(policy, gc_timer_stw);
} else {
return new G1Policy(policy, gc_timer_stw);
}
}
-----------------------------------------------
src/hotspot/share/gc/g1/g1HeterogeneousCollectorPolicy.cpp
38 log_info(gc, ergo)("Setting MaxNewSize to " SIZE_FORMAT "
which is 80%% of dram memory. Dram usage can be lowered by setting
MaxNewSize to a lower value", (size_t)reasonable_max);
- If you want to avoid hard-coded value of '80': "which is " SIZE_FORMAT
"%%" ... , MaxRamFractionForYoung * 100"
-----------------------------------------------
test/hotspot/jtreg/TEST.groups
-gc/{nvdimm}
- Remove '{}'.
- FYI, test groups will still work but will not exclude 'nvdimm' folder
because of parenthesis. I didn't mean to use 'gc/{nvdimm}', it was just
an example. :)
Thanks,
Sangheon
>
> Thanks
> Kishor
>
>> -----Original Message-----
>> From: Kharbas, Kishor
>> Sent: Thursday, December 13, 2018 1:14 AM
>> To: 'sangheon.kim at oracle.com' <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
>>
>> New webrev : http://cr.openjdk.java.net/~kkharbas/8211425/webrev.12/
>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.11_to_12/
>>
>> Some compilation errors were introduced in earlier webrev, fixed those and
>> some cosmetic changes.
>>
>> Thanks
>> Kishor
>>
>>> -----Original Message-----
>>> From: Kharbas, Kishor
>>> Sent: Wednesday, December 12, 2018 6:45 PM
>>> To: 'sangheon.kim at oracle.com' <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,
>>> Here is the complete re-based patch
>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.11/
>>>
>>>> -----Original Message-----
>>>> From: Kharbas, Kishor
>>>> Sent: Wednesday, December 12, 2018 6:07 PM
>>>> To: 'sangheon.kim at oracle.com' <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,
>>>> New incremental webrev and comments inline -
>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.10_to_11/
>>>> Will re-base the complete webrev can send it soon.
>>>>
>>>>> -----Original Message-----
>>>>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>>>>> Sent: Wednesday, December 12, 2018 4:22 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/12/18 12:49 AM, Kharbas, Kishor wrote:
>>>>>> Hi Sangheon,
>>>>>> I have worked on your comments in webrev.10. Replying inline just to
>>>>> formally check all points as done.
>>>>> Thanks for addressing all my comments.
>>>>> Here are some minor comment. As this review thread is going to the
>> end,
>>>>> I tried to review full webrev.
>>>>>
>>>>> - webrev.10 conflicts with the latest source code.
>>>>>
>>>>> ----------------------------------------------------------
>>>>> src/hotspot/share/gc/g1/jvmFlagConstraintsG1.cpp
>>>>> - My previous comment was based on my bad assumption about your
>>>> logic,
>>>>> sorry. I was thinking you need fast exit path if
>>>>> NewSize/MaxNewSize/(whatever options related to young gen size) are
>>>>> larger than physical RAM. So I recommended to add it on constraint
>>>>> function.
>>>>> If we need ergonomic value, definitely have to add on other place. e.g.
>>>>> collector policy. Ergonomically choosing value behavior seems better
>>>>> than exiting unless given options are conflicting.
>>>>> - Similar work done at Parallel GC patch(8211424) needs here too.
>>>> [Kharbas, Kishor] Yes I have G1HeterogeneousCollectorPolicy and
>>>> G1HeterogeneousYoungGenSizer to do similar checking as 8211424.
>>>>
>>>>> - Could you do some tests with young gen related flags added? I will
>>>>> try too.
>>>> [Kharbas, Kishor] I did some testing. Will do more.
>>>>
>>>>>
>>>>> ----------------------------------------------------------
>>>>> src/hotspot/share/prims/whitebox.cpp
>>>>> 524 return (jlong)g1h->base() +
>>>>> Universe::heap()->collector_policy()->max_heap_byte_size();
>>>>> - Use g1h instead of Universe::heap().
>>>>>
>>>>>
>>>>> ----------------------------------------------------------
>>>>> 2571 experimental(ccstr, AllocateOldGenAt,
>>>>> NULL, \
>>>>> 2572 "Path to the directoy where a temporary file will be
>>>>> " \
>>>>> 2573 "created to use as the backing store for old
>>>>> generation") \
>>>>> - We discussed to add somewhere about 'a temporary file will be
>> created
>>>>> for the size of Xmx'. Are you planning to add it to release note?
>>>>> For the record, this is slightly different than AllocateHeapAt which
>>>>> creates Xmx sized file for whole Java Heap and use no RAM for java
>> heap.
>>>>> This feature is only allocating old gen portion on nvdimm but the
>>>>> temporal file will be created size of Xmx for performance reason. But
>>>>> there is a logic to manage not to commit more than Xmx for both
>>>> young/old.
>>>> [Kharbas, Kishor] I added some explanation in globals.hpp.
>>>>
>>>> Thanks,
>>>> Kishor
>>>>> ----------------------------------------------------------
>>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.cpp
>>>>> 26 #include "gc/g1/g1HeterogeneousHeapPolicy.hpp"
>>>>> 27 #include "gc/g1/g1CollectedHeap.hpp"
>>>>> - Wrong order for line 26 and 27.
>>>>>
>>>>>
>>>>> ----------------------------------------------------------
>>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.cpp
>>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.hpp
>>>>> - Need 1 more space at line 2 ~ 23.
>>>>>
>>>>>
>>>>> ----------------------------------------------------------
>>>>> test/hotspot/jtreg/gc/nvdimm/TestHumongousObjectsOnNvdimm.java
>>>>> test/hotspot/jtreg/gc/nvdimm/TestOldObjectsOnNvdimm.java
>>>>> test/hotspot/jtreg/gc/nvdimm/TestYoungObjectsOnDram.java
>>>>> - You are missing ',' between 2018 and Oracle at line 2.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Sangheon
>>>>>
>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: sangheon.kim at oracle.com
>> [mailto:sangheon.kim at oracle.com]
>>>>>>> Sent: Tuesday, December 11, 2018 2:18 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/11/18 12:30 AM, 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/
>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.08_to_09/
>>>>>>> Here are some my comments.
>>>>>>> - I can't find newly added tests are excluded in this patch.
>>>>>> [Kharbas, Kishor] Done.
>>>>>>> - Some checking for not to exceed physical DRAM, Stefan also
>>>> mentioned
>>>>>>> this. Playing with NewSizePercentage and MaxNewSizePercentage,
>> and
>>>>> their
>>>>>>> constraint functions as well. Make sure checking time should be after
>>>>>>> ergonomics(AfterErgo).
>>>>>> [Kharbas, Kishor] Added logic in G1 constraint functions.
>>>>>>
>>>>>>> - Test results(both this feature on/off) with suggested test groups.
>>>>>>> - webrev.09 still contains ParallelGC patch.
>>>>>>>
>>>>>> [Kharbas, Kishor] removed in latest webrev.
>>>>>>> ----------------------------------
>>>>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.cpp
>>>>>>> 36 _g1h = g1h;
>>>>>>> - line 36 is redundant as G1Policy::init() does it. And a member of
>> _g1h
>>>>>>> here is also redundant.
>>>>>>>
>>>>>> [Kharbas, Kishor] Done.
>>>>>>
>>>>>>> 37
>>>>>>> static_cast<HeterogeneousHeapRegionManager*>(g1h->hrm())-
>>>>>>>> adjust_dram_regions((uint)young_list_target_length(),
>>>>>>> _g1h->workers());
>>>>>>> - You can add HeterogeneousHeapRegionManager* as a member of
>>> this
>>>>>>> policy
>>>>>>> if want to avoid static_cast. All 4 methods defined in this class have
>>>>>>> static_cast.
>>>>>>>
>>>>>> [Kharbas, Kishor] Done.
>>>>>>> ----------------------------------
>>>>>>> src/hotspot/share/gc/g1/g1HeterogeneousHeapPolicy.hpp
>>>>>>> 32 G1CollectedHeap* _g1h;
>>>>>>> - G1Policy already has G1CollectedHeap* as a member.
>>>>>>>
>>>>>> [Kharbas, Kishor] Done.
>>>>>>> Thanks,
>>>>>>> Sangheon
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> 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