Hi Alan, Round 4:
I have redrafted the JEP and updated the implementation in the light of your last 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.04/ n.b. the webrev is against the latest jdkdev head. n.b.b. there is only one change additional to those which respond to your feedback (all documented below). I had to redefine the x86 status bits used to tag presence of clflushopt and clwb insns (in class VM_Version). The ones I originally bagged were claimed by some new vector (VAES?) API -- which borked things spectacularly until worked ut what was going on. > On 06/08/18 10:29, Alan Bateman wrote: >> 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. I have not made this change for reasons outlined in my previous post. >> 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). The JEP is now slim and trim, omitting all details of the implementation. >> 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 have updated the JEP to require that stats for current persistent MappedByteBuffer instances (i.e. those mapped with MapMode.READ_ONLY_PERSISTENT or MapMode.READ_WRITE_PERSISTENT) are exposed via a new BufferPoolMXBean with name "mapped_persistent". This is additional to and separate from the bean & associated stats which currently detail other file-map derived MappedByteBuffer instances. The JEP requires this new bean to be exposed in the list retrieved by a call to ManagementFactory.getPlatformMXBeans(BufferPoolMXBean.class) >> - there are residual references to map_persistent in several places I think I have removed all of them from the code >> - MappedByteBuffer.force will need to specify IAE IAE was intended for the case where an attempt was made to map a PERSISTENT MapMode and the fd turned out not to be for an NVM. It is not possible to detect this reliably when running on a Linux kernels which do not implement MAP_SYNC + MAP_SHARED_PRIVATE. So, I changed the API to return an IOException in all cases, with an embedded error message indicating the problem as accurately as possible. That means there is no change to the method's exception signature, rather to the possible causes for the IOException. >> - The new methods are missing @since I think I have added these annotations in all places where they are needed. >> - 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. I think I fixed all occurrences. >> - 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. Sure, happy to do that when we get there. regards, Andrew Dinn ----------- Senior Principal Software Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
