On Tue, 27 Jun 2023 21:52:08 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 ten commits:
>
> - 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.
> - remove unnecessary cast
> - 8308995: Update Network IO JFR events to be static mirror events
src/java.base/share/classes/java/net/Socket.java line 1133:
> 1131: return parent.getSoTimeout();
> 1132: } catch (Throwable t) {
> 1133: // ignored - avoiding exceptions in jfr event data
> gathering
This should be SocketException, not Throwable. That said, I think it would be
useful to know why the SocketReadEvent includes the timeout. Is this used to
see If read durations are close to the timeout? I assume once this code is
fixed to deal with the exceptional case that the need to include the timeout
for the success case will mostly go away.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1244728684
