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

[Thomas Stüfe]

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.

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
>