[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-nio-dev
Subject:    Re: RFR: 8224974: Implement JEP 352
From:       Andrew Dinn <adinn () redhat ! com>
Date:       2019-06-24 11:35:34
Message-ID: 8f72d3b5-3a01-a7de-3b09-35571266a87d () redhat ! com
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]