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 13 19:04:17 UTC 2018
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.
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.
Again, hope to have more manual testing with young gen size related
options added.
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_07/
>>> I have some comments.
>>>
>>> ----------------------------------------------------------
>>> src/hotspot/share/gc/parallel/generationSizer.hpp
>>> 46 bool is_hetero_heap() const;
>>> - 'virtual'?
>>>
>>> ----------------------------------------------------------
>>> src/hotspot/share/gc/parallel/parallelArguments.cpp
>>> 100 return create_heap_with_policy<ParallelScavengeHeap,
>>> GenerationSizer>();
>>> - Missing 2 spaces at line 100.
>>>
>>> ----------------------------------------------------------
>>> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.cpp
>>> 2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights
>>> reserved.
>>> ...
>>> 23 */
>>> - Missing 1 space for line 2 ~ 23.
>>>
>>> 39 julong phys_mem = FLAG_IS_DEFAULT(MaxRAM) ?
>>> os::physical_memory() : (julong)MaxRAM;
>>> - I saw your reasoning, but why would you like to allow MaxRAM via
>>> command-line? Accidentally it can be larger than physical RAM.
>>>
>>> ----------------------------------------------------------
>>> src/hotspot/share/gc/parallel/heterogeneousGenerationSizer.hpp
>>> - Missing 1 space for line 2 ~ 23.
>>>
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>>> -----Original Message-----
>>>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>>>>> Sent: Wednesday, December 12, 2018 1:38 AM
>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>>>> sangheon.kim at oracle.com; 'hotspot-gc-dev at openjdk.java.net'
>>>>> <hotspot-gc- dev at openjdk.java.net>; 'Hotspot dev runtime'
>>>>> <hotspot-runtime- dev at openjdk.java.net>
>>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>> Subject: Re: RFR(M): 8211424: Allocation of old generation of java
>>>>> heap on alternate memory devices - ParallelOld
>>>>>
>>>>> Hi Kishor,
>>>>>
>>>>> On 2018-12-12 09:16, Kharbas, Kishor wrote:
>>>>>> Hi Sangheon, Stefan,
>>>>>>
>>>>>> Thank you for your reviews.
>>>>>> I have an updated webrev (against g1 patch) at :
>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.06/
>>>>> src/hotspot/share/gc/parallel/generationSizer.cpp
>>>>> -------------------------------------------------
>>>>> Instead of adding set_ergo_flags_for_hetero_heap() to the generic
>>>>> GenerationSizer I think instead having a
>>>>> HeterogeneousGenerationSizer which does this at the correct point
>>>>> during initialization is a better approach.
>>>>>
>>>>> Also I don't think you should use the whole physical ram as limit,
>>>>> the JVM will use memory and you probably don't want to try to expand
>>>>> the young gen all the way to the max. The normal heap sizing code
>>>>> used when nothing is set uses MaxRamPercentage to limit the heap.
>>>>> There is also the flag MaxRam which can be used to limit the amount
>>>>> of DRAM used by the heap so I think limiting MaxNewSize to
>>>>> MIX(MaxRam, physical mem) * MaxRamPercentage /
>>>>> 100 would be a good idea. Something similar to what is done in
>>>>> Arguments::set_heap_size().
>>>>>
>>>> [Kharbas, Kishor] I added HeterogeneousGenerationSizer which would
>>>> limit NewSize and MaxNewSize. My reasoning is as follows,
>>>> - If MaxRAM is set on command line, use that as available dram
>>>> memory
>>>> - If MaxRAMPercentage or MaxRamFraction is set on cmd line, we
>>>> will limit young size accordingly.
>>>> - Otherwise, I use my own ' MaxRamFractionForYoung' to limit young
>>>> size. Reason is - since MaxRAMPercentage is very low (20%) since its
>>>> meant to size heap when MaxHeap is not specified.
>>>>
>>>>> src/hotspot/share/runtime/arguments.cpp
>>>>> ---------------------------------------
>>>>> 2960 // When AllocateOldGenAt is set, we cannot use largepages for
>>>>> entire heap memory.
>>>>> 2961 // Only young gen which is allocated in dram can use large
>>>>> pages, but we currently don't support that.
>>>>> 2962 if (!FLAG_IS_DEFAULT(AllocateOldGenAt)) {
>>>>> 2963 FLAG_SET_DEFAULT(UseLargePages, false);
>>>>> 2964 }
>>>>>
>>>>> From what I can see you have not addressed this or explained why
>>>>> this have to be done here instead of in GCArguments.
>>>>>
>>>> [Kharbas, Kishor] I moved this to GCArguments. But since this is also
>>>> part of G1 patch, so it's not seen here.
>>>>
>>>> Thank you
>>>> Kishor
>> http://cr.openjdk.java.net/~kkharbas/8211424/8211424.webrev.05_to_0
>>>>>> 6
>>>>>>
>>>>>> Jtreg tests (hotspot_runtime, compiler+hotspot, serviceability,
>>>>> vmTestbase_vm_gc) on my system gave me following results.
>>>>>> (1) OpenJDK tip : passed: 1,806; failed: 24; error: 189
>>>>>> (2) Parallel patch, no flag : passed: 1,590; failed: 15; error:
>>>>>> 242
>>>>>> (3) Parallel patch with flag : passed 1,816; failed: 15; error: 188
>>>>>>
>>>>>> New failure in (1) -> (2)
>>>>>> -
>>>>> compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/Tes
>>>>> tResol
>>>>>
>>>>> vedJavaType.java
>>>>>> I looked at the report, but could not figure out why it
>>>>>> fails. Most likely it's
>>>>> my environment not being setup correctly.
>>>>> I think this was an issue in OpenJDK pushed during the weekend and
>>>>> fixed (test excluded) Monday afternoon.
>>>>>> New failures in (1) -> (3)
>>>>>> -
>>>>>> compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/Te
>>>>>> stR
>>>>>> esolvedJavaType.java
>>>>>> - compiler/aot/verification/vmflags/TrackedFlagTest.java
>>>>>> Fails because we disable UseCompressedOops.
>>>>>>
>>>>>> @Stefan, I will do performance regression test with SPECjvm2008
>>>>> benchmarks.
>>>>> Good, SPECjvm2008 should not show any problems, but good to run
>>>>> anyways.
>>>>> I've run SPECjbb2015 without seeing any regressions so that is also
>>>>> good.
>>>>>
>>>>> Thanks,
>>>>> Stefan
>>>>>
>>>>>> Thanks,
>>>>>> Kishor
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>>>>>>> Sent: Tuesday, December 11, 2018 2:17 PM
>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; Stefan Johansson
>>>>>>> <stefan.johansson at oracle.com>; 'hotspot-gc-dev at openjdk.java.net'
>>>>>>> <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev runtime' <hotspot-
>>>>>>> runtime-dev at openjdk.java.net>
>>>>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>> Subject: Re: RFR(M): 8211424: Allocation of old generation of java
>>>>>>> heap on alternate memory devices - ParallelOld
>>>>>>>
>>>>>>> Hi Kishor,
>>>>>>>
>>>>>>> On 12/11/18 1:00 AM, Kharbas, Kishor wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Here is an updated webrev which fixes the comments below.
>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.05
>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.04_to_05/
>>>>>>> Here are some general comments:
>>>>>>> - As I told in review thread of 8211425-G1 part review thread, I
>>>>>>> would like to see separate patch for Parallel GC part.
>>>>>>> - A way not to use more than DRAM, same as G1.
>>>>>>> - Testing results with/without this feature.
>>>>>>>
>>>>>>> And I have some minor comments here.
>>>>>>>
>>>>>>> -----------------------------------
>>>>>>> src/hotspot/share/gc/parallel/adjoiningGenerations.cpp
>>>>>>> 287 AdjoiningGenerations*
>>>>>>> AdjoiningGenerations::create_adjoining_generations(ReservedSpace
>>>>>>> old_young_rs,
>>>>>>> 288 GenerationSizer* policy,
>>>>>>> 289 size_t alignment) {
>>>>>>> - Wrong alignment at line 288/289.
>>>>>>>
>>>>>>>
>>>>>>> -----------------------------------
>>>>>>> src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.cp
>>>>>>> p
>>>>>>> 47 _total_size_limit = policy->max_heap_byte_size();
>>>>>>> - Please move back to initialization list unless there's any good
>>>>>>> reason.
>>>>>>>
>>>>>>>
>>>>>>> -----------------------------------
>>>>>>> src/hotspot/share/gc/parallel/generationSizer.cpp
>>>>>>> - Copyright update
>>>>>>>
>>>>>>> 80 if(is_hetero_heap() && UseAdaptiveGCBoundary) {
>>>>>>> 81 // This is the size that young gen can grow to, when
>>>>>>> AdaptiveGCBoundary is true.
>>>>>>> 83 // This is the size that old gen can grow to, when
>>>>>>> AdaptiveGCBoundary is true.
>>>>>>> - Line 81 and 83, 'AdaptiveGCBoundary' -> 'UseAdaptiveGCBoundary'.
>>>>>>>
>>>>>>>
>>>>>>> -----------------------------------
>>>>>>> src/hotspot/share/gc/parallel/generationSizer.hpp
>>>>>>> - Copyright update
>>>>>>>
>>>>>>>
>>>>>>> -----------------------------------
>>>>>>> src/hotspot/share/gc/parallel/psParallelCompact.cpp
>>>>>>> 1998 // We also return when its a heterogenous heap because old
>>>>>>> generation cannot absorb data from eden
>>>>>>> - 'also return' -> 'also return false'.
>>>>>>> - 'its' -> 'it's' or 'it is'
>>>>>>>
>>>>>>> 2000 if (!(UseAdaptiveSizePolicy && UseAdaptiveGCBoundary) ||
>>>>>>> ParallelScavengeHeap::heap()->ps_collector_policy()->is_hetero_hea
>>>>>>> p()
>>>>>>> ) {
>>>>>>> - Add new line for the new condition?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sangheon
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> I have create separate webrev for tests since they are common
>>>>>>>> this issue and 8211425 -
>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.test.00/
>>>>>>>>
>>>>>>>> I am running jtreg tests for all hotspot tests to check the
>>>>>>>> stability.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Kishor
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>>>>>>>>> Sent: Friday, November 30, 2018 6:12 AM
>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; 'hotspot-gc-
>>>>>>>>> dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>;
>>>>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>>>>>>>>> sangheon.kim at oracle.com
>>>>>>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>>>> Subject: Re: RFR(M): 8211424: Allocation of old generation of
>>>>>>>>> java heap on alternate memory devices - ParallelOld
>>>>>>>>>
>>>>>>>>> Hi Kishor,
>>>>>>>>>
>>>>>>>>> On 2018-11-22 08:17, Kharbas, Kishor wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Requesting review of the patch for allocating old generation of
>>>>>>>>>> parallelold gc on alternate memory devices such nv-dimm.
>>>>>>>>>>
>>>>>>>>>> The design of this implementation is explained on the bug page
>>>>>>>>>> - https://bugs.openjdk.java.net/browse/JDK-8211424.
>>>>>>>>>>
>>>>>>>>>> This is subtask of
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8202286
>>>>>>>>>> which is implementation for parallel old gc.
>>>>>>>>>>
>>>>>>>>>> Please follow the parent issue here : 64 if
>>>>>>>>>> (AllocateOldGenAt != NULL
>>>>>>> &&
>>>>>>>>> UseAdaptiveGCBoundary) {
>>>>>>>>> 65 heap_size =
>>>>>>>>>
>> AdjoiningGenerationsForHeteroHeap::required_reserved_memory(_collect
>>>>> o
>>>>>>>>> r
>>>>>>>>> _policy);
>>>>>>>>> 66 }
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8202286.
>>>>>>>>>>
>>>>>>>>>> (PS: this is continuation of old thread 'RFR(M): 8204908:
>>>>>>>>>> NVDIMM for POGC and G1GC - ReserveSpace.cpp changes are
>> mostly
>>>>>>>>>> eliminated/no collector specific code.')
>>>>>>>>>>
>>>>>>>>>> Webrev:
>> http://cr.openjdk.java.net/~kkharbas/8211424/webrev.04
>>>>>>>>> First a general comment which goes a long the same lines as many
>>>>>>>>> of my comments for the G1 part. I would like to see some more
>>>>>>>>> abstraction around the use of AllocateOldGenAt, I think we
>>>>>>>>> should consider it during argument parsing to turn off
>>>>>>>>> unsupported things like compressed oops and once during
>>>>>>>>> initialization of the GC to know if we are using the feature or
>>>>>>>>> not. After that there should only be calls like
>>>>>>>>> policy->is_hetero_heap() if we really need to decide on this.
>>>>>>>>>
>>>>>>>>> src/hotspot/share/gc/parallel/adjoiningGenerations.cpp
>>>>>>>>> ------------------------------------------------------
>>>>>>>>> 97
>>>>>>>>> virtual_spaces()->reserved_space().first_part(max_low_byte_size,
>>>>>>>>> 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