RFR(M): 8171181: Supporting heap allocation on alternative memory devices

[Kharbas, Kishor]

Hi David!
Thanks a lot for the review, I will work on your comments and from others. Will provide an update soon!

I only have a few clarifications -

1. On you comment 
	> In os::create_file_for_heap:
	>   - the warning should include the reason for the failure. Though it is unclear
	> whether this should be a warning or a fatal error.
    My thinking was to let the calling function in VirtualSpace.cpp handle the correct action. And it does terminate the application.

2. > In os::map_memory_to_file:
     >   - report reasons for failure explicitly

     Do you mean the message in "vm_exit_during_initialization(err_msg("Error in mapping Java heap at the given filesystem directory"));" should be more explicit?

Thanks
Kishor

> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Monday, February 27, 2017 5:41 PM
> To: Kharbas, Kishor <kishor.kharbas at intel.com>; hotspot-runtime-
> dev at openjdk.java.net
> Subject: Re: RFR(M): 8171181: Supporting heap allocation on alternative
> memory devices
> 
> Hi Kishor,
> 
> I took a quick look at this and have a few initial comments ... but overall it
> looks pretty straight-forward.
> 
> On 28/02/2017 9:52 AM, Kharbas, Kishor wrote:
> > Hello all!
> > I have proposed a JEP to support allocation of Java heap on new memory
> devices such as NV-DIMMs like Intel's 3D XPoint.
> > Such a feature has applications in multi-JVM deployments, big data
> applications and in-memory databases.
> > Please find the details here -
> > https://bugs.openjdk.java.net/browse/JDK-8171181 and the patch for the
> > implementation is at
> > http://cr.openjdk.java.net/~kkharbas/8153327_webrev.04
> > I would appreciate a review and feedback from the community for this JEP.
> 
> Is there something linux specific about the implementation? If not it should
> be in os_posix.cpp and be usable by all the POSIX compliant OS.
> 
> In os::create_file_for_heap:
> 
>   - you need to use the per-thread signal masking function, pthread_sigmask,
> not sigprocmask. Also you need restore the signal mask before the error
> return of -1
>   - you can't use umask as that might impact files concurrently created by
> other code in the VM. I expect you will need to create it then fchmod it.
>   - the warning should include the reason for the failure. Though it is unclear
> whether this should be a warning or a fatal error.
>   - check returns codes from library calls - add assertions that they succeed if
> they should be "impossible" to fail. Though I think perhaps an unlink failure
> may need to be anticipated.
> 
> In os::map_memory_to_file:
>   - report reasons for failure explicitly
>   - do not assign posix_fallocate to errno!
> 
> The err_msg function is used for printf-style formatting, so you don't need it
> for simple string messages. But in some cases your messages should also
> include strerror output when there are failures.
> 
> Some tests will also be needed of course.
> 
> Thanks,
> David
> 
> > Thank you!
> > Kishor
> >