RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]

[Erik Gahlin]

On Tue, 19 Sep 2023 15:35:11 GMT, Tim Prinzing <tprinzing at 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