On Tue, 6 Jun 2023 19:39:31 GMT, Tim Prinzing <tprinz...@openjdk.org> wrote:
> The socket read/write JFR events currently use instrumentation of java.base
> code using templates in the jdk.jfr modules. This results in some java.base
> code residing in the jdk.jfr module which is undesirable.
>
> JDK19 added static support for event classes. The old instrumentor classes
> should be replaced with mirror events using the static support.
>
> In the java.base module:
> Added two new events, jdk.internal.event.SocketReadEvent and
> jdk.internal.event.SocketWriteEvent.
> java.net.Socket and sun.nio.ch.SocketChannelImpl were changed to make use of
> the new events.
>
> In the jdk.jfr module:
> jdk.jfr.events.SocketReadEvent and jdk.jfr.events.SocketWriteEvent were
> changed to be mirror events.
> In the package jdk.jfr.internal.instrument, the classes
> SocketChannelImplInstrumentor, SocketInputStreamInstrumentor, and
> SocketOutputStreamInstrumentor were removed. The JDKEvents class was updated
> to reflect all of those changes.
>
> The existing tests in test/jdk/jdk/jfr/event/io continue to pass with the new
> implementation:
> Passed: jdk/jfr/event/io/TestSocketChannelEvents.java
> Passed: jdk/jfr/event/io/TestSocketEvents.java
>
> I added a micro benchmark which measures the overhead of handling the jfr
> socket events.
> test/micro/org/openjdk/bench/java/net/SocketEventOverhead.java.
> It needs access the jdk.internal.event package, which is done at runtime with
> annotations that add the extra arguments.
> At compile time the build arguments had to be augmented in
> make/test/BuildMicrobenchmark.gmk
src/java.base/share/classes/java/net/Socket.java line 43:
> 41: import java.util.Collections;
> 42:
> 43: import jdk.internal.event.SocketWriteEvent;
You'll probably want to clean up the imports to avoid having import of
jdk.internal classes in two places.
src/java.base/share/classes/java/net/Socket.java line 1109:
> 1107: nbytes = read0(b, off, len);
> 1108: } finally {
> 1109: SocketReadEvent.checkForCommit(start, nbytes,
> parent.getRemoteSocketAddress(), parent.getSoTimeout());
So if read throws, this will commit a jdk.SocketReadEvent with size 0, maybe
this will change later to include the exception?
src/java.base/share/classes/java/net/Socket.java line 1114:
> 1112: }
> 1113:
> 1114: private int read0(byte[] b, int off, int len) throws
> IOException {
Can you rename this to implRead?
src/java.base/share/classes/java/net/Socket.java line 1215:
> 1213: return;
> 1214: }
> 1215: int bytesWritten = 0;
Is bytesWritten needed?
src/java.base/share/classes/java/net/Socket.java line 1226:
> 1224: }
> 1225:
> 1226: public void write0(byte[] b, int off, int len) throws
> IOException {
Can you change this to be private and rename to implWrite?
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 421:
> 419: }
> 420:
> 421: private int read0(ByteBuffer buf) throws IOException {
Can you rename this to implRead too?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1236731407
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1237226982
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1236719052
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1236719603
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1236719373
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1237227861
