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
Tue Dec 11 22:17:20 UTC 2018
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(_collector
>> _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