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
Tue Nov 6 05:39:01 UTC 2018


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