On Mon, 2 Nov 2020 11:26:51 GMT, Maurizio Cimadamore <mcimadam...@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
