RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
Kharbas, Kishor
kishor.kharbas at intel.com
Fri Dec 21 01:18:44 UTC 2018
Hi Sangheon,
Thanks for the review.
I have a minor update to properly close temporary nv-dimm file.
http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.14/
http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.13_to_14/
Thanks
Kishor
> -----Original Message-----
> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> Sent: Thursday, December 20, 2018 1:25 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/20/18 12:40 AM, Kharbas, Kishor wrote:
> > 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
> Webrev.13 looks good.
>
> Thanks,
> Sangheon
>
>
> >
> > 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_1
> >>>> 0
> >>>>>>> /
> >>>>>>>
> >>>>>>> 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_0
> >>>> 8
> >>>>>>>>> /
> >>>>>>>>>
> >> 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/runtim
> >>>>>>>>>>>>> e/
> >>>>>>>>>>>>> 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/runti
> >>>>>>>>>>>>>> me
> >>>>>>>>>>>>>> /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(ReservedSpac
> >>>>>>>> e
> >>>>>>>>>>>>>>> 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
> >>>>>>>>>>>>>>>>> policy->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))->initiali
> >>>>>>>>>>>>>>>>> ze
> >>>>>>>>>>>>>>>>> ()
> >>>>>>>>>>>>>>>>> )
> >>>>>>>>>>>>>>>>> {
> >>>>>>>>>>>>>>>>> 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