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

Tue Dec 11 09:23:06 UTC 2018

On 2018-12-11 10:00, Kharbas, Kishor wrote:
> 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/
> 
src/hotspot/share/runtime/arguments.cpp
---------------------------------------
2960   // When AllocateOldGenAt is set, we cannot use largepages for 
entire heap memory.
2961   // Only young gen which is allocated in dram can use large pages, 
but we currently don't support that.
2962   if (!FLAG_IS_DEFAULT(AllocateOldGenAt)) {
2963     FLAG_SET_DEFAULT(UseLargePages, false);
2964   }

Is there any reason for setting this here instead of with the other 
flags in gcArguments.cpp?

> 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/
> 
The @requires line for these tests should only include vm.gc=="null" 
adding the or statements will make the tests fail if G1 or Parallel is 
provided through test.java.opts.

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

Have you done any performance testing for the Parallel version, without 
the feature enabled to make sure we don't have any regressions against 
the baseline?

Cheers,
Stefan

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