RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld

[Stefan Johansson]

Hi Kishor,

On 2018-11-22 08:17, Kharbas, Kishor wrote:
> Hi all,
> 
> Requesting review of the patch for allocating old generation of 
> parallelold 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-8211424.
> 
> This is subtask of https://bugs.openjdk.java.net/browse/JDK-8202286 
> which is implementation for parallel old gc.
> 
> Please follow the parent issue here :   64   if (AllocateOldGenAt != NULL && UseAdaptiveGCBoundary) {
   65     heap_size = 
AdjoiningGenerationsForHeteroHeap::required_reserved_memory(_collector_policy);
   66   }
> https://bugs.openjdk.java.net/browse/JDK-8202286.
> 
> (PS: this is continuation of old thread 'RFR(M): 8204908: NVDIMM for 
> POGC and G1GC - ReserveSpace.cpp changes are mostly eliminated/no 
> collector specific code.’)
> 
> Webrev: http://cr.openjdk.java.net/~kkharbas/8211424/webrev.04

First a general comment which goes a long the same lines as many of my 
comments for the G1 part. I would like to see some more abstraction 
around the use of AllocateOldGenAt, I think we should consider it during 
argument parsing to turn off unsupported things like compressed oops and 
once during initialization of the GC to know if we are using the feature 
or not. After that there should only be calls like 
policy->is_hetero_heap() if we really need to decide on this.

src/hotspot/share/gc/parallel/adjoiningGenerations.cpp
------------------------------------------------------
97 
virtual_spaces()->reserved_space().first_part(max_low_byte_size, 
AllocateOldGenAt != NULL /* split */);
   One example of what I mention above, I think you should consider 
doing the same as you did for G1 and create the ParallelScavengeHeap 
with at CollectorPolicy that is nvdimm aware.

src/hotspot/share/gc/parallel/asPSOldGen.hpp
--------------------------------------------
   32 #include "gc/parallel/psFileBackedVirtualspace.hpp"

I see no use of this in this header.

src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp
------------------------------------------------------
   65   // is the heap backed by two different memories?
   66   bool _is_hetero_heap;

As for G1 I think this doesn't need to be stored in the heap but the 
CollectorPolicy (GenerationSizer) knows this and can be queried when 
needed. Keeping the getter is fine, but just have it call the policy.

src/hotspot/share/gc/parallel/psOldGen.cpp
------------------------------------------
   75   if(AllocateOldGenAt != NULL) {
   76     _virtual_space = new PSFileBackedVirtualSpace(rs, alignment, 
AllocateOldGenAt);
   77     if (!(static_cast 
<PSFileBackedVirtualSpace*>(_virtual_space))->initialize()) {
   78       vm_exit_during_initialization("Could not map space for 
PSOldGen at given AllocateOldGenAt path");
   79     }

Either pass in if this is a hetero heap or ask the heap, it can be 
obtained by ParallelScavengeHeap::heap().

I also think it would be better to have the PSFileBackedVirtualSpace 
constructor call initialize directly and let it fail rather than 
returning false, since we will abort the VM in this case anyway.

src/hotspot/share/memory/universe.cpp
-------------------------------------
  829   // When AllocateOldGenAt is set, we cannot use largepages for 
entire heap memory.
  830   // Only young gen which is allocated in dram can use large 
pages, but we currently don't support that.
  831   use_large_pages = (AllocateOldGenAt != NULL) ? false : 
use_large_pages;

Move this to argument parsing and do FLAG_SET_DEFAULT(UseLargePages, 
false) when nvdimm is used.

src/hotspot/share/runtime/arguments.cpp
---------------------------------------
1627   if(AllocateOldGenAt != NULL) {
1628     FLAG_SET_ERGO(bool, UseCompressedOops, false);
1629     return;
1630   }

As for G1 I can't see no reason why this can't be moved to 
GCArguments::init.

Thanks,
Stefan

> 
>                  
> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.03_to_04
> 
> Thanks
> 
> Kishor
>