RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Alan Bateman]

On 25/07/2018 12:44, Andrew Dinn wrote:
> Round 2
> -------
> I have updated the JEP and uploaded a revised webrev in the light of
> existing feedback
>
>    JEP JIRA: https://bugs.openjdk.java.net/browse/JDK-8207851
>
>    Formatted JEP: http://openjdk.java.net/jeps/8207851
>
>    New webrev: http://cr.openjdk.java.net/~adinn/pmem/webrev.02/
>
> I would welcome any further comments (I guess I'd be happy to push this
> as is but I find it hard to believe it does not require more work.
> Someone must have something to add :-)
>
I read through the draft JEP and also skimmed the latest webrev.

The current iteration, to introduce new MapMode values, is not too bad 
but it makes me wonder if we could avoid new modes altogether by 
detecting that the file is backed by NVM. Is there a fcntl cmd or some 
other syscall that can be used to detect this? I'm asking because it 
would open the potential to discuss limiting the API changes to just 
MappedByteBuffer.

In the draft JEP then the Summary, Goals, Non-Goals, Success Metrics, 
and Motivation sections read well. The Description section is very wordy 
and includes a lot of implementation detail that I assume will be 
removed before it is submitted (my guess is that it can be distilled 
down to a couple of paragraphs). As a comparison, the API surface in JEP 
337 is much larger but we were able to reduce the text down to a few 
paragraphs. The Testing sectioning highlights the challenges and reminds 
me that we have several features that will need maintainers to 
continuously test to ensure that a feature doesn't bit rot (SCTP and the 
proposed APIs for RDMA sockets are in the same boat).

Are you familiar with BufferPoolMXBean which can be used by management 
tools to monitor pool of buffers? I'm wondering if we should introduce a 
new pool for NVM so that it doesn't show up in the "mapped" pool.

I can't tell from this thread if you are looking for detailed feedback 
on the current patch. A couple of random things:
- there are residual references to map_persistent in several places
- MappedByteBuffer.force will need to specify IAE
- The new methods are missing @since
- I think it would be clearer if MappedByteBuffer.force use "region" 
rather than "sub-region" as it is used in the context of the buffer, not 
the original file.
- There will be round of clean up needed to the changes to java.base to 
get the javadoc and code consistent with the javadoc/code in this area. 
I assume we'll get back to that when the patch is closer to review.

-Alan.