RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
Kharbas, Kishor
kishor.kharbas at intel.com
Sat Dec 15 02:02:38 UTC 2018
Thank you for the review.
I have new webrev with suggested changes at : http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.12/
http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.11_to_12
Thanks
Kishor
> -----Original Message-----
> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> Sent: Friday, December 14, 2018 9:57 AM
> To: Stefan Johansson <stefan.johansson at oracle.com>; 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>
> 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/14/18 2:30 AM, Stefan Johansson wrote:
> > Hi Kishor,
> >
> > Thanks for addressing my comments, but I have one more :)
> >
> > On 2018-12-14 08:59, Kharbas, Kishor wrote:
> >> New webrev with small incremental change to make young sizing logs
> >> similar to G1.
> >>
> >> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.11
> > src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
> > --------------------------------------------------------------
> > 37 char calc_str[100];
> > 38 calc_str[0] = 0;
> >
> > Instead of using a raw char buffer we have the FormatBuffer class that
> > can be used. If you look in
> > G1CollectedHeap::do_collection_pause_at_safepoint there is an example
> > on how you can use it. I think such change would both help readability
> > and robustness.
> I like this suggestion.
>
> In addition to this:
> -------------------------
> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
> 36 // Check the available dram memory to limit NewSize and MaxNewSize.
> And then
> 37 void HeterogeneousGenerationSizer::initialize_flags() {
> - Incomplete comment at line 36?
>
> Thanks,
> Sangheon
>
>
> >
> > Thanks,
> > Stefan
> >
> >> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.10_to_11
> >>
> >> Thanks
> >> Kishor
> >>
> >>> -----Original Message-----
> >>> From: Kharbas, Kishor
> >>> Sent: Thursday, December 13, 2018 1:01 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 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_0
> >>>> 7/
> >>>>>>> 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/te
> >>>>>>>>> st
> >>>>>>>>> /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/t
> >>>>>>>>>> es
> >>>>>>>>>> t/
> >>>>>>>>>> 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_hete
> >>>>>>>>>>> ro
> >>>>>>>>>>> _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(_collec
> >>>> t
> >>>>>>>>> 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
> >>>>>>>>>>>>> _s iz 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