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 6 00:34:07 UTC 2018
Hi Kishor,
Forgot to replying on your previous question of:
>> 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?
>
Yes, exclude from all gc test groups.
For exmpale:
tier1_gc_1 = \
gc/epsilon/ \
gc/g1/ \
-gc/g1/ihop/TestIHOPErgo.java
*-gc/{nvdimm}*
tier1_gc_2 = \
gc/ \
-gc/epsilon/ \
*-gc/{nvdimm}*
hotspot_not_fast_gc = \
:hotspot_gc \
-:tier1_gc
*-gc/{nvdimm}*
Thanks,
Sangheon
On 12/5/18 3:02 PM, sangheon.kim at oracle.com wrote:
> 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)
>
> 2. Do we still need pre-mature old type after introducing 'borrowing
> concept'? (Stefan also mentioned about this)
>
> 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).
>
> --------------------------------------
> 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.
>
>
> --------------------------------------
> 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.
>
> 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.
>
>
> --------------------------------------
> 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?
>
>
> --------------------------------------
> 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'?
>
> 135 bool is_old_full();
> - 'const'?
>
>
>> 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?
>
> 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
>>>>>>>>>>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181205/d6e60a4a/attachment.htm>
More information about the hotspot-gc-dev
mailing list