RFR: 8207851: Implement JEP 352

[Andrew Dinn]

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