Improve NIO channel support for buffer views over segments #512

ChrisHegarty · 2021-04-21T14:25:14Z

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

Further details can be found at: https://inside.java/2021/04/21/fma-and-nio-channels/

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed

Reviewers

Maurizio Cimadamore (@mcimadamore - Committer) ⚠️ Review applies to 297b653

Contributors

Patrick Concannon <[email protected]>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/512/head:pull/512
$ git checkout pull/512

Update a local copy of the PR:
$ git checkout pull/512
$ git pull https://git.openjdk.java.net/panama-foreign pull/512/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 512

View PR using the GUI difftool:
$ git pr show -t 512

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/512.diff

bridgekeeper · 2021-04-21T14:25:56Z

👋 Welcome back chegar! A progress list of the required criteria for merging this PR into foreign-jextract will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

ChrisHegarty · 2021-04-21T15:10:28Z

/contributor add pconcannon

openjdk · 2021-04-21T15:11:12Z

@ChrisHegarty
Contributor Patrick Concannon <[email protected]> successfully added.

src/java.base/share/classes/java/nio/Buffer.java

mcimadamore · 2021-04-21T15:06:38Z

src/java.base/share/classes/java/nio/Buffer.java

+                @Override
+                public Object acquireScope(Buffer targetBuffer, boolean async) {
+                    var targetScope = targetBuffer.scope();
+                    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

mcimadamore · 2021-04-21T15:08:37Z

src/java.base/share/classes/sun/nio/ch/IOUtil.java

+            releasers.run();
+    }
+
+    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.

mcimadamore · 2021-04-21T15:09:51Z

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java

@@ -382,7 +382,7 @@ default MemorySegment asSlice(MemoryAddress newBase) {
     * If this segment is associated with a shared scope, calling certain I/O operations on the resulting buffer might result in
     * an unspecified exception being thrown. Examples of such problematic operations are {@link FileChannel#read(ByteBuffer)},
     * {@link FileChannel#write(ByteBuffer)}, {@link java.nio.channels.SocketChannel#read(ByteBuffer)} and
-     * {@link java.nio.channels.SocketChannel#write(ByteBuffer)}.
+     * {@link java.nio.channels.SocketChannel#write(ByteBuffer)}.  TODO HERE Fix this comment


TODO comment here.

I removed this reminder TODO, and updated the spec to refer to async I/O operations with byte buffer views over segments associated with a confined scope.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java

mcimadamore · 2021-04-21T15:11:01Z

test/jdk/java/foreign/channels/AbstractChannelsTest.java

+/**
+ * Not a test, but infra for channel tests.
+ */
+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.

I still need to stabilise the tests on Windows, and expand out coverage for other async channel types, file, UDP, but that could be done in a follow-on PR.

mcimadamore · 2021-04-21T15:21:13Z

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).

mlbridge · 2021-04-21T15:37:15Z

Webrevs

…ndleImpl

mcimadamore · 2021-04-22T11:36:32Z

src/java.base/share/classes/java/nio/Buffer.java

+                @Override
+                public Scope.Handle acquireScope(Buffer buffer, boolean async) {
+                    var scope = buffer.scope();
+                    if (scope == null || scope.isImplicit()) {


do you still need the null handling (both here and in IOUtil) ? We just return a singleton now for implicit scope, so I think you can make the code more regular w/o losing any performance.

The null check is still needed - for "regular" buffers (that do not have a scope).

The isImplicit check can probably be removed. It is an unproven mirco-optimization to avoid the bloat of chaining unnecessary handles. It could be removed, and dropped altogether or pushed into the IOUtil code that does the chaining ( do not create a new Releaser for a handle that we've seen before, I assume handles can be compared by identity? ) Maybe I'll just drop this, and revisit later when benchmarking.

I see - I wonder if it would make sense to use global scope in these cases. But yes, this is probably best tackled in a separate patch.

mcimadamore · 2021-04-22T11:36:59Z

src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template

        void checkValidState();

        Thread ownerThread();

+        boolean isImplicit();


sorry for the extra shuffling - this will all go away when we exit incubation ;-)

mcimadamore · 2021-04-22T11:39:55Z

src/java.base/share/classes/java/nio/Buffer.java

+                    if (scope == null || scope.isImplicit()) {
+                        return null;
+                    }
+                    if (async && scope.ownerThread() != null) {


Crazy idea - are we sure we need this check? If user wants to really pass a confined buffer to an async IO, I belive a failure would still occur when the segment is accessed by a different thread - do you see reporting an error eagerly as something important? Will fibers change things so that e.g. we might be able to do more with a single thread?

I think that there is scope for tweaking this in the future. At the moment with the current structure, the Windows async socket channel implementation will access the memory behind the scope's/segment's back ( through the long address value that it retrieves ). It stores and reuses the "naked" address value from a thread other than that of the thread which initialed the I/O operation, all without invoking any memory access operations from Java. It's a non-trivial change to alter this, and not even clear if we should.

Ugh- ok, I see that different implementation might not necessarily trigger a scope check...

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java

mcimadamore

I'm not an expert of deep NIO socket code, but the proposed changes look sensible to me. Thus patch vastly expand usability of buffers obtained from memory segments, and it does so in a very minimalistic (but general) way.

mcimadamore · 2021-04-22T13:36:44Z

src/java.base/share/classes/java/nio/Buffer.java

+                @Override
+                public Scope.Handle acquireScope(Buffer buffer, boolean async) {
+                    var scope = buffer.scope();
+                    if (scope == null || scope.isImplicit()) {


I see - I wonder if it would make sense to use global scope in these cases. But yes, this is probably best tackled in a separate patch.

mcimadamore · 2021-04-22T13:37:30Z

src/java.base/share/classes/java/nio/Buffer.java

+                    if (scope == null || scope.isImplicit()) {
+                        return null;
+                    }
+                    if (async && scope.ownerThread() != null) {


Ugh- ok, I see that different implementation might not necessarily trigger a scope check...

openjdk · 2021-04-22T13:40:29Z

@ChrisHegarty This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

Improve NIO channel support for buffer views over segments

Co-authored-by: Patrick Concannon <[email protected]>
Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the foreign-memaccess+abi branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the foreign-memaccess+abi branch, type /integrate in a new comment.

mlbridge · 2021-04-22T14:35:14Z

Mailing list message from Pedro Lamarão on panama-dev:

Em qui., 22 de abr. de 2021 ?s 11:24, Chris Hegarty <chegar at openjdk.java.net>
escreveu:

On Thu, 22 Apr 2021 11:39:55 GMT, Maurizio Cimadamore <
mcimadamore at openjdk.org> wrote:

Crazy idea - are we sure we need this check? If user wants to really
pass a confined buffer to an async IO, I belive a failure would still occur
when the segment is accessed by a different thread - do you see reporting
an error eagerly as something important? Will fibers change things so that
e.g. we might be able to do more with a _single_ thread?

I think that there is scope for tweaking this in the future. At the moment
with the current structure, the Windows async socket channel implementation
will access the memory behind the scope's/segment's back ( through the
`long` address value that it retrieves ). It stores and reuses the "naked"
address value from a thread other than that of the thread which initialed
the I/O operation, all without invoking any memory access operations from
Java. It's a non-trivial change to alter this, and not even clear if we
should.

Consider also what the Windows API calls "registered I/O", also linux
"io_uring", which potentially causes this memory to be directly accessed
way down in the system software architecture. Atte. Pedro.

--
Pedro Lamar?o
https://www.prodist.com.br
Securing Critical Systems
Tel: +55 11 4380-6585

Antes de imprimir esta mensagem e seus anexos, certifique-se que seja
realmente necess?rio.
Proteger o meio ambiente ? nosso dever.
Before printing this e-mail or attachments, be sure it is necessary.
It is in our hands to protect the environment.

mlbridge · 2021-04-22T14:35:15Z

Mailing list message from Pedro Lamarão on panama-dev:

Em qui., 22 de abr. de 2021 ?s 11:24, Chris Hegarty <chegar at openjdk.java.net>
escreveu:

On Thu, 22 Apr 2021 11:39:55 GMT, Maurizio Cimadamore <
mcimadamore at openjdk.org> wrote:

Crazy idea - are we sure we need this check? If user wants to really
pass a confined buffer to an async IO, I belive a failure would still occur
when the segment is accessed by a different thread - do you see reporting
an error eagerly as something important? Will fibers change things so that
e.g. we might be able to do more with a _single_ thread?

I think that there is scope for tweaking this in the future. At the moment
with the current structure, the Windows async socket channel implementation
will access the memory behind the scope's/segment's back ( through the
`long` address value that it retrieves ). It stores and reuses the "naked"
address value from a thread other than that of the thread which initialed
the I/O operation, all without invoking any memory access operations from
Java. It's a non-trivial change to alter this, and not even clear if we
should.

Consider also what the Windows API calls "registered I/O", also linux
"io_uring", which potentially causes this memory to be directly accessed
way down in the system software architecture. Atte. Pedro.

--
Pedro Lamar?o
https://www.prodist.com.br
Securing Critical Systems
Tel: +55 11 4380-6585

Antes de imprimir esta mensagem e seus anexos, certifique-se que seja
realmente necess?rio.
Proteger o meio ambiente ? nosso dever.
Before printing this e-mail or attachments, be sure it is necessary.
It is in our hands to protect the environment.

ChrisHegarty · 2021-04-22T16:28:22Z

_Mailing list message from [Pedro Lamarão]...
Em qui., 22 de abr. de 2021 ?s 11:24, Chris Hegarty
....

I think that there is scope for tweaking this in the future. At the moment
with the current structure, the Windows async socket channel implementation
will access the memory behind the scope's/segment's back ( through the
long address value that it retrieves ). It stores and reuses the "naked"
address value from a thread other than that of the thread which initialed
the I/O operation, all without invoking any memory access operations from
Java. It's a non-trivial change to alter this, and not even clear if we
should.

Consider also what the Windows API calls "registered I/O", also linux
"io_uring", which potentially causes this memory to be directly accessed
way down in the system software architecture. Atte. Pedro.

Correct, it's a detail of the implementation how the memory is ultimately accessed, and when (and for how long) the resource scope handle is acquired. So may vary for different implementations. The Windows asynchronous socket channels in the JDK use Overlapped I/O and registers to receive a notification via an I/O completion port.

ChrisHegarty · 2021-04-22T20:00:02Z

/integrate

openjdk · 2021-04-22T20:01:45Z

@ChrisHegarty Since your change was applied there has been 1 commit pushed to the foreign-memaccess+abi branch:

c2812d9: Misc javadoc fixes

Your commit was automatically rebased without conflicts.

Pushed as commit 8c53fef.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Prototype 2 Initial load

70ca014

Remove unnecessary abstract override

42de52a

mcimadamore reviewed Apr 21, 2021

View reviewed changes

remove explicit checkValidState check

60d491c

ChrisHegarty marked this pull request as ready for review April 21, 2021 15:31

openjdk bot added the rfr Ready for review label Apr 21, 2021

ChrisHegarty added 4 commits April 22, 2021 09:04

Merge branch 'foreign-memaccess+abi' into buffers+resourceScope_Lock

a56707b

Fix Windows issues

7a8c389

Apply new acquire/release semantics, and remove AutoCloseable from Ha…

a88ad97

…ndleImpl

Resolve outstanding TODO in MemoryScope spec

d8d29d3

mcimadamore reviewed Apr 22, 2021

View reviewed changes

ChrisHegarty changed the base branch from foreign-jextract to foreign-memaccess+abi April 22, 2021 13:05

mcimadamore reviewed Apr 22, 2021

View reviewed changes

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java Show resolved Hide resolved

mcimadamore approved these changes Apr 22, 2021

View reviewed changes

openjdk bot added the ready Ready to be integrated label Apr 22, 2021

remove implicit check

297b653

mcimadamore approved these changes Apr 22, 2021

View reviewed changes

Additional test fix for Windows

3c08280

openjdk bot closed this Apr 22, 2021

openjdk bot added the integrated Pull request has been integrated label Apr 22, 2021

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Apr 22, 2021

chrisvest mentioned this pull request May 6, 2021

Revisit hacks in MemorySegment/ByteBuffer integration netty/netty-incubator-buffer-api#58

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve NIO channel support for buffer views over segments #512

Improve NIO channel support for buffer views over segments #512

ChrisHegarty commented Apr 21, 2021 •

edited

Loading

bridgekeeper bot commented Apr 21, 2021

ChrisHegarty commented Apr 21, 2021

openjdk bot commented Apr 21, 2021

mcimadamore Apr 21, 2021

mcimadamore Apr 21, 2021

mcimadamore Apr 21, 2021

ChrisHegarty Apr 22, 2021

mcimadamore Apr 21, 2021

ChrisHegarty Apr 22, 2021

mcimadamore commented Apr 21, 2021 •

edited

Loading

mlbridge bot commented Apr 21, 2021 •

edited

Loading

mcimadamore Apr 22, 2021

ChrisHegarty Apr 22, 2021

mcimadamore Apr 22, 2021

mcimadamore Apr 22, 2021

mcimadamore Apr 22, 2021

ChrisHegarty Apr 22, 2021

mcimadamore Apr 22, 2021

mcimadamore left a comment

mcimadamore Apr 22, 2021

mcimadamore Apr 22, 2021

openjdk bot commented Apr 22, 2021 •

edited

Loading

mlbridge bot commented Apr 22, 2021

mlbridge bot commented Apr 22, 2021

ChrisHegarty commented Apr 22, 2021

ChrisHegarty commented Apr 22, 2021

openjdk bot commented Apr 22, 2021

Improve NIO channel support for buffer views over segments #512

Improve NIO channel support for buffer views over segments #512

Conversation

ChrisHegarty commented Apr 21, 2021 • edited Loading

Progress

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Apr 21, 2021

ChrisHegarty commented Apr 21, 2021

openjdk bot commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore commented Apr 21, 2021 • edited Loading

mlbridge bot commented Apr 21, 2021 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Apr 22, 2021 • edited Loading

mlbridge bot commented Apr 22, 2021

mlbridge bot commented Apr 22, 2021

ChrisHegarty commented Apr 22, 2021

ChrisHegarty commented Apr 22, 2021

openjdk bot commented Apr 22, 2021

ChrisHegarty commented Apr 21, 2021 •

edited

Loading

mcimadamore commented Apr 21, 2021 •

edited

Loading

mlbridge bot commented Apr 21, 2021 •

edited

Loading

openjdk bot commented Apr 22, 2021 •

edited

Loading