RFR(M): 8204908: NVDIMM for POGC and G1GC - ReserveSpace.cpp changes are mostly eliminated/no collector specific code.

David Holmes david.holmes at oracle.com
Mon Jun 18 05:35:45 UTC 2018

On 17/06/2018 5:00 AM, Thomas Stüfe wrote:
> Hi Vinay!
> this is the third thread you opened for this issue; it would helpful
> if you would not change subjects, because it splinters discussions on
> the mailing list.

+100 on that!

At this stage the JEP should be being discussed more than the prototype 
implementation. I see:


but zero discussion.


> For reference, this is the first mail thread with our first exchange:
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2018-June/022342.html
> ---
> Thank you for your perseverance and patience.
> But unfortunately, my concerns have not been alleviated a lot by the
> current patch. I still think this stretches an already
> partly-ill-defined interface further.
> About os::commit_memory(): as I wrote in my first mail:
> "So far, for the most part, the os::{reserve|commit}_memory APIs have
> been agnostic to the underlying implementation. You pretty much tie it
> to mmap() now. This adds implicit restrictions to the API we did not
> have before (e.g. will not  work if platform uses SysV shm APIs to
> implement these APIs)."
> In your response
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2018-June/022356.html
> you explain why you do this. I understand your motives. I still
> dislike this, for the reasons I gave before: it adds a generic API to
> a set of generic APIs which IMHO breaks the implicit contract they all
> have with each other.
> Your patch also blurrs the difference between runtime "os::" layer and
> GC. "os" is a general OS-wrapping API layer, and should not know nor
> care about GC internals:
> +  static inline int nvdimm_fd() {
> +    // ParallelOldGC adaptive sizing requires nvdimm fd.
> +    return _nvdimm_fd;
> +  }
> +  static inline address dram_heapbase() {
> +    return _dram_heap_base;
> +  }
> +  static inline address nvdimm_heapbase() {
> +    return _nvdimm_heap_base;
> +  }
> +  static inline uint nvdimm_regionlength() {
> +    return _nvdimm_region_length;
> +  }
> IMHO, currently the memory management is ill prepared for your patch;
> yes, one could shove it in, but at the expense of maintainability and
> code clearness. This expense would have to be carried by all JVM
> developers, regardless whether they work on your hardware and benefit
> from this feature.
> So I think this would work better with some preparatory refactoring
> done in the VM. Red Hat and Oracle did similar efforts by refactoring
> the GC interface before adding new GCs: see
> https://bugs.openjdk.java.net/browse/JDK-8163329.
> Maybe we could think about how to do this. It certainly would be a worthy goal.
> Kind Regards, Thomas
> (BTW, I really do not like the fact that in os_posix.cpp,
> os::map_memory_to_file() and os::allocate_file() do an
> vm_exit_during_initialization() in case of an error! These are
> (supposed to be) general purpose APIs and under no circumstances
> should they end the process. This is already in the hotspot now, added
> as part of JDK-8190308. This should be fixed.)
> On Fri, Jun 15, 2018 at 7:59 PM, Awasthi, Vinay K
> <vinay.k.awasthi at intel.com> wrote:
>> HI Thomas,
>> Thanks for your input..
>> Now there is *no* change in virtualspace.cpp...
>> I moved reserve and commit (this is how memory backed by file is handled) from reserve space  to commit places in respective gcs... All changes are again localized and isolated with os::has_nvdimm()/AllocateOldGenAT.
>> There are also fixes (1 line changes) added related to alignment and there is no un-mapping etc.. before mapping nvdimm backed dax file.
>> Full Patch patch is here..
>> http://cr.openjdk.java.net/~kkharbas/8204908/webrev.04
>> Any input is welcome.
>> Thanks,
>> Vinay
>> -----Original Message-----
>> From: Thomas Schatzl [mailto:thomas.schatzl at oracle.com]
>> Sent: Friday, June 15, 2018 6:53 AM
>> To: Awasthi, Vinay K <vinay.k.awasthi at intel.com>; 'Paul Su' <paul.su 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: Kharbas, Kishor <kishor.kharbas at intel.com>; Aundhe, Shirish <shirish.aundhe at intel.com>; Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>> Subject: Re: RFR(M): 8204908: NVDIMM for POGC and G1GC - ReserveSpace.cpp changes are mostly eliminated/no collector specific code.
>> Hi Vinay,
>> On Thu, 2018-06-14 at 20:49 +0000, Awasthi, Vinay K wrote:
>>> Now ReservedSpace.cpp has logic to only open NVDIMM File (as it was
>>> done for AllocateheapAt).. if successful, set up 3 flags
>>> (base/nvdimm_present/file handle) at the end. There is *NO* collector
>>> specific code.
>>> All work has been moved to g1PagebasedVirtualSpace.cpp.. I am
>>> committing memory here and setting dram_heapbase used by g1 here.
>>> JEP to support allocating Old generation on NV-DIMM - https://bugs.op
>>> enjdk.java.net/browse/JDK-8202286
>>> Here is the implementation bug link: https://bugs.openjdk.java.net/br
>>> owse/JDK-8204908
>>> Patch is Uploaded at (full patch/incremental patch)
>>> http://cr.openjdk.java.net/~kkharbas/8204908/webrev.02/
>>> http://cr.openjdk.java.net/~kkharbas/8204908/webrev.02_to_01/
>>> Tested default setup (i.e. no file is being passed for heap) and
>>> AllocateHeapAt/AllocateOldGenAt with POGC and G1GC.. all passing… Any
>>> and all comments are welcome!
>>    looking briefly through the changes, I think they look much better already to move the G1 specific stuff into G1 code; however I would like to think about how we could reduce the complexity further and solve the case of allowing multiple mapping sources (tmpfs file, nvram, different "types" of RAM) for different parts of the heap in an even cleaner way.
>> Thanks,
>>    Thomas

More information about the hotspot-runtime-dev mailing list