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 1101:
> 1099: @Override
> 1100: public int read(byte[] b, int off, int len) throws IOException {
> 1101: if (! SocketReadEvent.enabled()) {
Drop the space in "! SocketReadEvent" as it is inconsistent with the existing
code.
src/java.base/share/classes/jdk/internal/event/SocketReadEvent.java line 119:
> 117: public static void checkForCommit(long start, long nbytes,
> SocketAddress remote, long timeout) {
> 118: long duration = timestamp() - start;
> 119: if (shouldCommit(duration)) {
I think you know this, but this will need to be re-examined for SocketChannel
configured non-blocking.
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 408:
> 406: @Override
> 407: public int read(ByteBuffer buf) throws IOException {
> 408: if (!SocketReadEvent.enabled()) {
The read/write with sun.nio.ch.SocketInputStream and SocketOutputStream does
not go through SC.read/write so I think SocketAdaptor read/write will need
attention, maybe a future PR as there are other code paths that aren't covered
in this PR.
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 416:
> 414: nbytes = implRead(buf);
> 415: } finally {
> 416: SocketReadEvent.checkForCommit(start, nbytes,
> getRemoteAddress(), 0);
This will need to be changed to use remoteAddress(), can't use
getRemoteAddress() as it might throw.
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 466:
> 464: throws IOException
> 465: {
> 466: if (! SocketReadEvent.enabled()) {
Spurious space here too, several other places.
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 474:
> 472: nbytes = implRead(dsts, offset, length);
> 473: } finally {
> 474: SocketReadEvent.checkForCommit(start, nbytes,
> getRemoteAddress(), 0);
This has to be remoteAddress() too.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238312001
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238317470
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238323035
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238318572
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238318875
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238319120
