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().
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.
> 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/TestResolvedJavaType.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/TestResolvedJavaType.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
>>>>>
>