RFR(M): 8204908: NVDIMM for POGC and G1GC - ReserveSpace.cpp changes are mostly eliminated/no collector specific code.
sangheon.kim at oracle.com
sangheon.kim at oracle.com
Thu Oct 11 06:16:09 UTC 2018
Hi Kishor,
On 10/3/18 6:24 PM, Kharbas, Kishor wrote:
> Hi,
> I have an update to the webrev which addresses comments from Sangheon during an offline discussion.
>
> The JEP has been moved to an enhancement, consequently bug id '8204908' has been closed. I have created a new issue for this implementation -https://bugs.openjdk.java.net/browse/JDK-8211424.
> Check the comments in main issue -https://bugs.openjdk.java.net/browse/JDK-8202286.
> Any suggestion on whether I continue on this thread or start new one. Not only the bug id has changed, but also the design, implementation.
Thanks for the explanation.
> Full:http://cr.openjdk.java.net/~kkharbas/8211424/webrev-8204908.03
> Incremental :http://cr.openjdk.java.net/~kkharbas/8211424/webrev-8204908.03_to_02
webrev.03 looks okay in general.
A few minor nits:
src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.cpp
104 _max_old_byte_size(_max_total_size - _min_young_byte_size),
105 _max_young_byte_size(_max_total_size - _min_old_byte_size),
106 _max_total_size(max_total_size) {
- line 104 and 105 will use uninitialized value of _max_total_size. I
couldn't catch it before.
One style comment is that adjoiningGenerationsForHeteroHeap.cpp uses
both high()/low() and young_vs()/old_vs() which makes me confused. I
would prefer not to add young_vs()/old_vs(), but I don't have strong
opinion on this. But using only one(high() or young_vs()) instead of
mixed use of high() or young_vs() at ctor seems better.
> Testing : Passed tier1_gc and tier1_runtime jtret tests.
> I added extra options "-XX:+UnlockExperimentalVMOptions -XX:AllocateOldGenAt=/home/Kishor" to each test. There are some tests which I had to exclude since they won't work with this flag. Example: tests in ConcMarkSweepGC which does not support this flag, tests requiring CompressedOops to be enabled, etc.
Thanks for testing.
Could you also test without AllocateOldGetAt option enabled? As this
option will be disabled as a default, modified codes also need
verifications.
Thanks,
Sangheon
> Thanks,
> Kishor
>
>> -----Original Message-----
>> From: Kharbas, Kishor
>> Sent: Wednesday, September 19, 2018 3:57 PM
>> To:sangheon.kim at oracle.com; Thomas Stüfe
>> <thomas.stuefe at gmail.com>; Thomas Schatzl
>> <thomas.schatzl at oracle.com>
>> Cc:hotspot-gc-dev at openjdk.java.net; Hotspot dev runtime <hotspot-
>> runtime-dev at openjdk.java.net>; Viswanathan, Sandhya
>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>> <shirish.aundhe at intel.com>; Kharbas, Kishor<kishor.kharbas at intel.com>
>> Subject: RE: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>> ReserveSpace.cpp changes are mostly eliminated/no collector specific code.
>>
>> Hi,
>> I have an small update to the patch to disable UseCompressedOops.
>> http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.02/
>> http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.02_to_01/
>>
>> Regards,
>> Kishor
>>
>>> -----Original Message-----
>>> From: Kharbas, Kishor
>>> Sent: Wednesday, September 19, 2018 9:35 AM
>>> To: 'sangheon.kim at oracle.com'<sangheon.kim at oracle.com>; Thomas
>> Stüfe
>>> <thomas.stuefe at gmail.com>; Thomas Schatzl
>> <thomas.schatzl at oracle.com>
>>> Cc:hotspot-gc-dev at openjdk.java.net; Hotspot dev runtime <hotspot-
>>> runtime-dev at openjdk.java.net>; Viswanathan, Sandhya
>>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>>> <shirish.aundhe at intel.com>; Kharbas, Kishor<kishor.kharbas at intel.com>
>>> Subject: RE: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>>> ReserveSpace.cpp changes are mostly eliminated/no collector specific
>> code.
>>> Thanks Sangheon,
>>>
>>> I have uploaded the updated patch at,
>>> http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.01/
>>> http://cr.openjdk.java.net/~kkharbas/8204908/webrev-
>> 8204908.01_to_00/
>>> I have fixed all the indentation and format related comments, others I
>>> have pasted here below with my comments inline.
>>>
>>> =============================
>>>> src/hotspot/share/gc/parallel/adjoiningGenerations.hpp
>>>> - Copyright update
>>>>
>>>> 62 AdjoiningGenerations();
>>>> - Why we need this ctor?
>>>>
>>>>> I need this default ctor for constructing
>>> adjoiningGenerationsForHeteroHeap, since I don't want to call the non-
>>> default constructor of adjoiningGenerations.
>>>
>>>> =========================
>>>> /src/hotspot/share/gc/parallel/adjoiningVirtualSpaces.hpp
>>>> - Copyright update
>>>>
>>>> 88 virtual PSVirtualSpace* high() { return _high; }
>>>> 89 virtual PSVirtualSpace* low() { return _low; }
>>>> - Those methods don't need 'virtual' specifier as high()/low()
>>>> methods are only used at ctor of AdjoiningGenerations. The other
>>>> calling site is ctor of AdjoiningGenerationsForHeteroHeap but it is
>>>> used another type of
>>>> high()/low() which returns _young_vs or _old_vs.
>>>>
>>>>> I feel overriding these methods is a good idea from design
>>>>> standpoint;
>>> code changes in future would take benefit of this.
>>>
>>> ========================
>>>> src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.hpp
>>>> 45 PSVirtualSpace* _young_vs;
>>>> 46 PSVirtualSpace* _old_vs;
>>>> - Can't we use 'AdjoiningVirtualSpaces::_low' and '_high' instead?
>>>> If it is not the case, adding high(),low() may result in confusion
>>>> between
>>>> AdjoiningVirtualSpaces::high() and low(). So use other name for
>>>> current HeteroVirtualSpaces::high()/low().
>>>>
>>>>> AdjoiningVirtualSpaces contains two adjacent virtual spaces in the
>>>>> same
>>> reserved space and separated by a boundary. That’s why the name 'high'
>>> and 'low'.
>>>>> The class I added - HeteroVirtualSpaces, are not adjacent and do
>>>>> not
>>> share same reserved space. So I have names them '_young_vs' and
>> '_old_vs'.
>>> But from outside, i.e, users of VirtualSpaces, they call high() and
>>> low() to access these spaces. So I have not changed the function names.
>>>
>>>> -----Original Message-----
>>>> From:sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
>>>> Sent: Monday, September 17, 2018 11:32 AM
>>>> To: Kharbas, Kishor<kishor.kharbas at intel.com>; Thomas Stüfe
>>>> <thomas.stuefe at gmail.com>; Thomas Schatzl
>>> <thomas.schatzl at oracle.com>
>>>> Cc:hotspot-gc-dev at openjdk.java.net; Hotspot dev runtime <hotspot-
>>>> runtime-dev at openjdk.java.net>; Viswanathan, Sandhya
>>>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>>>> <shirish.aundhe at intel.com>
>>>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>>>> ReserveSpace.cpp changes are mostly eliminated/no collector specific
>>> code.
>>>> Hi Kishor,
>>>>
>>>>
>>>> On 9/4/18 10:41 PM, Kharbas, Kishor wrote:
>>>>> Hi,
>>>>> I have uploaded implementation for parallel scavenge at
>>>>> http://cr.openjdk.java.net/~kkharbas/8204908/webrev-8204908.00
>>>>> Majority of the implementation is handled in two new files -
>>>> adjoiningGenerationsForHeteroHeap.cpp and
>>> psFileBackedVirtualspace.cpp.
>>>> Would love to hear your feedback and suggestions.
>>>> Sorry for late review.
>>>>
>>>> ----------------
>>>> General comments.
>>>>
>>>> 1. Looks like this patch is not based on latest repository, as it
>>>> fails with - Wreorder issue.
>>>>
>>>> 2. I see many wrong alignment location of having only 2 spaces after
>>>> new line that is continued.
>>>> e.g.
>>>> a->b(c,
>>>> __d, xxxx); // 2 spaces
>>>> this should be
>>>> a->b(c,
>>>> _____d, xxx); // 'd' should locate under 'c' as those are in the same
>> context.
>>>> 3. I see assertion failures with below options combinations. There
>>>> could be more... Please run jtreg tests before posting webrev.
>>>> -XX:+UseLargePages -XX:+UseSHM -version -XX:+UseLargePages
>>>> -XX:+UseNUMA -version -XX:+UseLargePages - XX:AllocateHeapAt=.
>>>> -version
>>>>
>>>> =========================
>>>> src/hotspot/share/gc/parallel/adjoiningGenerations.cpp
>>>> - Copyright update
>>>>
>>>> 43 _virtual_spaces(new AdjoiningVirtualSpaces(old_young_rs,
>>>> policy->min_old_size(),
>>>> 44 policy->min_young_size(), alignment) ) {
>>>> - Wrong alignment?
>>>>
>>>>
>>>> =========================
>>>> src/hotspot/share/gc/parallel/adjoiningGenerations.hpp
>>>> - Copyright update
>>>>
>>>> 62 AdjoiningGenerations();
>>>> - Why we need this ctor?
>>>>
>>>>
>>>> =========================
>>>> /src/hotspot/share/gc/parallel/adjoiningVirtualSpaces.hpp
>>>> - Copyright update
>>>>
>>>> 88 virtual PSVirtualSpace* high() { return _high; }
>>>> 89 virtual PSVirtualSpace* low() { return _low; }
>>>> - Those methods don't need 'virtual' specifier as high()/low()
>>>> methods are only used at ctor of AdjoiningGenerations. The other
>>>> calling site is ctor of AdjoiningGenerationsForHeteroHeap but it is
>>>> used another type of
>>>> high()/low() which returns _young_vs or _old_vs.
>>>>
>>>>
>>>> =========================
>>>> src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp
>>>> - no comments.
>>>>
>>>>
>>>> =========================
>>>> src/hotspot/share/gc/parallel/psOldGen.cpp
>>>> =========================
>>>> src/hotspot/share/gc/parallel/psOldGen.hpp
>>>> 32 #include "gc/parallel/psFileBackedVirtualspace.hpp"
>>>> - Include this header file at cpp seems better rather than hpp.
>>>>
>>>> =========================
>>>> src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.cpp
>>>> 1 /*
>>>> 2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
>>>> 3 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE
>>> HEADER.
>>>> - Wrong alignment. The second/below star should locate under first
>>>> line
>>> star.
>>>> - Similarily there are more wrong alignment locations.
>>>> . Line 49, 50
>>>> . 52, 53
>>>> . 54, 55
>>>> . 64, 65
>>>> . 67, 68 69 70
>>>> . 72, 73 74 75 76
>>>> . 79, 80
>>>> . 81, 82
>>>> . 85, 86
>>>> . 87, 88
>>>> . 106, 107 108 109
>>>> . 114, 115
>>>> . 166, 167 168
>>>> . 178, 179 180
>>>> . 186, 187 188
>>>> . 218, 219 220
>>>> . 230, 231 232
>>>> . 239, 240 241
>>>>
>>>>
>>>> 59 // Initialize the virtual spaces. Then pass the
>>>> 60 // a virtual to each generation for initialization of the
>>>> - Then pass 'the a' virtual to each generation. 'the a'?
>>>>
>>>> 64 (static_cast
>>>> <HeteroVirtualSpaces*>(_virtual_spaces))->initialize(max_old_byte_si
>>>> ze
>>>> ,
>>>> init_old_byte_size,
>>>> 65 init_young_byte_size);
>>>> - Just making 'initilize' method as 'virtual' seems better.
>>>>
>>>> 39
>>>>
>> AdjoiningGenerationsForHeteroHeap::AdjoiningGenerationsForHeteroHeap(
>>>> ReservedSpace
>>>> old_young_rs, size_t total_size_limit,
>>>> 40 GenerationSizer* policy, size_t alignment) :
>>>> _total_size_limit(total_size_limit) {
>>>> - Wrong alirnment at line 40. 'Generation' should be under
>> 'ReservedSpace'
>>>> at line 39.
>>>>
>>>> 49 _virtual_spaces = new HeteroVirtualSpaces(old_young_rs,
>>>> min_old_byte_size, 70 (static_cast
>>>> <HeteroVirtualSpaces*>(_virtual_spaces))->max_young_size());
>>>> 75 (static_cast
>>>> <HeteroVirtualSpaces*>(_virtual_spaces))->max_old_size(),
>>>> - Instead assigning at line 49 and then cast to
>>>> HeteroVirtualSpaces*, create/initialize HeteroVirtualSpaces, get
>>>> max_young/old_size() and the assign to _virtual_spaces would be better
>> alternative.
>>>> 109 _min_young_byte_size(min_yg_byte_size),
>>>> _max_total_size(max_total_size) {
>>>> 110 _max_old_byte_size = _max_total_size - _min_young_byte_size;
>>>> 111 _max_young_byte_size = _max_total_size - _min_old_byte_size;
>>>> - _max_old_byte_size and _max_young_byte_size can move to
>>>> initialization list.
>>>>
>>>> 117 // This the reserved space exclusively for old generation.
>>>> 122 // This the reserved space exclusively for young generation.
>>>> - Missing 'is'? i.e. // This *is* the reserved space exclusively for
>>>> old generation.
>>>>
>>>> 131 vm_exit_during_initialization("Could not reserve enough space
>> for "
>>>> 132 "object heap");
>>>> - 'object heap' can stay same line. Or changing align is necessary.
>>>>
>>>> 137 vm_exit_during_initialization("Could not reserve enough space
>> for "
>>>> 138 "object heap");
>>>> - 'object heap' can stay same line. Or changing align is necessary.
>>>>
>>>> 152 if (uncommitted_in_old > 0) {
>>>> - This condition seems redundant as uncommitted_in_old is type of
>> size_t.
>>>> 159 if (bytes_needed == 0) {
>>>> 160 return true;
>>>> 161 }
>>>> - This condition, can move to inside of 'if (uncommitted_in_old > 0)'
>>>> condition because if uncommitted_in_old is zero, there's no way
>>>> bytes_needed to be zero - bytes_needed is guaranteed not to be zero
>>>> from caller site, so safe to move between line 154 and 155. The
>>>> condition to return true is 'uncommitted_size == bytes_needed &&
>>>> success of expand_by()'.
>>>>
>>>> 176 bool ret = _young_vs->shrink_by(shrink_size);
>>>> 177 assert(ret, "We should be able to shrink young space");
>>>> - I would prefer to add more conditions below if we fails to shrink.
>>>> i.e. just assert seems not enough.
>>>>
>>>> 189 _old_vs->expand_by(bytes_to_add_in_old);
>>>> - Check the return value of expand_by().
>>>>
>>>> 211 if (bytes_needed == 0) {
>>>> 212 return true;
>>>> 213 }
>>>> - Same as 'adjust_boundary_down()'
>>>>
>>>> 244 DEBUG_ONLY(size_t total_size_after =
>>>> _young_vs->reserved_size()
>>>> + _old_vs->reserved_size());
>>>> 245 assert(total_size_after == total_size_before, "should be
>>>> equal");
>>>> - Why adjust_boundary_up() doesn't have this kind of check?
>>>>
>>>>
>>>> =========================
>>>> src/hotspot/share/gc/parallel/adjoiningGenerationsForHeteroHeap.hpp
>>>> 1 /*
>>>> 2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
>>>> 3 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE
>>> HEADER.
>>>> 4 *
>>>> - Wrong alignment. The second/below star should locate under first
>>>> line
>>> star.
>>>> 37 size_t total_size_limit() {
>>>> 38 return _total_size_limit;
>>>> 39 }
>>>> - I don't think we need this but if you prefer to have, add 'const'.
>>>>
>>>> 45 PSVirtualSpace* _young_vs;
>>>> 46 PSVirtualSpace* _old_vs;
>>>> - Can't we use 'AdjoiningVirtualSpaces::_low' and '_high' instead?
>>>> If it is not the case, adding high(),low() may result in confusion
>>>> between
>>>> AdjoiningVirtualSpaces::high() and low(). So use other name for
>>>> current HeteroVirtualSpaces::high()/low().
>>>>
>>>> 55 HeteroVirtualSpaces(ReservedSpace rs,
>>>> 56 size_t min_old_byte_size,
>>>> 57 size_t min_young_byte_size, size_t max_total_size,
>>>> 58 size_t alignment);
>>>> - Wrong alignment. Line 56 ~ 58 should be same as the location of
>>>> 'ReservedSpace' at line 55.
>>>>
>>>> 67 size_t max_young_size() { return _max_young_byte_size; }
>>>> 68 size_t max_old_size() { return _max_old_byte_size; }
>>>> - 'const'?
>>>>
>>>> 70 void initialize(size_t initial_old_reserved_size, size_t
>>>> init_low_byte_size,
>>>> 71 size_t init_high_byte_size);
>>>> - Wrong alignment
>>>>
>>>> 82 size_t reserved_byte_size();
>>>> - 'const'?
>>>>
>>>> =========================
>>>> src/hotspot/share/gc/parallel/psFileBackedVirtualspace.cpp
>>>> 1 /*
>>>> 2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
>>>> - Wrong alignment. The second/below star should locate under first
>>>> line
>>> star.
>>>> 36 if (_fd == -1) {
>>>> 37 return;
>>>> 38 }
>>>> - I prefer to have 'initialize()' method to handle when fails to
>>>> create the heap file. Current implementation seems easy to call
>>>> additional
>>> 'initialize()'.
>>>> Ctor of PSVirtualSpace doesn't have any failure path(e.g.
>>>> allocation), so no further check is needed around line 18:psOldGen.cpp.
>>>> But yours is different, so something should be checked.
>>>>
>>>> 33 _mapping_succeeded = false;
>>>> 34 _file_path = path;
>>>> 46 _mapping_succeeded = true;
>>>> 47 _special = true;
>>>> - Can move to initialization list with proper initial value.
>>>>
>>>> 55 assert(special(), "_special should always be true for
>>>> PSFileBackedVirtualSpace");
>>>> 66 assert(special(), "_special should always be true for
>>>> PSFileBackedVirtualSpace");
>>>> - For these 2, can we have more specific message? e.g. to include
>>>> meaning of expand or shrink.
>>>>
>>>>
>>>> =========================
>>>> src/hotspot/share/gc/parallel/psFileBackedVirtualspace.hpp
>>>> 1 /*
>>>> 2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
>>>> - Wrong alignment. The second/below star should locate under first
>>>> line
>>> star.
>>>> 37 PSFileBackedVirtualSpace(ReservedSpace rs, const char*
>>>> file_path);
>>>> 38 bool expand_by(size_t bytes);
>>>> - Just recommendation to add new line between 37 and 38.
>>>>
>>>> 43 #endif //
>>> SHARE_VM_GC_PARALLEL_PSFILEBACKEDVIRTUALSPACE_HPP
>>>> - Last line is not terminated with a newline.
>>>>
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>> I will post G1 GC implementation in a few days.
>>>>>
>>>>> Thanks
>>>>> Kishor.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From:sangheon.kim at oracle.com
>> [mailto:sangheon.kim at oracle.com]
>>>>>> Sent: Thursday, August 30, 2018 4:06 PM
>>>>>> To: Kharbas, Kishor<kishor.kharbas at intel.com>; Thomas Stüfe
>>>>>> <thomas.stuefe at gmail.com>; Thomas Schatzl
>>>> <thomas.schatzl at oracle.com>
>>>>>> Cc:hotspot-gc-dev at openjdk.java.net; Hotspot dev runtime
>>>>>> <hotspot-runtime-dev at openjdk.java.net>; Viswanathan, Sandhya
>>>>>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>>>>>> <shirish.aundhe at intel.com>
>>>>>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>>>>>> ReserveSpace.cpp changes are mostly eliminated/no collector
>>>>>> specific
>>>> code.
>>>>>> Hi Kishor,
>>>>>>
>>>>>> On 8/30/18 11:55 AM, Kharbas, Kishor wrote:
>>>>>>> Hi Sangheon,
>>>>>>>
>>>>>>> So far I don’t see a need to do so. I will post my
>>>>>>> implementation code
>>>>>> today or tomorrow, if we see a need or any simplification by
>>>>>> changing classes in VirtualSpace.hpp, we can discuss that.
>>>>>> Okay.
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>>
>>>>>>
>>>>>>> Regards,
>>>>>>> -Kishor
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From:sangheon.kim at oracle.com
>>> [mailto:sangheon.kim at oracle.com]
>>>>>>>> Sent: Wednesday, August 29, 2018 10:17 AM
>>>>>>>> To: Kharbas, Kishor<kishor.kharbas at intel.com>; Thomas Stüfe
>>>>>>>> <thomas.stuefe at gmail.com>; Thomas Schatzl
>>>>>> <thomas.schatzl at oracle.com>
>>>>>>>> Cc:hotspot-gc-dev at openjdk.java.net; Hotspot dev runtime
>>>>>>>> <hotspot-runtime-dev at openjdk.java.net>; Viswanathan, Sandhya
>>>>>>>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>>>>>>>> <shirish.aundhe at intel.com>
>>>>>>>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>>>>>>>> ReserveSpace.cpp changes are mostly eliminated/no collector
>>>>>>>> specific
>>>>>> code.
>>>>>>>> Hi Kishor,
>>>>>>>>
>>>>>>>> On 8/29/18 9:52 AM, Kharbas, Kishor wrote:
>>>>>>>>> Hi Sangheon,
>>>>>>>>>
>>>>>>>>> Thanks for reviewing the design.
>>>>>>>>> Since the collectors do not use them for heap memory, I did
>>>>>>>>> not have to override VirtualSpace
>>>>>>>> Sorry, I meant ReservedSpace and its friends at
>>>>>>>> share/memory/virtualspace.hpp.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Sangheon
>>>>>>>>
>>>>>>>>
>>>>>>>>> -Kishor
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From:sangheon.kim at oracle.com
>>>> [mailto:sangheon.kim at oracle.com]
>>>>>>>>>> Sent: Tuesday, August 28, 2018 2:38 PM
>>>>>>>>>> To: Kharbas, Kishor<kishor.kharbas at intel.com>; Thomas Stüfe
>>>>>>>>>> <thomas.stuefe at gmail.com>; Thomas Schatzl
>>>>>>>> <thomas.schatzl at oracle.com>
>>>>>>>>>> Cc:hotspot-gc-dev at openjdk.java.net; Hotspot dev runtime
>>>>>>>>>> <hotspot-runtime-dev at openjdk.java.net>; Viswanathan,
>> Sandhya
>>>>>>>>>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>>>>>>>>>> <shirish.aundhe at intel.com>
>>>>>>>>>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>>>>>>>>>> ReserveSpace.cpp changes are mostly eliminated/no collector
>>>>>>>>>> specific
>>>>>>>> code.
>>>>>>>>>> Hi Kishor,
>>>>>>>>>>
>>>>>>>>>> On 8/21/18 10:57 AM, Kharbas, Kishor wrote:
>>>>>>>>>>> Hi All,
>>>>>>>>>>>
>>>>>>>>>>> Thank you for your valuable comments and feedback in this
>>>>>>>>>>> thread so
>>>>>>>> far.
>>>>>>>>>>> After taken in all the comments and reading thoroughly
>>>>>>>>>>> through the code, I
>>>>>>>>>> feel that the complexity added to virtualSpace.cpp is due to
>>>>>>>>>> lack of abstraction in VirtualSpace and at GC level. NV-DIMM
>>>>>>>>>> size and file paths are being passed all the way to OS calls.
>>>>>>>>>>> So I went back to the drawing board and created a high level
>>>>>>>>>>> design to remove all the pain points of current implementation.
>>>>>>>>>>> I have uploaded the design at
>>>>>>>>>>>
>> http://cr.openjdk.java.net/~kkharbas/8202286/Design%20for%20JEP%20JDK
>>>>>>>>>> -
>>>>>>>>>>> 8202286.pdf I would love to hear your feedback and
>> suggestions.
>>>>>>>>>>> Once we get confidence in the design, I will work on the
>>>>>> implementation.
>>>>>>>>>> The design looks good to me.
>>>>>>>>>> If you are planning to change VirtualSpace at
>>>>>>>>>> share/memory/virtualspace.hpp, including it on the design
>>>>>>>>>> document would be helpful too.
>>>>>>>>>>
>>>>>>>>>> Probably folks involved in the previous discussions would say
>>>>>>>>>> whether the design well covers their concerns or not.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Sangheon
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> PS:
>>>>>>>>>>> 1. Vinay has transitioned to another team in Intel, so he
>>>>>>>>>>> won't be able to
>>>>>>>>>> continue on this jep.
>>>>>>>>>>> 2. If you feel this should be part of JEP discussion thread,
>>>>>>>>>>> we can take it
>>>>>>>>>> there.
>>>>>>>>>>> Thanks and regards,
>>>>>>>>>>> Kishor
>>>>>>>>>>>
>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
>>>>>>>>>>>> Sent: Friday, June 22, 2018 9:25 AM
>>>>>>>>>>>> To: Thomas Schatzl<thomas.schatzl at oracle.com>
>>>>>>>>>>>> Cc: Awasthi, Vinay K<vinay.k.awasthi at intel.com>; Paul Su
>>>>>>>>>>>> <paul.su at oracle.com>;hotspot-gc-dev at openjdk.java.net;
>>>> Hotspot
>>>>>>>> dev
>>>>>>>>>>>> runtime<hotspot-runtime-dev at openjdk.java.net>;
>>> Viswanathan,
>>>>>>>>>> Sandhya
>>>>>>>>>>>> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
>>>>>>>>>>>> <shirish.aundhe at intel.com>; Kharbas, Kishor
>>>>>>>>>>>> <kishor.kharbas at intel.com>
>>>>>>>>>>>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC -
>>>>>>>>>>>> ReserveSpace.cpp changes are mostly eliminated/no collector
>>>>>>>>>>>> specific
>>>>>>>>>> code.
>>>>>>>>>>>> Hi Thomas,
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Jun 22, 2018 at 4:44 PM, Thomas Schatzl
>>>>>>>>>>>> <thomas.schatzl at oracle.com> wrote:
>>>>>>>>>>>>> Hi Thomas,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, 2018-06-19 at 13:40 +0200, Thomas Stüfe wrote:
>>>>>>>>>>>>>> Hi Vinay,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, Jun 18, 2018 at 6:47 PM, Awasthi, Vinay K
>>>>>>>>>>>>>> <vinay.k.awasthi at intel.com> wrote:
>>>>>>>>>>>>>>> Hi Thomas,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Os::commit_memory calls map_memory_to_file which is
>>> same
>>>>>> as
>>>>>>>>>>>>>>> os::reserve_memory.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I am failing to see why os::reserve_memory can call
>>>>>>>>>>>>>>> map_memory_to_file (i.e. tie it to mmap) but
>>>> commit_memory
>>>>>>>>>> can't...
>>>>>>>>>>>>>>> Before this patch, commit_memory never dealt with
>>>>>>>>>>>>>>> incrementally committing pages to device so there has to
>>>>>>>>>>>>>>> be a way to pass file descriptor and offset. Windows has
>>>>>>>>>>>>>>> no such capability to manage incremental commits. All
>>>>>>>>>>>>>>> other OSes do and that is why map_memory_to_file is used
>>>>>>>>>>>>>>> (which
>>>> by
>>>>>>>>>>>>>>> the way also works on Windows).
>>>>>>>>>>>>>> AIX uses System V shared memory by default, which follows
>>>>>>>>>>>>>> a different allocation scheme (semantics more like
>>>>>>>>>>>>>> Windows
>>>>>>>>>> VirtualAlloc...
>>>>>>>>>>>>>> calls).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> But my doubts are not limited to that one, see my earlier
>>>>>>>>>>>>>> replies and those of others. It really makes sense to
>>>>>>>>>>>>>> step back one step and discuss the JEP first.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> There is already a discussion thread as David mentioned
>>>>>>>>>>>>> (http://mail.op
>>>>>>>>>>>>> enjdk.java.net/pipermail/hotspot-gc-dev/2018-
>>>> May/022092.html)
>>>>>>>> that
>>>>>>>>>>>>> so far nobody answered to.
>>>>>>>>>>>>>
>>>>>>>>>>>> Ah, I thought he wanted to have the JEP discussed in the
>>>>>>>>>>>> comments section of the JEP itself.
>>>>>>>>>>>>
>>>>>>>>>>>>> I believe discussion about contents the JEP and the
>>>>>>>>>>>>> implementation should be separate.
>>>>>>>>>>>>>
>>>>>>>>>>>> makes sense.
>>>>>>>>>>>>
>>>>>>>>>>>>> So far what I gathered from the responses to the
>>>>>>>>>>>>> implementation, the proposed idea itself is not the issue
>>>>>>>>>>>>> here (allowing the use of NVDIMM memory for parts of the
>>>>>>>>>>>>> heap for allowing the use of larger heaps to improve
>>>>>>>>>>>>> overall performance; I am not saying that the text doesn't
>>>>>>>>>>>>> need a bit more work :) ), but rather how an
>>>>>>>>>>>>> implementation of this JEP
>>>> should proceed.
>>>>>>>>>>>> I have no problem with adding NVDIMM support. I think it is
>>>>>>>>>>>> a cool
>>>>>>>>>> feature.
>>>>>>>>>>>> Also, the awkwardness off the memory management
>> abstraction
>>>>>> layer
>>>>>>>>>>>> in hotspot has always been a sore point with me (I
>>>>>>>>>>>> originally implemented the AIX mm layer in os_aix.cpp, and
>>>>>>>>>>>> those are painful memories). So, I have a lot of sympathy
>>>>>>>>>>>> for Vinays
>>>> struggles.
>>>>>>>>>>>> Unfortunately not much time atm, but I will respond to your
>>> mail.
>>>>>>>>>>>>> Let's discuss the non-implementation stuff in that thread.
>>>>>>>>>>>>> Vinay or I can repost the proposal email if you do not
>>>>>>>>>>>>> have it any more so that answers will be in-thread.
>>>>>>>>>>>>>
>>>>>>>>>>>> Okay, sounds good.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Thomas
>>>>>>>>>>>>
>>>>>>>>>>>> (one of us should change his name to make this less
>>>>>>>>>>>> confusing
>>>>>>>>>>>> :-)
>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Thomas
>>>>>>>>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181010/a297eeed/attachment.htm>
More information about the hotspot-gc-dev
mailing list