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 00:33:41 UTC 2018
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_06
>>>>
>>>> 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/TestResol
>>>
>>> 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/TestR
>>>> 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.cpp
>>>>> 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_heap()
>>>>> ) {
>>>>> - 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(_collecto
>>>>>>> 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