RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
Stefan Johansson
stefan.johansson at oracle.com
Wed Dec 12 23:04:39 UTC 2018
Hi Kishor,
On 2018-12-12 21:51, Kharbas, Kishor wrote:
> New webrev and comments inline.
>
> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.07/
> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.06_to_07/
>
>> -----Original Message-----
>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>> Sent: Wednesday, December 12, 2018 1:38 AM
>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>> sangheon.kim at oracle.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>
>> 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-12-12 09:16, Kharbas, Kishor wrote:
>>> Hi Sangheon, Stefan,
>>>
>>> Thank you for your reviews.
>>> I have an updated webrev (against g1 patch) at :
>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.06/
>> src/hotspot/share/gc/parallel/generationSizer.cpp
>> -------------------------------------------------
>> Instead of adding set_ergo_flags_for_hetero_heap() to the generic
>> GenerationSizer I think instead having a HeterogeneousGenerationSizer
>> which does this at the correct point during initialization is a better approach.
>>
>> Also I don't think you should use the whole physical ram as limit, the JVM will
>> use memory and you probably don't want to try to expand the young gen all
>> the way to the max. The normal heap sizing code used when nothing is set
>> uses MaxRamPercentage to limit the heap. There is also the flag MaxRam
>> which can be used to limit the amount of DRAM used by the heap so I think
>> limiting MaxNewSize to MIX(MaxRam, physical mem) * MaxRamPercentage /
>> 100 would be a good idea. Something similar to what is done in
>> Arguments::set_heap_size().
>>
> [Kharbas, Kishor] I added HeterogeneousGenerationSizer which would limit NewSize and MaxNewSize. My reasoning is as follows,
> - If MaxRAM is set on command line, use that as available dram memory
> - If MaxRAMPercentage or MaxRamFraction is set on cmd line, we will limit young size accordingly.
> - Otherwise, I use my own ' MaxRamFractionForYoung' to limit young size. Reason is - since MaxRAMPercentage is very low (20%) since its meant to size heap when MaxHeap is not specified.
I have not had time to do any testing on this but it looks really
promising. I think your FractionForYoung might be a bit too aggressive,
only leaving 20% of the RAM to the rest of the JVM and the OS might be
too little.
I also wonder if NewSize should be set to the same value as MaxNewSize,
but I guess this is the best we can do if it for some reason is larger
than the maximum allowed.
>> 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 }
>>
>> From what I can see you have not addressed this or explained why this have
>> to be done here instead of in GCArguments.
>>
> [Kharbas, Kishor] I moved this to GCArguments. But since this is also part of G1 patch, so it's not seen here.
I realized that once going through the G1 patch, thanks.
Cheers,
Stefan
> Thank you
> Kishor
>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.05_to_06
>>>
>>> Jtreg tests (hotspot_runtime, compiler+hotspot, serviceability,
>> vmTestbase_vm_gc) on my system gave me following results.
>>> (1) OpenJDK tip : passed: 1,806; failed: 24; error: 189
>>> (2) Parallel patch, no flag : passed: 1,590; failed: 15; error: 242
>>> (3) Parallel patch with flag : passed 1,816; failed: 15; error: 188
>>>
>>> New failure in (1) -> (2)
>>> -
>> compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResol
>> vedJavaType.java
>>> I looked at the report, but could not figure out why it fails. Most likely it's
>> my environment not being setup correctly.
>> I think this was an issue in OpenJDK pushed during the weekend and fixed
>> (test excluded) Monday afternoon.
>>> New failures in (1) -> (3)
>>> -
>>> compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestR
>>> esolvedJavaType.java
>>> - compiler/aot/verification/vmflags/TrackedFlagTest.java
>>> Fails because we disable UseCompressedOops.
>>>
>>> @Stefan, I will do performance regression test with SPECjvm2008
>> benchmarks.
>> Good, SPECjvm2008 should not show any problems, but good to run
>> anyways.
>> I've run SPECjbb2015 without seeing any regressions so that is also good.
>>
>> Thanks,
>> Stefan
>>
>>> Thanks,
>>> Kishor
>>>
>>>> -----Original Message-----
>>>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>>>> Sent: Tuesday, December 11, 2018 2:17 PM
>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
>>>> <stefan.johansson at oracle.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>
>>>> 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 12/11/18 1:00 AM, 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/
>>>> Here are some general comments:
>>>> - As I told in review thread of 8211425-G1 part review thread, I
>>>> would like to see separate patch for Parallel GC part.
>>>> - A way not to use more than DRAM, same as G1.
>>>> - Testing results with/without this feature.
>>>>
>>>> And I have some minor comments here.
>>>>
>>>> -----------------------------------
>>>> src/hotspot/share/gc/parallel/adjoiningGenerations.cpp
>>>> 287 AdjoiningGenerations*
>>>> AdjoiningGenerations::create_adjoining_generations(ReservedSpace
>>>> old_young_rs,
>>>> 288 GenerationSizer* policy,
>>>> 289 size_t alignment) {
>>>> - Wrong alignment at line 288/289.
>>>>
>>>>
>>>> -----------------------------------
>>>> src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.cpp
>>>> 47 _total_size_limit = policy->max_heap_byte_size();
>>>> - Please move back to initialization list unless there's any good reason.
>>>>
>>>>
>>>> -----------------------------------
>>>> src/hotspot/share/gc/parallel/generationSizer.cpp
>>>> - Copyright update
>>>>
>>>> 80 if(is_hetero_heap() && UseAdaptiveGCBoundary) {
>>>> 81 // This is the size that young gen can grow to, when
>>>> AdaptiveGCBoundary is true.
>>>> 83 // This is the size that old gen can grow to, when
>>>> AdaptiveGCBoundary is true.
>>>> - Line 81 and 83, 'AdaptiveGCBoundary' -> 'UseAdaptiveGCBoundary'.
>>>>
>>>>
>>>> -----------------------------------
>>>> src/hotspot/share/gc/parallel/generationSizer.hpp
>>>> - Copyright update
>>>>
>>>>
>>>> -----------------------------------
>>>> src/hotspot/share/gc/parallel/psParallelCompact.cpp
>>>> 1998 // We also return when its a heterogenous heap because old
>>>> generation cannot absorb data from eden
>>>> - 'also return' -> 'also return false'.
>>>> - 'its' -> 'it's' or 'it is'
>>>>
>>>> 2000 if (!(UseAdaptiveSizePolicy && UseAdaptiveGCBoundary) ||
>>>> ParallelScavengeHeap::heap()->ps_collector_policy()->is_hetero_heap()
>>>> ) {
>>>> - Add new line for the new condition?
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>
>>>>> 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(_collecto
>>>>>> r
>>>>>> _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
>>>>>>>
More information about the hotspot-gc-dev
mailing list