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 21:01:24 UTC 2018
Hi Sagheon,
I have a new webrev at - http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.10/
http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.09_to_10/
Comments inline.
> -----Original Message-----
> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> Sent: Thursday, December 13, 2018 11:04 AM
> 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/12/18 7:59 PM, Kharbas, Kishor wrote:
> > 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/
>
> ----------------------------------------------------------
> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
> 36 julong young_size_limit = os::physical_memory();
> - This variable is not used.
[Kharbas, Kishor] Fixed.
>
> 58 log_info(gc, ergo)("Setting MaxNewSize to " SIZE_FORMAT "
> which is 80% of dram memory. Dram usage can be lowered by setting
> MaxNewSize to a lower value", (size_t)reasonable_max);
> - Later I saw you fixed this compile error on webrev.09, good.
>
[Kharbas, Kishor] Fixed. And I changed log message according to Stefan's comment.
> Again, hope to have more manual testing with young gen size related options
> added.
>
[Kharbas, Kishor] I tested different cases with different combinations of values for MaxNewSize, NewSize, MaxRAM, MaxRAMPercentage/Fraction, etc.
Thank you
Kishor
> Thanks,
> Sangheon
>
> >
> > 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/T
> >>>>> es
> >>>>> 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_h
> >>>>>>> ea
> >>>>>>> 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_siz
> >>>>>>>>> e, 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