RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
Stefan Johansson
stefan.johansson at oracle.com
Mon Dec 17 11:07:07 UTC 2018
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_11
>>>>
>>>> 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_hete
>>>>>>>>>>>>> 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_byte
>>>>>>>>>>>>>>> _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