RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices (original) (raw)

Stefan Johansson stefan.johansson at oracle.com
Fri Oct 26 14:32:02 UTC 2018

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() || Heaplock->ownedbyself())) { 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 tier1gc and tier1runtime 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-runtime-dev mailing list