Hi Alan,
Thanks for looking at this and apologies for the delayed response (c/o
our annual Red Hat OpenJDK team meeting taking place all last week).
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.
That might well be a cleaner, simpler model. However, there are two
issues with it:
Firstly, there does not appear to be a supported way to identify whether
or not a device -- or a file associated with the device -- is mappable
using MAP_SYNC. A hack would be to attempt a map call with MAP_SYNC and
see if it failed with a suitably revealing errno code but that seems
rather gross and, perhaps, unreliable (the same errno code might occur
for other reasons).
Secondly, it may not always be the case that users will want to map an
NVM device with MAP_SYNC and rely on explicit line-by-line writeback. If
a user only needs block-level syncing then she might prefer to map the
file as a normal device and rely on force to perform block level
writeback via the driver.
> 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).
I will be happy to perform this trimming down. The detail was merely to
ensure the draft implementation was understandable.
> 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 am not familiar with BufferPoolMXBean but I will investigate.
I'm agnostic as to whether to track NVM buffers separate from other
buffers but I am happy to consider adding this as a requirement.
However, before doing so, can you explain further why there is a need to
keep this case separate? In particular, I am puzzled by your use of
"mapped". The NVM memory is definitely mapped although obviously it is
of a different type to say private mapped volatile memory or a mapped
disk file. Also, are these latter cases distinguished as far as
BufferPoolMXBean bean counting is concerned? NVM seems to me to fall
somewhere between the two.
> I can't tell from this thread if you are looking for detailed feedback
> on the current patch. A couple of random things:
I guess its not the primary concern (which is to get the JEP submitted)
but comments are welcome now or later.
> - 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.
Ok, I'll post an update when I have trimmed the implementation details
the JEP and tweaked the code according to the above comments. Thank you
for your feedback.
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
