Hi Alan, Thank you for the review.
On 26/05/2019 19:25, Alan Bateman wrote: > You may want to take a pass over the JEP to make sure that everything is > accurate. I notice, for example, the section on BufferPoolMXBean has the > old name READ_ONLY_PERSISTENT. We went through a couple of iterations in > the discussion here so there might be a few others. Thanks for spotting that. I corrected several wrong mentions of PERSISTENT instead of SYNC. I also coerced a mention of MAP_SYNC to render using a fixed width font. I also updated the description of the exception signature changes to FileChannel.map to explain how they will correlate with specific cases handled by this JEP implementation (or, rather, cases that are explicitly not handled by the implementation). Finally I updated the MXBean name as you suggested (see below). > I think the API changes are okay. I don't see a CSR yet but I assume > you'll get to that soon. Yes, I'll raise one ASAP. Could you clarify what changes I need to document in the CSR? Here are my current thoughts: ManagementFactoryHelper/FileChannelImpl I am assuming the change that exposes the new MXBean needs to be mentioned somwehere. However, that change doesn't actually affect any API. It just means that a new bean with a new name appears in the list of memory beans. I don't see anything which documents those bean names. Am I missing something? (probably :). com/sun/nio/file/ExtendedMapMode (in jdk.unsupported) I'm assuming the CSR needs to propose javadoc for the 2 exposed MapMode values, explaining what these modes are used for and which exceptions documented in FileChannel.map get thrown for the cases where their use is unsupported by the JVM or the OS, respectively. Is that correct? jdk/internal/misc/ExtendedMapMode (in java.base) Do I need to provide javadoc for the two new MapMode values and include them in the CSR? I was assuming not. FileChannelImpl method map The javadoc in FileChannel lists the new exceptions that might be thrown by this implementation but does not mention any specifics to say how they might relate to use of the XXX_SYNC MapModes. Do I need to propose updates for the FileChannel javadoc in the CSR or am I ok to provide that detail in the doc for com/sun/nio/file/ExtendedMapMode? Unsafe method writebackMemory I was assuming Unsafe.writebackMemory is internal to the JDK so does not need a mention in the CSR. Is that correct? > I've read through the changes to java.base and jdk.unsupported. > > Just a few minor points: > > - I assume the update FileChannel.java should be dropped as it's just a > left over from when we agreed to split out the updates to the Java SE API. Yes, that is a leftover. It has been removed from the latest patch. > - com.sun.nio.file.ExtendedMapMode.*_SYNC are missing javadoc, or rather > the descriptions are truncated with "...". I think this dates from when > were working out the right place to expose these constants. The source > file (and the internal ExtendedMapModes are missing copyright headers too). Thanks, I have added javadoc comments. > - We didn't discuss the name of the buffer pool that is exposed through > the JMX/management interface. We could take inspiration from the names > of the CodeHeap spaces that are exposed with MemoryPoolMXBeans as there > is an established convention for naming there, e.g. "mapped - > 'non-volatile memory'". The JEP used the name mapped_persistent" while the code named it mapped_sync. I have changed both to use the name "mapped - 'non-volatile memory'". Does this need further discussion by other parties? Or is that a final decision? > - Minor nit in Unmapper is that the methods to increment/decrement the > usage should use Java conventions so probably should be incrementUsage > and decrementUsage. Caught me red-handed. Also fixed. > - PmemTest. This is awkward and I wonder if it should be @run > main/manual rather than @ignore. Also `@modules jdk.unsupported` would > be useful to ensure it will be skipped if run with a test JDK that > doesn't have this module. Yes, I agree that @run main/manual is far better than @ignore and the @modules requirement is also a very good idea. I have updated both. New webrev: http://cr.openjdk.java.net/~adinn/8207851/webrev.01 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
