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