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
Wed Nov 28 05:24:36 UTC 2018
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)
- 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.
2. New tests related general comments:
- As AllocateOldGenAt feature will not be tested regularly, new tests
should be excluded by default.
- New tests need filters for other unsupported GCs.
e.g. @requires vm.gc=="null" | vm.gc.G1 | vm.gc.Parallel
- Are there any reason for printing stdout in new tests? i.e. looks like
we don't need 'System.out.println(output.getStdout());'
----------------------
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(ReservedSpace 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(ReservedSpace 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