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 09:18:54 UTC 2018
New webrev - http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.09/
http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.08_to_09
There was a compilation error introduced by earlier one.
Thanks,
Kishor
> -----Original Message-----
> From: Kharbas, Kishor
> Sent: Wednesday, December 12, 2018 8:00 PM
> To: 'sangheon.kim at oracle.com' <sangheon.kim at oracle.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>; Kharbas,
> Kishor <kishor.kharbas at intel.com>
> Subject: RE: RFR(M): 8211424: Allocation of old generation of java heap on
> alternate memory devices - ParallelOld
>
> 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/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