On Thu, 29 Oct 2020 14:13:40 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
>>> @mcimadamore, if you pull from current master, you would get the Linux >>> x86_32 tier1 run "for free". >> >> Just did that - I also removed TestMismatch from the problem list in the >> latest iteration, and fixed the alignment for long/double layouts, after >> chatting with the team (https://bugs.openjdk.java.net/browse/JDK-8255350) > > I've just uploaded another iteration which addresses some comments from > @AlanBateman. Basically, there are some operations on Channel and Socket > which take ByteBuffer as arguments, and then, if such buffers are *direct*, > they get the address and pass it down to some native function. This idiom is > problematic because there's no way to guarantee that the buffer won't be > closed (if obtained from a memory segment) after the address has been > obtained. As a stop gap solution, I've introduced checks in > `DirectBuffer::address` method, which is used in around 30 places in the JDK. > This method will now throw if (a) the buffer has a shared scope, or (b) if > the scope is confined, but already closed. With this extra check, I believe > there's no way to misuse the buffer obtained from a segment. We have > discussed plans to remove this limitations (which we think will be possible) > - but for the time being, it's better to play the conservative card. 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/548
