RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)

[Alan Bateman]

On Mon, 2 Nov 2020 11:26:51 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> I looked through the changes in this update.
>> 
>> The shared memory segment support looks sound and the mechanism to close a shared memory segment is clever (albeit a bit surprising at first that it does global handshake to look for a frame in a scoped region. Also surprising that close can cause failure at both ends - it took me a while to see that this is pragmatic approach).
>> 
>> The share method specifies NPE if thread == null but there is no thread parameter, is this a cut 'n paste error? Another one in registerCleaner where it should be NPE if the cleaner is null.
>> 
>> I think the javadoc for the close method needs to be a bit clearer on the state of the memory segment when IllegalStateException is thrown. Will it be marked "not alive" when it fails? Does this mean there is a resource leak? I think an apiNote to explain the rational for why close is not idempotent is also needed, or maybe it should be re-visited so that close is a no-op when the memory segment is not alive.
>> 
>> Now that MemorySegment is AutoCloseable then maybe the term "alive" should be replaced with "open" or  "closed" and isAlive replaced with isOpen is isClosed.
>> 
>> FileDescriptor can be attraction nuisance and forced reference counting everywhere that it is used. Is it needed? Could an isMapped method work instead?
>> 
>> mapFromPath was in the second preview but I think the method name should be re-examined as it maps a file, the path just locates the file.  Naming is subjectives but in this case using "map" or "mapFile" would fit beside the allocateNative methods.
>> 
>> MappedMemorySegments. The force method specifies a write back guarantee but at the same time, the implNote in the class description suggests that the methods might be a no-op. You might want to adjust the wording to avoid any suggestion that force might be a no-op.
>> 
>> The javadoc for copyFrom isn't changed in this update but I notice it specifies IndexOutOfBoundException when the source segment is larger than the receiver, have other exceptions been examined?
>> 
>> I don't have any any comments on MemoryAccess except that it's not immediately clear why there are "Byte" methods that take a ByteOrder. Make sense for the multi-byte types of course.
>> 
>> The updates the java/nio sources look okay but it would be helpful if the really long lines could be chopped down as it's just too hard to do side-by-side reviews when the lines are so long. A minor nit but the changes X-Buffer.java.template mess up the alignment of the parameters to copyMemory/copySwapMemory methods.
>
>> The javadoc for copyFrom isn't changed in this update but I notice it specifies IndexOutOfBoundException when the source segment is larger than the receiver, have other exceptions been examined?
> 
> This exception is consistent with other uses of this exception throughout this API (e.g. when writing a segment out of bounds).

I assume the CSR needs to be updated so that it's in sync with the API changes in the latest round.

-------------

PR: https://git.openjdk.java.net/jdk/pull/548