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

Tue Jun 19 11:40:22 UTC 2018

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.

BTW, I openend https://bugs.openjdk.java.net/browse/JDK-8205335, could
you please take a look?

Thanks, Thomas

>
> Thanks,
> Vinay
>
> -----Original Message-----
> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> Sent: Saturday, June 16, 2018 12:01 PM
> To: Awasthi, Vinay K <vinay.k.awasthi at intel.com>
> Cc: Thomas Schatzl <thomas.schatzl at oracle.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 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
>>