[foreign-jextract] RFR: Improve NIO channel support for buffer views over segments

[Maurizio Cimadamore]

On Wed, 21 Apr 2021 14:25:14 GMT, Chris Hegarty <chegar at openjdk.org> wrote:

> Initial prototype changes to use resource scope handles when performing I/O operations with synchronous and asynchronous channels.

A "lesson learned" in this PR seems to be that the AutoCloseable feature of ResourceScope.Handle is not as "hot" as it seemed. In fact, we don't seem to rely on that much (if at all). In part, I think, to avoid warnings; in part because the code just doesn't lend to it (e.g. handles are stored in fields).

src/java.base/share/classes/java/nio/Buffer.java line 827:

> 825:                 public Object acquireScope(Buffer targetBuffer, boolean async) {
> 826:                     var targetScope = targetBuffer.scope();
> 827:                     if (targetScope == null || targetScope.isImplicit()) {

Hopefully some of this dance will not be necessary if we move to the proposed acquire/release API.

src/java.base/share/classes/java/nio/Buffer.java line 831:

> 829:                     }
> 830:                     if (async && targetScope.ownerThread() != null) {
> 831:                         throw new IllegalStateException("Confined scope not supported");

Good catch - a confined scope has no chance to work with async IO :-)

src/java.base/share/classes/java/nio/Buffer.java line 835:

> 833:                     try {
> 834:                         targetScope.checkValidState();
> 835:                     } catch (ScopedMemoryAccess.Scope.ScopedAccessError e) {   // ####: recheck thrown exs

Surprised to see this - your code should not be doing this - e.g. a failure in acquire() should trigger an ISE. Is the problem that you got the low level singleton exception (in which case that seems a bug in acquire()).

src/java.base/share/classes/sun/nio/ch/IOUtil.java line 507:

> 505:     }
> 506: 
> 507:     static record LinkedRunnable(Runnable node, Runnable next)

Nice - an alternative is to keep wrapping runnables, which is a trick we sometimes do. Of course you can risk running out of stack with that approach, which doesn't apply here.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 385:

> 383:      * an unspecified exception being thrown. Examples of such problematic operations are {@link FileChannel#read(ByteBuffer)},
> 384:      * {@link FileChannel#write(ByteBuffer)}, {@link java.nio.channels.SocketChannel#read(ByteBuffer)} and
> 385:      * {@link java.nio.channels.SocketChannel#write(ByteBuffer)}.  TODO HERE Fix this comment

TODO comment here.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java line 56:

> 54: 
> 55:     @Override
> 56:     public abstract Handle acquire();

Is this really required?

test/jdk/java/foreign/channels/AbstractChannelsTest.java line 42:

> 40:  * Not a test, but infra for channel tests.
> 41:  */
> 42: public class AbstractChannelsTest {

Many thanks for adding this (and other) tests. It's nice to see that now all combination of IO and scopes are tested - this should give us some confidence when fine tuning the impl.

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

PR: https://git.openjdk.java.net/panama-foreign/pull/512