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
Fri Dec 14 17:57:19 UTC 2018
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_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
>>>>>>>>> /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/tes
>>>>>>>>>> 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_hetero
>>>>>>>>>>> _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(_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_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