On Tue, 19 Sep 2023 15:35:11 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 > > Tim Prinzing has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 12 commits: > > - Merge branch 'master' into JDK-8308995 > - More changes from review: > > I didn't like the name of the helper method 'checkForCommit' because it > doesn't indicate that it might commit the event. I also don't like > 'commitEvent' because it might not. Since JFR events are sort of like a > queue I went with a name from collections and called it 'offer' so using > it is something like 'SocketReadEvent.offer(...)' which seems like it > gets the idea across better. Also improved the javadoc for it. > > Removed the comments about being instrumented by JFR in > Socket.SocketInputStream and Socket.SocketOutputStream. > > I went ahead and moved the event commiting out of the finally block so > that we don't emit events when the read/write did not actually happen. > The bugid JDK-8310979 will be used to determine if more should be done > in this area. > > The implRead and implWrite were moved up with the other support methods > for read/write. > - less exception filtering when fetching socket read timeout > - remove unused SOCKET_READ and SOCKET_WRITE configurations. > - Merge branch 'master' into JDK-8308995 > > # Conflicts: > # src/jdk.jfr/share/classes/jdk/jfr/events/EventConfigurations.java > - Avoid exceptions getting address/timeout for jfr event. Remove unused > EventConiguration fields SOCKET_READ and SOCKET_WRITE. Remove spurious > whitespace. > - some changes from review. > > read0() to implRead() > write0() to implWrite() > trailing whitespace > - fix copyright date > - Added micro benchmark to measure socket event overhead. > - Some changes from review. > > Append a 0 to method names being wrapped. Use getHostString to avoid > a reverse lookup when fetching the hostname of the remote address. > - ... and 2 more: https://git.openjdk.org/jdk/compare/607bd4ed...6db6fab4 The new implementation calls getSocketRemoteAddress() and getSOTimeout() regardless if the event duration exceeds the threshold or not. This adds unnecessary overhead. Most socket write/reads are not committed, only those that take more than 20 ms (by default). I think you need something like this: if (SocketRead.shouldCommit(...)) { ... } ------------- PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1731379349
