RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
sangheon.kim at oracle.com
sangheon.kim at oracle.com
Thu Dec 20 21:25:19 UTC 2018
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_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