RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
Kharbas, Kishor
kishor.kharbas at intel.com
Thu Dec 20 08:40:15 UTC 2018
Thanks Stefan,
I have incorporated the comment in the new webrev at : http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.13
http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.12_to_13
Regards,
Kishor
> -----Original Message-----
> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
> Sent: Monday, December 17, 2018 3:07 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,
>
> Same problem here as for G1, the current patch doesn't build. Please add
> this patch to your changeset:
> http://cr.openjdk.java.net/~sjohanss/8211425/Final-Comment-Par/
>
> Cheers,
> Stefan
>
> On 2018-12-15 03:02, Kharbas, Kishor wrote:
> > 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_1
> >>>> 1
> >>>>
> >>>> 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_he
> >>>>>>>>>>>>> te
> >>>>>>>>>>>>> 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_by
> >>>>>>>>>>>>>>> te _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