On Wed, 28 Jun 2023 18:53:12 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 incrementally with one additional
> commit since the last revision:
>
> less exception filtering when fetching socket read timeout
Overall, it's good to move away from the "faraway instrumentation". Looks
forward to the next steps to get to a more complete solution.
src/java.base/share/classes/java/net/Socket.java line 44:
> 42: import java.util.Collections;
> 43:
> 44:
Minor nit, I assume this additional blank line is left over from a previous
iteration.
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 465:
> 463: public long read(ByteBuffer[] dsts, int offset, int length)
> 464: throws IOException
> 465: {
The supporting methods for read (beginRead, endRead, throwConnectionReset, ...)
are declared before the read methods. It's probably best to put the implRead
before the read too so that all the supporting methods are together. Same thing
with the write methods.
-------------
Marked as reviewed by alanb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14342#pullrequestreview-1588627786
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1301134829
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1301144638
