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