RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC

Kharbas, Kishor kishor.kharbas at intel.com
Fri Nov 2 21:48:43 UTC 2018


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?

I am working on addressing your comments on the patch (had been dragged into another project, so I am behind on this).

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