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 00:21:35 UTC 2018
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.
- Could you do some tests with young gen related flags added? I will
try too.
----------------------------------------------------------
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.
----------------------------------------------------------
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