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 The new code seems to accurately correspond to what the various `*Instrumentor` classes were doing, so that is good. I agree with Alan that potential exception that may arise when generating the event are an issue (e.g. call to getRemoteAddress() or other getter that may make a check on the socket state) - though that was already present in the original code. Maybe that could be fixed here though. In cases where the implRead/implWrite call throws an exception, shouldn't the event contain that exception, or at least exception message? If it doesn't should it be emitted at all, or should another event be emitted instead? ------------- PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1602505386
