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

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() 
|| 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
> 