RFR(M): 8204908: NVDIMM for POGC and G1GC (original) (raw)

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.03to02 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) {

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 tier1gc and tier1runtime 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.02to01/ 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.01to00/ 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 youngvs or oldvs. 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* youngvs; 46 PSVirtualSpace* oldvs; - 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 'youngvs' and 'oldvs'. 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   virtualspaces(new AdjoiningVirtualSpaces(oldyoungrs, policy->minoldsize(), 44                   policy->minyoungsize(), 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 youngvs or oldvs. ========================= 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   (staticcast <HeteroVirtualSpaces*>(virtualspaces))->initialize(maxoldbytesi ze , initoldbytesize, 65     inityoungbytesize); - Just making 'initilize' method as 'virtual' seems better. 39 AdjoiningGenerationsForHeteroHeap::AdjoiningGenerationsForHeteroHeap( ReservedSpace oldyoungrs, sizet totalsizelimit, 40   GenerationSizer* policy, sizet alignment) : totalsizelimit(totalsizelimit) { - Wrong alirnment at line 40. 'Generation' should be under 'ReservedSpace' at line 39. 49   virtualspaces = new HeteroVirtualSpaces(oldyoungrs, minoldbytesize,  70     (staticcast <HeteroVirtualSpaces*>(virtualspaces))->maxyoungsize()); 75     (staticcast <HeteroVirtualSpaces*>(virtualspaces))->maxoldsize(), - Instead assigning at line 49 and then cast to HeteroVirtualSpaces*, create/initialize HeteroVirtualSpaces, get maxyoung/oldsize() and the assign to virtualspaces would be better alternative.  109   minyoungbytesize(minygbytesize), maxtotalsize(maxtotalsize) {  110   maxoldbytesize = maxtotalsize - minyoungbytesize;  111   maxyoungbytesize = maxtotalsize - minoldbytesize; - maxoldbytesize and maxyoungbytesize 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     vmexitduringinitialization("Could not reserve enough space for "  132       "object heap"); - 'object heap' can stay same line. Or changing align is necessary.  137     vmexitduringinitialization("Could not reserve enough space for "  138       "object heap"); - 'object heap' can stay same line. Or changing align is necessary.  152   if (uncommittedinold > 0) { - This condition seems redundant as uncommittedinold is type of sizet.  159   if (bytesneeded == 0) {  160     return true;  161   } - This condition, can move to inside of 'if (uncommittedinold > 0)' condition because if uncommittedinold is zero, there's no way bytesneeded to be zero - bytesneeded is guaranteed not to be zero from caller site, so safe to move between line 154 and 155. The condition to return true is 'uncommittedsize == bytesneeded && success of expandby()'.  176     bool ret = youngvs->shrinkby(shrinksize);  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   oldvs->expandby(bytestoaddinold); - Check the return value of expandby().  211   if (bytesneeded == 0) {  212     return true;  213   } - Same as 'adjustboundarydown()'  244   DEBUGONLY(sizet totalsizeafter = youngvs->reservedsize() + oldvs->reservedsize());  245   assert(totalsizeafter == totalsizebefore, "should be equal"); - Why adjustboundaryup() 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   sizet totalsizelimit() { 38     return totalsizelimit; 39   } - I don't think we need this but if you prefer to have, add 'const'. 45     PSVirtualSpace*    youngvs; 46     PSVirtualSpace*    oldvs; - 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       sizet minoldbytesize, 57       sizet minyoungbytesize, sizet maxtotalsize, 58       sizet alignment); - Wrong alignment. Line 56 ~ 58 should be same as the location of 'ReservedSpace' at line 55. 67     sizet maxyoungsize() { return maxyoungbytesize; } 68     sizet maxoldsize() { return maxoldbytesize; } - 'const'? 70     void initialize(sizet initialoldreservedsize, sizet initlowbytesize, 71       sizet inithighbytesize); - Wrong alignment 82   sizet reservedbytesize(); - '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   mappingsucceeded = false; 34   filepath = path; 46   mappingsucceeded = 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* filepath); 38   bool   expandby(sizet bytes); - Just recommendation to add new line between 37 and 38. 43 #endif // SHAREVMGCPARALLELPSFILEBACKEDVIRTUALSPACEHPP - 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::commitmemory calls mapmemorytofile which is same as os::reservememory. I am failing to see why os::reservememory can call mapmemorytofile (i.e. tie it to mmap) but commitmemory can't... Before this patch, commitmemory 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 mapmemorytofile 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 osaix.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

More information about the hotspot-runtime-dev mailing list