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.

Kharbas, Kishor kishor.kharbas at intel.com
Thu Oct 4 01:24:39 UTC 2018

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.

Full: http://cr.openjdk.java.net/~kkharbas/8211424/webrev-8204908.03 Incremental : http://cr.openjdk.java.net/~kkharbas/8211424/webrev-8204908.03_to_02

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, 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 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 > 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