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 20:51:06 UTC 2018
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.
> 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.
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