RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
Kharbas, Kishor
kishor.kharbas at intel.com
Wed Dec 12 08:16:22 UTC 2018
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/
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.
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.
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