Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
On Tue, 19 Sep 2023 15:35:11 GMT, Tim Prinzing 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
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
On Tue, 19 Sep 2023 20:51:41 GMT, Tim Prinzing wrote: > The existing JFR tests TestSocketChannelEvents and TestSocketEvents in > jdk.jfr.event.io verify the events are still emitted as expected. Thanks Tim. Should 8308995 be listed in the `@bug` clause of these two tests? - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1727528861
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
On Wed, 20 Sep 2023 11:21:51 GMT, Daniel Fuchs wrote: > Thanks Tim. Should 8308995 be listed in the `@bug` clause of these two tests? I don't think so as these tests are just used to check that changes haven't broken anything. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1727532006
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
On Tue, 19 Sep 2023 15:35:11 GMT, Tim Prinzing 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 I sync'd master and updated the bug report with the successful test results. Created [JDK-8316558] to track potential timeout field removal. The existing JFR tests TestSocketChannelEvents and TestSocketEvents in jdk.jfr.event.io verify the events are still emitted as expected. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1726449349
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
> 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 - Changes: https://git.openjdk.org/jdk/pull/14342/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=05 Stats: 906 lines in 13 files changed: 519 ins; 379 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/14342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14342/head:pull/14342 PR: https://git.openjdk.org/jdk/pull/14342
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v3]
On Thu, 7 Sep 2023 21:54:44 GMT, Tim Prinzing wrote: > No. I think it's a relic from the distant past though. I think the timeout > field should be removed. It's not used on SocketChannel at all, and it > doesn't seem useful on Socket. Should we log an RFE to that effect? - PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1330294609
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v5]
On Thu, 7 Sep 2023 21:50:17 GMT, Tim Prinzing 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: > > 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. LGTM. Are there existing that will help verify that the read events and write events are still emitted as expected? If not shouldn't we write some? - PR Review: https://git.openjdk.org/jdk/pull/14342#pullrequestreview-1633551891
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 6 Sep 2023 15:55:21 GMT, Tim Prinzing wrote: >>> https://bugs.openjdk.org/browse/JDK-8310979 - better exception handling >>> https://bugs.openjdk.org/browse/JDK-8310978 - missing code paths for event >>> generation https://bugs.openjdk.org/browse/JDK-8310994 - non-blocking, >>> event for select ops >> >> Do you have a sense yet on events when the channel is configured >> non-blocking? I realise the threshold is 20ms so they are probably not >> recorded today but I wonder if these code paths will eventually include `|| >> !blocking` or if it useful to record data on all socket read/write ops. > >> > https://bugs.openjdk.org/browse/JDK-8310979 - better exception handling >> > https://bugs.openjdk.org/browse/JDK-8310978 - missing code paths for event >> > generation https://bugs.openjdk.org/browse/JDK-8310994 - non-blocking, >> > event for select ops >> >> Do you have a sense yet on events when the channel is configured >> non-blocking? I realise the threshold is 20ms so they are probably not >> recorded today but I wonder if these code paths will eventually include `|| >> !blocking` or if it useful to record data on all socket read/write ops. > > I think it's useful if trying to trace the calls (i.e. set to 0ms). > Apparently the security manager was being used for that by some. > @tprinzing > Your change (at version > [9fa2745](https://github.com/openjdk/jdk/commit/9fa2745960aea0bc45642081bac89fb5ef65809e)) > is now ready to be sponsored by a Committer. @tprinzing I don't mind sponsoring but I think it would help if you could sync up the branch and provide a summary on the testing was done. The jdk_net and jdk_nio test groups are in tier2. The jdk_jfr test group is in tier3. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1725248917
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v3]
On Tue, 22 Aug 2023 07:18:21 GMT, Alan Bateman wrote:
>> 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.
>
> Were you able to find out what the timeout is used for?
No. I think it's a relic from the distant past though. I think the timeout
field should be removed. It's not used on SocketChannel at all, and it doesn't
seem useful on Socket.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1319152153
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v5]
> 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: 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. - Changes: - all: https://git.openjdk.org/jdk/pull/14342/files - new: https://git.openjdk.org/jdk/pull/14342/files/d6f7df72..9fa27459 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=03-04 Stats: 151 lines in 5 files changed: 57 ins; 81 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/14342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14342/head:pull/14342 PR: https://git.openjdk.org/jdk/pull/14342
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 6 Sep 2023 15:55:21 GMT, Tim Prinzing wrote: > I think it's useful if trying to trace the calls (i.e. set to 0ms). > Apparently the security manager was being used for that by some. The SM isn't called once connected so I don't think anyone could have every done that. Yes, you could set the threshold to 0ms to emit event for every read/write but I wonder how useful that would be, maybe I/O stats would be more interesting. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1709568576
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Tue, 22 Aug 2023 07:31:36 GMT, Alan Bateman wrote: > > https://bugs.openjdk.org/browse/JDK-8310979 - better exception handling > > https://bugs.openjdk.org/browse/JDK-8310978 - missing code paths for event > > generation https://bugs.openjdk.org/browse/JDK-8310994 - non-blocking, > > event for select ops > > Do you have a sense yet on events when the channel is configured > non-blocking? I realise the threshold is 20ms so they are probably not > recorded today but I wonder if these code paths will eventually include `|| > !blocking` or if it useful to record data on all socket read/write ops. I think it's useful if trying to trace the calls (i.e. set to 0ms). Apparently the security manager was being used for that by some. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1708657617
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 28 Jun 2023 18:53:12 GMT, Tim Prinzing 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 One other thing is that the comments in Socket.SocketInputStream and Socket.SocketOutputStream where it has "This class is instrumented by Java Flight Recorder (JFR) to get socket I/O events" can be removed now. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1687765868
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 28 Jun 2023 18:53:12 GMT, Tim Prinzing 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 The stack trace will start in the incorrect method, i.e checkForCommit, when a utility method is used. This is probably something we will have to fix anyway. I filed an issue for that: https://bugs.openjdk.org/browse/JDK-8314745 - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1687749717
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Tue, 27 Jun 2023 18:29:45 GMT, Tim Prinzing wrote:
>> src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 408:
>>
>>> 406: @Override
>>> 407: public int read(ByteBuffer buf) throws IOException {
>>> 408: if (!SocketReadEvent.enabled()) {
>>
>> The read/write with sun.nio.ch.SocketInputStream and SocketOutputStream does
>> not go through SC.read/write so I think SocketAdaptor read/write will need
>> attention, maybe a future PR as there are other code paths that aren't
>> covered in this PR.
>
> I've created https://bugs.openjdk.org/browse/JDK-8310978 to drive the future
> PR to support the missing code paths
Thanks, it's a reminder that the existing SocketXXX events are incomplete
and/or not much I/O done with the socket adaptors.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1301143720
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v3]
On Wed, 28 Jun 2023 06:09:14 GMT, Alan Bateman wrote:
>> 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.
Were you able to find out what the timeout is used for?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1301136108
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 28 Jun 2023 18:53:12 GMT, Tim Prinzing 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
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Thu, 22 Jun 2023 13:39:51 GMT, Erik Gahlin wrote: > An exception event will be emitted. The event is disabled by default, but > there is ongoing work on a throttling mechanism, so it can be always-on. Good, I think the exception cases are probably the most interesting for this area when it comes to troubleshooting. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1687636801
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 2 Aug 2023 20:09:39 GMT, Alan Bateman wrote: > https://bugs.openjdk.org/browse/JDK-8310979 - better exception handling > https://bugs.openjdk.org/browse/JDK-8310978 - missing code paths for event > generation https://bugs.openjdk.org/browse/JDK-8310994 - non-blocking, event > for select ops Do you have a sense yet on events when the channel is configured non-blocking? I realise the threshold is 20ms so they are probably not recorded today but I wonder if these code paths will eventually include `|| !blocking` or if it useful to record data on all socket read/write ops. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1687630491
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 28 Jun 2023 18:53:12 GMT, Tim Prinzing 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 Looks good as far as I can tell. I would perhaps name the checkForCommit(...) method commitEvent(...) to make it more clear that it writes the event, but it's not a big thing. - Marked as reviewed by egahlin (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14342#pullrequestreview-1559847776
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 2 Aug 2023 18:19:27 GMT, Tim Prinzing wrote: > I believe this has all requested changes or has separate bug reports to > address changes yet needing to be made. > https://bugs.openjdk.org/browse/JDK-8310979 - better exception handling > https://bugs.openjdk.org/browse/JDK-8310978 - missing code paths for event > generation https://bugs.openjdk.org/browse/JDK-8310994 - non-blocking, event > for select ops > > This request has been patiently waiting for approval for a long time! I plan to come back to this soon, been busy with other things. It's good to have JBS issues created for the next steps. I assume you can start to prepare the changes for the next steps so that all the pieces can go into the same release. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1662903292
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 28 Jun 2023 18:53:12 GMT, Tim Prinzing 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 I believe this has all requested changes or has separate bug reports to address changes yet needing to be made. https://bugs.openjdk.org/browse/JDK-8310979 - better exception handling https://bugs.openjdk.org/browse/JDK-8310978 - missing code paths for event generation https://bugs.openjdk.org/browse/JDK-8310994 - non-blocking, event for select ops This request has been patiently waiting for approval for a long time! - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1662726848
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/14342/files - new: https://git.openjdk.org/jdk/pull/14342/files/27a766c7..d6f7df72 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14342/head:pull/14342 PR: https://git.openjdk.org/jdk/pull/14342
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v3]
On Tue, 27 Jun 2023 21:52:08 GMT, Tim Prinzing 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
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v3]
> 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 - Changes: https://git.openjdk.org/jdk/pull/14342/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=02 Stats: 908 lines in 13 files changed: 533 ins; 369 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14342/head:pull/14342 PR: https://git.openjdk.org/jdk/pull/14342
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v2]
> 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: Avoid exceptions getting address/timeout for jfr event. Remove unused EventConiguration fields SOCKET_READ and SOCKET_WRITE. Remove spurious whitespace. - Changes: - all: https://git.openjdk.org/jdk/pull/14342/files - new: https://git.openjdk.org/jdk/pull/14342/files/5faeb300..518adf8e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=00-01 Stats: 21 lines in 3 files changed: 10 ins; 2 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/14342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14342/head:pull/14342 PR: https://git.openjdk.org/jdk/pull/14342
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Thu, 22 Jun 2023 13:39:51 GMT, Erik Gahlin wrote: > 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? Added issue https://bugs.openjdk.org/browse/JDK-8310979 to add a field to SocketReadEvent and SocketWriteEvent in a separate PR - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1610037645
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Thu, 22 Jun 2023 10:21:46 GMT, Alan Bateman 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
>
> src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 408:
>
>> 406: @Override
>> 407: public int read(ByteBuffer buf) throws IOException {
>> 408: if (!SocketReadEvent.enabled()) {
>
> The read/write with sun.nio.ch.SocketInputStream and SocketOutputStream does
> not go through SC.read/write so I think SocketAdaptor read/write will need
> attention, maybe a future PR as there are other code paths that aren't
> covered in this PR.
I've created https://bugs.openjdk.org/browse/JDK-8310978 to drive the future PR
to support the missing code paths
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1244182424
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Thu, 22 Jun 2023 11:53:59 GMT, Daniel Fuchs wrote: > 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? An exception event will be emitted. The event is disabled by default, but there is ongoing work on a throttling mechanism, so it can be always-on. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1602659051
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Tue, 6 Jun 2023 19:39:31 GMT, Tim Prinzing 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
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Wed, 21 Jun 2023 15:48:30 GMT, Alan Bateman 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
>
> src/java.base/share/classes/java/net/Socket.java line 1109:
>
>> 1107: nbytes = read0(b, off, len);
>> 1108: } finally {
>> 1109: SocketReadEvent.checkForCommit(start, nbytes,
>> parent.getRemoteSocketAddress(), parent.getSoTimeout());
>
> So if read throws, this will commit a jdk.SocketReadEvent with size 0, maybe
> this will change later to include the exception?
The other issue to think about here is where the Socket is asynchronously
closed. In t hat case, implRead will throw but we'll end up with a confusing
suppressed exception due to the call to getSoTimeout. I think this will have to
be replaced with a call to a helper method that returns the timeout or 0.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238315000
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Tue, 6 Jun 2023 19:39:31 GMT, Tim Prinzing 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
src/java.base/share/classes/java/net/Socket.java line 1101:
> 1099: @Override
> 1100: public int read(byte[] b, int off, int len) throws IOException {
> 1101: if (! SocketReadEvent.enabled()) {
Drop the space in "! SocketReadEvent" as it is inconsistent with the existing
code.
src/java.base/share/classes/jdk/internal/event/SocketReadEvent.java line 119:
> 117: public static void checkForCommit(long start, long nbytes,
> SocketAddress remote, long timeout) {
> 118: long duration = timestamp() - start;
> 119: if (shouldCommit(duration)) {
I think you know this, but this will need to be re-examined for SocketChannel
configured non-blocking.
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 408:
> 406: @Override
> 407: public int read(ByteBuffer buf) throws IOException {
> 408: if (!SocketReadEvent.enabled()) {
The read/write with sun.nio.ch.SocketInputStream and SocketOutputStream does
not go through SC.read/write so I think SocketAdaptor read/write will need
attention, maybe a future PR as there are other code paths that aren't covered
in this PR.
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 416:
> 414: nbytes = implRead(buf);
> 415: } finally {
> 416: SocketReadEvent.checkForCommit(start, nbytes,
> getRemoteAddress(), 0);
This will need to be changed to use remoteAddress(), can't use
getRemoteAddress() as it might throw.
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 466:
> 464: throws IOException
> 465: {
> 466: if (! SocketReadEvent.enabled()) {
Spurious space here too, several other places.
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 474:
> 472: nbytes = implRead(dsts, offset, length);
> 473: } finally {
> 474: SocketReadEvent.checkForCommit(start, nbytes,
> getRemoteAddress(), 0);
This has to be remoteAddress() too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238312001
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238317470
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238323035
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238318572
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238318875
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1238319120
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Tue, 6 Jun 2023 19:39:31 GMT, Tim Prinzing 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 I believe the fields SOCKET_READ and SOCKET_WRITE in the jdk.jfr.events.EventConfigurations class can be removed. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1602173783
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Wed, 21 Jun 2023 16:59:22 GMT, Stuart Marks wrote: > Are we using a convention of `implRead` or `readImpl`? Either is ok with me, > but I think we had been using `readImpl` and similar elsewhere. This code is already using implXXX so it's just be consistent. - PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1237319486
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Wed, 21 Jun 2023 09:46:35 GMT, Alan Bateman 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
>
> src/java.base/share/classes/java/net/Socket.java line 1114:
>
>> 1112: }
>> 1113:
>> 1114: private int read0(byte[] b, int off, int len) throws
>> IOException {
>
> Can you rename this to implRead?
Are we using a convention of `implRead` or `readImpl`? Either is ok with me,
but I think we had been using `readImpl` and similar elsewhere.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1237316630
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events
On Tue, 6 Jun 2023 19:39:31 GMT, Tim Prinzing 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
src/java.base/share/classes/java/net/Socket.java line 43:
> 41: import java.util.Collections;
> 42:
> 43: import jdk.internal.event.SocketWriteEvent;
You'll probably want to clean up the imports to avoid having import of
jdk.internal classes in two places.
src/java.base/share/classes/java/net/Socket.java line 1109:
> 1107: nbytes = read0(b, off, len);
> 1108: } finally {
> 1109: SocketReadEvent.checkForCommit(start, nbytes,
> parent.getRemoteSocketAddress(), parent.getSoTimeout());
So if read throws, this will commit a jdk.SocketReadEvent with size 0, maybe
this will change later to include the exception?
src/java.base/share/classes/java/net/Socket.java line 1114:
> 1112: }
> 1113:
> 1114: private int read0(byte[] b, int off, int len) throws
> IOException {
Can you rename this to implRead?
src/java.base/share/classes/java/net/Socket.java line 1215:
> 1213: return;
> 1214: }
> 1215: int bytesWritten = 0;
Is bytesWritten needed?
src/java.base/share/classes/java/net/Socket.java line 1226:
> 1224: }
> 1225:
> 1226: public void write0(byte[] b, int off, int len) throws
> IOException {
Can you change this to be private and rename to implWrite?
src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 421:
> 419: }
> 420:
> 421: private int read0(ByteBuffer buf) throws IOException {
Can you rename this to implRead too?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1236731407
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1237226982
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1236719052
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1236719603
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1236719373
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1237227861
