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

[Kharbas, Kishor]

Hi,

Here is an updated webrev which fixes the comments below.
http://cr.openjdk.java.net/~kkharbas/8211424/webrev.05
http://cr.openjdk.java.net/~kkharbas/8211424/webrev.04_to_05/

I have create separate webrev for tests since they are common this issue and 8211425 - http://cr.openjdk.java.net/~kkharbas/8211424/webrev.test.00/

I am running jtreg tests for all hotspot tests to check the stability.

Thanks
Kishor

> -----Original Message-----
> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
> Sent: Friday, November 30, 2018 6:12 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>;
> sangheon.kim at oracle.com
> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
> Subject: Re: RFR(M): 8211424: Allocation of old generation of java heap on
> alternate memory devices - ParallelOld
> 
> 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
> >