RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
Kharbas, Kishor
kishor.kharbas at intel.com
Thu Dec 13 03:59:43 UTC 2018
Hi Sangheon, Stefan,
Here is an updated webrev - http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.07_to_08/
http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.08/
Thanks
Kishor
> -----Original Message-----
> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> Sent: Wednesday, December 12, 2018 4:34 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
>
> One more!
>
> ----------------------------------------------------------
> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
> 56 log_warning(gc, ergo)("Setting MaxNewSize to " SIZE_FORMAT "
> based on dram memory available", reasonable_max);
> 62 log_warning(gc, ergo)("Setting NewSize to " SIZE_FORMAT "
> based on dram memory available", reasonable_max);
> - You need to type-cast 'reasonable_max' to 'size_t' for both lines.
> Fails compiling on Mac.
>
> Thanks,
> Sangheon
>
>
> On 12/12/18 3:12 PM, sangheon.kim at oracle.com wrote:
> > Hi Kishor,
> >
> > On 12/12/18 12:51 PM, 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/
> > I have some comments.
> >
> > ----------------------------------------------------------
> > src/hotspot/share/gc/parallel/generationSizer.hpp
> > 46 bool is_hetero_heap() const;
> > - 'virtual'?
> >
> > ----------------------------------------------------------
> > src/hotspot/share/gc/parallel/parallelArguments.cpp
> > 100 return create_heap_with_policy<ParallelScavengeHeap,
> > GenerationSizer>();
> > - Missing 2 spaces at line 100.
> >
> > ----------------------------------------------------------
> > src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
> > 2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights
> > reserved.
> > ...
> > 23 */
> > - Missing 1 space for line 2 ~ 23.
> >
> > 39 julong phys_mem = FLAG_IS_DEFAULT(MaxRAM) ?
> > os::physical_memory() : (julong)MaxRAM;
> > - I saw your reasoning, but why would you like to allow MaxRAM via
> > command-line? Accidentally it can be larger than physical RAM.
> >
> > ----------------------------------------------------------
> > src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.hpp
> > - Missing 1 space for line 2 ~ 23.
> >
> >
> > Thanks,
> > Sangheon
> >
> >
> >>
> >>> -----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_0
> >>>> 6
> >>>>
> >>>> 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/Tes
> >>> tResol
> >>>
> >>> 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/Te
> >>>> stR
> >>>> 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.cp
> >>>>> p
> >>>>> 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_hea
> >>>>> p()
> >>>>> ) {
> >>>>> - 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(_collect
> >>> o
> >>>>>>> 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