Hi Alan, I have uploaded a webrev which I believe answers all outstanding review comments:
JIRA: https://bugs.openjdk.java.net/browse/JDK-8224974 webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.08/ This includes the changes Joe Darcy highlighted in his code review of the implementation CSR (module header comment plus @since 14 changes in Unsafe.java). It also allows for the tweaks to the force API documentation (JDK-8226203). So, I think the only work still outstanding is to agree changes to the JEP wording (as proposed in my previous post -- see below) plus endorsement and targeting of the JEP. regards, Andrew Dinn ----------- On 18/06/2019 12:43, Andrew Dinn wrote: > Hi Alan, > > Thanks for reviewing the JEP one more time. > > On 16/06/2019 09:47, Alan Bateman wrote: >> I re-read the JEP, trying to put myself in the position of someone >> reading it for the first time in 2020. >> >> Summary section: >> >> What would you think about replacing this with a sentence that makes it >> clear that the JEP adds new JDK-specific file mapping modes to allow the >> FileChannel API create MappedByteBuffers over non-volatile memory? > > I would be happy with that way of describing the behaviour change except > that what you suggest doesn't mention actually making it work -- which > /is/ part of the JEP. How about: > > "Add new JDK-specific file mapping modes, allowing the FileChannel API > to be used to create MappedByteBuffers over non-volatile memory, and > extend the underlying implementation to support this new use case" > >> Goals section: >> >> I think the first paragraph could be re-worded to make it clear that the >> goal is to use the existing MappedByteBuffer API to access NVM. > > Yes indeed: > > "This JEP proposes that class MappedByteBuffer be reworked to support > access to non-volatile memory (NVM). The only API change required is a > new enumeration employed by FileChannel clients to request mapping of a > file located on an NVM-backed file system rather than a conventional, > file storage system. Recent changes to the MappedByteBufer API mean that > it supports all the behaviours needed to allow direct memory updates and > provide the durability guarantees needed for higher level, Java client > libraries to implement persistent data types (e.g. block file systems, > journaled logs, persistent objects, etc.). The implementations of > FileChannel and MappedByteBuffer need revising to be aware of this new > backing type for the mapped file." > >> Paragraphs 2-5 split this into two sub-goals. The first suggests that it >> extends the MBB API but this is no longer the case. The second also >> hints that it adds a new API. I think these two need to be re-worded. > > Agreed. How about this: > > "The primary goal of this JEP is to ensure that clients can access and > update NVM from a Java program efficiently and coherently. A key element > of this goal is to ensure that individual writes (or small groups of > contiguous writes) to a buffer region can be committed with minimal > overhead i.e. to ensure that any changes which might still be in cache > are written back to memory." > > n.b. I snuck in the word coherence because I thought it clarified the > notion of committing to memory. > >> Goal 3 and 4 are okay, although I think the 4th could be summarized as >> allowing mapped buffers on NVM to be tracked by the existing monitoring >> and management APIs. > > Agreed. So, renumbering accordingly, how about this: > > "A second, subordinate goal is to implement this commit behaviour using > a restricted, JDK-internal API defined in class Unsafe, allowing it to > be re-used by classes other than MappedByteBuffer that may need to > commit NVM. > > A final, related goal is to allow buffers mapped over NVM to be tracked > by the existing monitoring and management APIs." > >> Description section >> >> It might be clearer of "Proposed Java API Changes" were renamed to >> "Proposed JDK-specific API changes". > > Agreed. > >> One other thing to mention is that I re-read the javadoc for the MBB >> force methods and I think we need to adjust one of the sentences in the >> existing (and new) methods to take account of implementation specific >> map modes. I've created JDK-8226203 [1] to track it. As support for >> implementation specific map modes is in new in Java SE 13 then it might >> be worth trying to get that fixed now while it is fresh in our minds. > Sure, I'll post an RFR with the javadoc patch as a separate step. Can it > go into jdk13? or is it too late for that? > > 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
