RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
Stefan Johansson
stefan.johansson at oracle.com
Thu Dec 6 14:03:22 UTC 2018
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.
>
> 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/
>
> =====================================================================================================================
> 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.
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.
>
> =====================================================================================================================
>
> 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