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.
