RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
Kharbas, Kishor
kishor.kharbas at intel.com
Wed Dec 12 23:18:42 UTC 2018
> -----Original Message-----
> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
> Sent: Wednesday, December 12, 2018 3:05 PM
> 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 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.
>
[Kharbas, Kishor] I can print a info message (if MaxNewSize is not set) saying - "Using 80% of dram as MaxNewSize, if less dram is to be used, set MaxNewSize on command line.. "
> >> 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.
>
[Kharbas, Kishor] Thank you. - Kishor
> 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/Test
> >> Resol
> >> 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/Tes
> >>> tR
> >>> 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