RFR: 8308995: Update Network IO JFR events to be static mirror events
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 - Commit messages: - some changes from review. - fix copyright date - Added micro benchmark to measure socket event overhead. - Some changes from review. - 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=00 Issue: https://bugs.openjdk.org/browse/JDK-8308995 Stats: 896 lines in 12 files changed: 523 ins; 367 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
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 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 [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 [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 [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 [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]
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 [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 [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 [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 [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
Integrated: 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 This pull request has now been integrated. Changeset: b275bdd9 Author:Tim Prinzing Committer: Alan Bateman URL: https://git.openjdk.org/jdk/commit/b275bdd9b55b567cfe60c389d5ef8b70615928f4 Stats: 906 lines in 13 files changed: 519 ins; 379 del; 8 mod 8308995: Update Network IO JFR events to be static mirror events Reviewed-by: egahlin, alanb - PR: https://git.openjdk.org/jdk/pull/14342
RFR: 8319571: Update jni/nullCaller/NullCallerTest.java to accept flags or mark as flagless
Marked as flagless. - Commit messages: - 8319571: Update jni/nullCaller/NullCallerTest.java to accept flags or mark as flagless Changes: https://git.openjdk.org/jdk/pull/16711/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16711&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8319571 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16711.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16711/head:pull/16711 PR: https://git.openjdk.org/jdk/pull/16711
Re: RFR: 8319571: Update jni/nullCaller/NullCallerTest.java to accept flags or mark as flagless [v2]
> Marked as flagless. Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - use ProcessTools helper class to run test. - Merge branch 'master' into JDK-8319571 - Merge branch 'master' into JDK-8319571 - 8319571: Update jni/nullCaller/NullCallerTest.java to accept flags or mark as flagless Marked as flagless. - Changes: - all: https://git.openjdk.org/jdk/pull/16711/files - new: https://git.openjdk.org/jdk/pull/16711/files/bbcbb810..b947469b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16711&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16711&range=00-01 Stats: 12689 lines in 337 files changed: 6298 ins; 3150 del; 3241 mod Patch: https://git.openjdk.org/jdk/pull/16711.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16711/head:pull/16711 PR: https://git.openjdk.org/jdk/pull/16711
RFR: 8310994: Add JFR event for selection operations
Added mirror event with static methods: jdk.internal.event.SelectionEvent that provides the duration of select calls and the count of how many keys are available. Emit the event from SelectorImpl::lockAndDoSelect Test at jdk.jfr.event.io.TestSelectionEvents - Commit messages: - remove trailing whitespace - event logic outside of the lock, selector in try block - remove unused import - fix TestConfigure failure - add event defaults - Merge branch 'master' into JDK-8310994 - minor test cleanup - Merge branch 'master' into JDK-8310994 - 8310994: Add JFR event for selection operations Changes: https://git.openjdk.org/jdk/pull/16710/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310994 Stats: 265 lines in 9 files changed: 264 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
Re: RFR: 8310994: Add JFR event for selection operations
On Fri, 17 Nov 2023 16:22:55 GMT, Tim Prinzing wrote:
> Added mirror event with static methods: jdk.internal.event.SelectionEvent
> that provides the duration of select calls and the count of how many keys are
> available.
>
> Emit the event from SelectorImpl::lockAndDoSelect
>
> Test at jdk.jfr.event.io.TestSelectionEvents
Yes, in JDKEvents this is the list:
private static final Class[] instrumentationClasses = new Class[] {
FileInputStreamInstrumentor.class,
FileOutputStreamInstrumentor.class,
RandomAccessFileInstrumentor.class,
FileChannelImplInstrumentor.class
};
On Tue, Nov 28, 2023 at 2:43 PM Alan Bateman ***@***.***>
wrote:
> Isn't this a replacement for the existing instrumentation? If these are
> new events I am sorry.
>
> It's a new event.
>
> What is keeping us from removing ASM in that case?
>
> I think file I/O, there is still instrumentation used for FileInputStream,
> FileOutputStream, RandomAccessFile and nio.FileChannel.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/openjdk/jdk/pull/16710#issuecomment-1830703533>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AWJ25UH2YJZFS5RQLAIXAH3YGZEHZAVCNFSM6AA7QBCIFGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZQG4YDGNJTGM>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
-
PR Comment: https://git.openjdk.org/jdk/pull/16710#issuecomment-1830842223
Re: RFR: 8310994: Add JFR event for selection operations [v2]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Change event generation: - selectNow is filtered out - select that times out is always sent - select without timeout uses duration test - rename event to SelectorSelect, field to selectionKeyCount. - Merge branch 'master' into JDK-8310994 - remove trailing whitespace - event logic outside of the lock, selector in try block - remove unused import - fix TestConfigure failure - add event defaults - Merge branch 'master' into JDK-8310994 - minor test cleanup - ... and 2 more: https://git.openjdk.org/jdk/compare/a4905c7c...2f7dafd8 - Changes: - all: https://git.openjdk.org/jdk/pull/16710/files - new: https://git.openjdk.org/jdk/pull/16710/files/fbb93112..2f7dafd8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=00-01 Stats: 73918 lines in 2175 files changed: 42613 ins; 21404 del; 9901 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Fri, 8 Dec 2023 06:30:21 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/jdk/internal/event/SelectionEvent.java line 38:
>>
>>> 36: public class SelectionEvent extends Event {
>>> 37:
>>> 38: public int count;
>>
>> It could also be interesting to provide the `timeout` that was given to the
>> selection operation.
>
>> It could also be interesting to provide the `timeout` that was given to the
>> selection operation.
>
> I've tried to work through issues, esp. around selector spinning, and being
> able to distinguish select from selectNow is important for all of them, so
> yes, the timeout is needed or else no emit when the timeout == 0 as that's
> the case you have to filter out when troubleshooting.
I've added filtering of selectNow(), and an event is emitted if there is a
timeout independent of the threshold. The duration should roughly equal the
timout in that case. I added more test cases to cover those two changes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425744977
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Thu, 23 Nov 2023 11:18:45 GMT, Erik Gahlin wrote:
>> src/jdk.jfr/share/classes/jdk/jfr/events/SelectionEvent.java line 43:
>>
>>> 41:
>>> 42: @Label("Selection Count")
>>> 43: @Description("Number of channels selected")
>>
>> I suspect you'll need to rename this event to something like
>> "SelectorSelect" as "Selection" could be anything.
>>
>> We'll to find a better name for the field and the label too. There are two
>> forms of selection operations. One form operates on a selected-key set where
>> the select/selectNow methods returns the number of keys aded to the
>> Selector's ready set. The other form performs an action on each selected
>> key. I'll try to come up a suggestions for the names, I suspect a label
>> "number of channels ready for I/O or added to ready set" would be the most
>> accurate.
>
> It would also be good if the name reflect that it is related to channels so
> it won't clash with other events in the future.
I've made the suggested changes. I changed the "count" field to
"selectorKeyCount" which hopefully seems reasonable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425746321
Re: RFR: 8310994: Add JFR event for selection operations [v2]
On Wed, 13 Dec 2023 18:47:50 GMT, Daniel Fuchs wrote:
>> Tim Prinzing has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 12 additional
>> commits since the last revision:
>>
>> - Change event generation:
>>
>>- selectNow is filtered out
>>- select that times out is always sent
>>- select without timeout uses duration test
>> - rename event to SelectorSelect, field to selectionKeyCount.
>> - Merge branch 'master' into JDK-8310994
>> - remove trailing whitespace
>> - event logic outside of the lock, selector in try block
>> - remove unused import
>> - fix TestConfigure failure
>> - add event defaults
>> - Merge branch 'master' into JDK-8310994
>> - minor test cleanup
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/f9910028...2f7dafd8
>
> src/java.base/share/classes/jdk/internal/event/SelectorSelectEvent.java line
> 41:
>
>> 39: public class SelectorSelectEvent extends Event {
>> 40:
>> 41: public int selectionKeyCount;
>
> I still believe we should record the timeout parameter in the event.
Good point about the wakeup. I'll add the field.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1425765145
Re: RFR: 8310994: Add JFR event for selection operations [v3]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: add select timeout field to the event - Changes: - all: https://git.openjdk.org/jdk/pull/16710/files - new: https://git.openjdk.org/jdk/pull/16710/files/2f7dafd8..13262293 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=01-02 Stats: 21 lines in 4 files changed: 12 ins; 3 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
Re: RFR: 8310994: Add JFR event for selection operations [v4]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: comment fixup - Changes: - all: https://git.openjdk.org/jdk/pull/16710/files - new: https://git.openjdk.org/jdk/pull/16710/files/13262293..18064cd1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=02-03 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
Integrated: 8319571: Update jni/nullCaller/NullCallerTest.java to accept flags or mark as flagless
On Fri, 17 Nov 2023 16:27:44 GMT, Tim Prinzing wrote: > Updated to use ProcessTool helper class to run test passing flags. This pull request has now been integrated. Changeset: b78896b9 Author: Tim Prinzing Committer: Mandy Chung URL: https://git.openjdk.org/jdk/commit/b78896b9aafcb15f453eaed6e154a5461581407b Stats: 27 lines in 1 file changed: 1 ins; 21 del; 5 mod 8319571: Update jni/nullCaller/NullCallerTest.java to accept flags or mark as flagless Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/16711
Re: RFR: 8310994: Add JFR event for selection operations [v5]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 16 commits: - update copyright dates - Merge branch 'master' into JDK-8310994 # Conflicts: #src/jdk.jfr/share/classes/jdk/jfr/internal/MirrorEvents.java #src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java - comment fixup - add select timeout field to the event - Change event generation: - selectNow is filtered out - select that times out is always sent - select without timeout uses duration test - rename event to SelectorSelect, field to selectionKeyCount. - Merge branch 'master' into JDK-8310994 - remove trailing whitespace - event logic outside of the lock, selector in try block - remove unused import - ... and 6 more: https://git.openjdk.org/jdk/compare/36246c97...2e11e84a - Changes: https://git.openjdk.org/jdk/pull/16710/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=04 Stats: 321 lines in 9 files changed: 317 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
RFR: 8329138: Convert JFR FileForceEvent to static mirror event
Currently the JFR event FileForceEvent is generated by instrumenting the sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer mirror events with static methods. Added the event at jdk.internal.event.FileForceEvent, and changed jdk.jfr.events.FileForceEvent to be a mirror event. Updated FileChannelImpl to use the jdk internal event static methods, and removed the force() method from FileChannelImplInstrumentor. Uses the existing tests. - Commit messages: - javadoc fixup - remove mirrors from JDKEvents - 8329138: Convert JFR FileForceEvent to static mirror event Changes: https://git.openjdk.org/jdk/pull/18542/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329138 Stats: 159 lines in 7 files changed: 123 ins; 30 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event
On Fri, 29 Mar 2024 00:52:46 GMT, Tim Prinzing wrote: > Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Okay, I'll update those two places to emit the event. It looks like adding a file descriptor property to the event is needed, and there would be no file path in those cases. - PR Comment: https://git.openjdk.org/jdk/pull/18542#issuecomment-2027722419
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v2]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: Add support for AsynchronousFileChannel.force(). - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/005db0df..8e20527e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=00-01 Stats: 124 lines in 5 files changed: 111 ins; 3 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8310994: Add JFR event for selection operations [v6]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: remove selector spin event attempt and associated test. - Changes: - all: https://git.openjdk.org/jdk/pull/16710/files - new: https://git.openjdk.org/jdk/pull/16710/files/2e11e84a..d10be83e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=04-05 Stats: 46 lines in 2 files changed: 1 ins; 43 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v3]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: requested changes - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/8e20527e..6903c823 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=01-02 Stats: 7 lines in 2 files changed: 4 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v2]
On Tue, 2 Apr 2024 04:48:37 GMT, Alan Bateman wrote:
>> Tim Prinzing has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add support for AsynchronousFileChannel.force().
>
> src/java.base/share/classes/jdk/internal/event/FileForceEvent.java line 35:
>
>> 33: * {@link #commit(long, long, String, boolean)} method
>> 34: * must be the same as the order of the fields.
>> 35: */
>
> You may have to re-word this comment to avoid confusion with the metaData
> parameter. That is, there is event meta data and there is file metaData, if
> you see what I mean.
I see what you mean. I reworded the javadoc.
> test/jdk/jdk/jfr/event/io/TestAsynchronousFileChannelEvents.java line 50:
>
>> 48:
>> 49: public static void main(String[] args) throws Throwable {
>> 50: File blah = File.createTempFile("blah", null);
>
> Can you change this to use the tests's scratch rather that /tmp, meaning
> `Files.createTempFile(Path.of("."), "blah", "jfr")`. That way the recording
> is available in the event that the test fails.
I'm not sure what you mean about the recording. The file is the
AsynchronousFileChannel under test and does not contain the event recording.
> test/jdk/jdk/jfr/event/io/TestAsynchronousFileChannelEvents.java line 64:
>
>> 62:
>> 63: data.flip();
>> 64: ch.write(data, 0);
>
> This just initiates the write operation, it doesn't wait until it completes.
> It returns a Future so adding .get() will ensure that it waits and that there
> is potentially data to write back to the file system.
I do realize the write doesn't wait. I was under the impression that flush()
does wait until everything has been flushed to disk. I went ahead and added
.get() as requested.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1566405842
PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1566413683
PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1566409791
Re: RFR: 8310994: Add JFR event for selection operations [v7]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: cleanup of annotations - Changes: - all: https://git.openjdk.org/jdk/pull/16710/files - new: https://git.openjdk.org/jdk/pull/16710/files/d10be83e..9ece2ff5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=05-06 Stats: 5 lines in 1 file changed: 0 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v4]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: fix windows build issue - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/6903c823..70815943 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=02-03 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8310994: Add JFR event for selection operations [v8]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: more annotation fixup - Changes: - all: https://git.openjdk.org/jdk/pull/16710/files - new: https://git.openjdk.org/jdk/pull/16710/files/9ece2ff5..e896c4ec Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=06-07 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: test file local to test - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/70815943..366fca19 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=03-04 Stats: 5 lines in 1 file changed: 1 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v2]
On Tue, 16 Apr 2024 15:49:08 GMT, Daniel Fuchs wrote: >> I'm not sure what you mean about the recording. The file is the >> AsynchronousFileChannel under test and does not contain the event recording. > > It's anyway better to create temporary files in the test scratch directory > rather than in /tmp. This way the temp files get cleaned up with the test, > and there less chances of conflicts if several tests are running concurrently > (and less chances on slowly filling up /tmp onthe machine if clean up actions > fail) I moved it to the tests scratch area - PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1568102079
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Thu, 18 Apr 2024 14:32:28 GMT, Erik Gahlin wrote: >> src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 66: >> >>> 64: FileWriteEvent.class, >>> 65: SocketReadEvent.class, >>> 66: SocketWriteEvent.class, >> >> I'm guessing that this change which remove these two event classes is a >> drive-by-cleanup that should actually have been done with some previous fix >> in this area? >> Just wanted to double check it was intended as it doesn't seem to be related >> to file events. > > I think it might be the cause for https://bugs.openjdk.org/browse/JDK-8329330 > I have sent out a PR to remove those separately so the fix can be backported. > https://github.com/openjdk/jdk/pull/18843 That's correct, it is an unrelated cleanup (other than it seems to cause tests to fail now). @egahlin, do you want me to remove those from this PR? - PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1570953838
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v6]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: undo fix being handled in JDK-8329330. - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/366fca19..28bf429d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=04-05 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v6]
On Thu, 18 Apr 2024 18:46:36 GMT, Tim Prinzing wrote: >> Currently the JFR event FileForceEvent is generated by instrumenting the >> sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer >> mirror events with static methods. >> >> Added the event at jdk.internal.event.FileForceEvent, and changed >> jdk.jfr.events.FileForceEvent to be a mirror event. >> >> Updated FileChannelImpl to use the jdk internal event static methods, and >> removed the force() method from FileChannelImplInstrumentor. >> >> Uses the existing tests. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > undo fix being handled in JDK-8329330. Prior to the last commit, this passed all tests tier 1-3 on all platform. It should be ready. - PR Comment: https://git.openjdk.org/jdk/pull/18542#issuecomment-2064929545
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v7]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: IntelliJ trying to help - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/28bf429d..17b0be7b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=05-06 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v8]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: remove unecessary blank line - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/17b0be7b..f7859fc4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=06-07 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v9]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Merge branch 'refs/heads/master' into JDK-8329138 - remove unecessary blank line - IntelliJ trying to help - undo fix being handled in JDK-8329330. - test file local to test - fix windows build issue - requested changes - Add support for AsynchronousFileChannel.force(). - javadoc fixup - remove mirrors from JDKEvents - ... and 1 more: https://git.openjdk.org/jdk/compare/819f3d6f...743b92a6 - Changes: https://git.openjdk.org/jdk/pull/18542/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=08 Stats: 279 lines in 12 files changed: 238 ins; 27 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v10]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: fix mistake in merge - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/743b92a6..8bc34431 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18542&range=08-09 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8310994: Add JFR event for selection operations [v9]
> Added mirror event with static methods: jdk.internal.event.SelectionEvent > that provides the duration of select calls and the count of how many keys are > available. > > Emit the event from SelectorImpl::lockAndDoSelect > > Test at jdk.jfr.event.io.TestSelectionEvents Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits: - fix merge - Merge branch 'refs/heads/master' into JDK-8310994 # Conflicts: #src/jdk.jfr/share/classes/jdk/jfr/internal/MirrorEvents.java - more annotation fixup - cleanup of annotations - remove selector spin event attempt and associated test. - update copyright dates - Merge branch 'master' into JDK-8310994 # Conflicts: #src/jdk.jfr/share/classes/jdk/jfr/internal/MirrorEvents.java #src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java - comment fixup - add select timeout field to the event - Change event generation: - selectNow is filtered out - select that times out is always sent - select without timeout uses duration test - ... and 11 more: https://git.openjdk.org/jdk/compare/819f3d6f...dd43f4fa - Changes: https://git.openjdk.org/jdk/pull/16710/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16710&range=08 Stats: 277 lines in 9 files changed: 273 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16710/head:pull/16710 PR: https://git.openjdk.org/jdk/pull/16710
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Thu, 11 Apr 2024 16:39:24 GMT, Daniel Fuchs wrote: >> I think it's okay for now. If there is another phase of this work to help >> diagnose spinning issues then it will need to re-visited. I'm very concerned >> about the possible changes for that second phase, but this first phase of >> instrumentation is not disruptive. > > OK. I am a little concerned about how often this event will be fired when > using the HttpClient - given that it's enabled by default. Idle connections > sitting in the pool will fire it at least once per connection every 1500ms. > That may not be too bad. Should I set the default to be fairly high (like maybe 1600ms)? I think to be useful people will have to set the threshold to something that fits their needs anyway. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1583825215
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v10]
On Mon, 29 Apr 2024 20:01:38 GMT, Tim Prinzing wrote: >> Currently the JFR event FileForceEvent is generated by instrumenting the >> sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer >> mirror events with static methods. >> >> Added the event at jdk.internal.event.FileForceEvent, and changed >> jdk.jfr.events.FileForceEvent to be a mirror event. >> >> Updated FileChannelImpl to use the jdk internal event static methods, and >> removed the force() method from FileChannelImplInstrumentor. >> >> Uses the existing tests. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > fix mistake in merge I went ahead and ran teir1 through teir3 tests on everything after the merge. Everything passes so this is ready for integration. - PR Comment: https://git.openjdk.org/jdk/pull/18542#issuecomment-2083946114
Integrated: 8329138: Convert JFR FileForceEvent to static mirror event
On Fri, 29 Mar 2024 00:52:46 GMT, Tim Prinzing wrote: > Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. This pull request has now been integrated. Changeset: f4caac8d Author:Tim Prinzing Committer: Alan Bateman URL: https://git.openjdk.org/jdk/commit/f4caac8dea1c95234743712386cb28a1ecb11197 Stats: 280 lines in 12 files changed: 238 ins; 28 del; 14 mod 8329138: Convert JFR FileForceEvent to static mirror event Reviewed-by: alanb, egahlin - PR: https://git.openjdk.org/jdk/pull/18542
Re: RFR: 8310996: Add JFR event for connect operations
On Wed, 16 Oct 2024 06:40:33 GMT, Alan Bateman wrote:
>> Adds a JFR event for socket connect operations.
>>
>> Existing tests TestSocketEvents and TestSocketChannelEvents modified to also
>> check for connect events.
>
> src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java line 624:
>
>> 622: SocketConnectEvent.commit(start, duration,
>> isa.getHostString(), address.getHostAddress(), port, connected);
>> 623: }
>> 624: }
>
> Would it be possible to update the JBS or PR description to indicate if the
> intent is to record an event when the connection cannot be established? I'm
> asking because the change will only record an event when a connection is
> successfully established ("connected" is always true here).
>
> JFR will record exceptions already of course but I think for troubleshooting
> purposes, recording an event when "connect" hangs and eventually fails is
> very useful to have.
Capturing all calls even if they threw an exception does seem pretty useful.
I'll update the JBS
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1807983535
Re: RFR: 8310996: Add JFR event for connect operations
On Wed, 16 Oct 2024 06:46:54 GMT, Alan Bateman wrote: >> Adds a JFR event for socket connect operations. >> >> Existing tests TestSocketEvents and TestSocketChannelEvents modified to also >> check for connect events. > > src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java line 1006: > >> 1004: boolean connected = implConnect(sa); >> 1005: SocketConnectEvent.offer(start, connected, sa); >> 1006: return connected; > > It would be useful if the JBS or PR could say what the intent it for > SocketChannels that are configured non-blocking. I assume that implConnect > will execute in <20ms (the threshold in the JFR config) so no event will be > ever be recorded when configured non-blocking. It would be possible to save > the timestamp when connecting, and then use in finishConnect but that does > depend on timely usage. The non-blocking case is poorly handled. I'll update the JBS. The untimely use of finishConnect would cause an artificially bad looking event duration which might be a bit misleading. - PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1807984988
Re: RFR: 8310996: Add JFR event for connect operations [v3]
> Adds a JFR event for socket connect operations. > > Existing tests TestSocketEvents and TestSocketChannelEvents modified to also > check for connect events. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: improved exception names and handling - Changes: - all: https://git.openjdk.org/jdk/pull/21528/files - new: https://git.openjdk.org/jdk/pull/21528/files/7113bd75..a8898ffc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=01-02 Stats: 33 lines in 3 files changed: 9 ins; 8 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/21528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21528/head:pull/21528 PR: https://git.openjdk.org/jdk/pull/21528
Re: RFR: 8310996: Add JFR event for connect operations [v2]
> Adds a JFR event for socket connect operations. > > Existing tests TestSocketEvents and TestSocketChannelEvents modified to also > check for connect events. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: Added support for connection failure and non-blocking connections. If an exception is thrown while attempting a connection, the message from the exception is stored in the event. The start time of the initial connect call is stored and used when a finishConnect call is successful or an exception is thrown. - Changes: - all: https://git.openjdk.org/jdk/pull/21528/files - new: https://git.openjdk.org/jdk/pull/21528/files/05227c9d..7113bd75 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=00-01 Stats: 91 lines in 4 files changed: 44 ins; 14 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/21528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21528/head:pull/21528 PR: https://git.openjdk.org/jdk/pull/21528
Re: RFR: 8310996: Add JFR event for connect operations [v4]
> Adds a JFR event for socket connect operations. > > Existing tests TestSocketEvents and TestSocketChannelEvents modified to also > check for connect events. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: suggested changes - Changes: - all: https://git.openjdk.org/jdk/pull/21528/files - new: https://git.openjdk.org/jdk/pull/21528/files/a8898ffc..ce9d39e2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=02-03 Stats: 76 lines in 2 files changed: 49 ins; 15 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/21528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21528/head:pull/21528 PR: https://git.openjdk.org/jdk/pull/21528
RFR: 8310996: Add JFR event for connect operations
Adds a JFR event for socket connect operations. Existing tests TestSocketEvents and TestSocketChannelEvents modified to also check for connect events. - Commit messages: - fix default settings - fix merge - Merge branch 'master' into JDK-8310996 - added tests and support for sockets. - 8310996: Add JFR event for connect operations Changes: https://git.openjdk.org/jdk/pull/21528/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310996 Stats: 247 lines in 12 files changed: 236 ins; 3 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/21528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21528/head:pull/21528 PR: https://git.openjdk.org/jdk/pull/21528
Re: RFR: 8310996: Add JFR event for connect operations [v4]
On Wed, 6 Nov 2024 07:31:37 GMT, Alan Bateman wrote:
>> Tim Prinzing has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> suggested changes
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/JDKEvents.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights
>> reserved.
>
> I assume you didn't mean to change that.
Not sure how that happened, but I'll fix it
> test/jdk/jdk/jfr/event/io/TestSocketChannelEvents.java line 106:
>
>> 104:
>> 105: try (SocketChannel sc =
>> SocketChannel.open(ssc.getLocalAddress())) {
>> 106:
>> addExpectedEvent(IOEvent.createSocketConnectEvent(sc.socket()));
>
> This is SocketChannel in blocking mode where the connect succeeds. There is
> also the non-blocking and where connect fails. In addition the connection can
> established with the socket adaptor. So 6 possible cases for SocketChannel if
> the test is expanded.
yes, testing still to be done
> test/jdk/jdk/jfr/event/io/TestSocketEvents.java line 108:
>
>> 106: try (Socket s = new Socket()) {
>> 107: s.connect(ss.getLocalSocketAddress());
>> 108:
>> addExpectedEvent(IOEvent.createSocketConnectEvent(s));
>
> This is Socket.connect success case, I assume we'll need a test for fail case
> too.
yes
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1831527507
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1831529879
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1831529362
Re: RFR: 8310996: Add JFR event for connect operations [v6]
> Adds a JFR event for socket connect operations. > > Existing tests TestSocketEvents and TestSocketChannelEvents modified to also > check for connect events. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: split socket connect failure out to its own event. - Changes: - all: https://git.openjdk.org/jdk/pull/21528/files - new: https://git.openjdk.org/jdk/pull/21528/files/13f81570..f7b3be00 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=04-05 Stats: 327 lines in 16 files changed: 240 ins; 21 del; 66 mod Patch: https://git.openjdk.org/jdk/pull/21528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21528/head:pull/21528 PR: https://git.openjdk.org/jdk/pull/21528
Re: RFR: 8310996: Add JFR event for connect operations [v7]
> Adds a JFR event for socket connect operations. > > Existing tests TestSocketEvents and TestSocketChannelEvents modified to also > check for connect events. Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Merge branch 'master' into JDK-8310996 # Conflicts: #src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java - split socket connect failure out to its own event. - Added more tests for socket connect events. - SocketAdapter connect - SocketAdapter connect with exception - Socket connect with exception - SocketChannel connect with exception - SocketChannel non-blocking connect - SocketChannel non-blocking connect with exception - suggested changes - improved exception names and handling - Added support for connection failure and non-blocking connections. If an exception is thrown while attempting a connection, the message from the exception is stored in the event. The start time of the initial connect call is stored and used when a finishConnect call is successful or an exception is thrown. - fix default settings - fix merge - Merge branch 'master' into JDK-8310996 # Conflicts: #src/jdk.jfr/share/classes/jdk/jfr/internal/JDKEvents.java #src/jdk.jfr/share/classes/jdk/jfr/internal/MirrorEvents.java - added tests and support for sockets. - ... and 1 more: https://git.openjdk.org/jdk/compare/dfa5620f...a379609e - Changes: https://git.openjdk.org/jdk/pull/21528/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=06 Stats: 738 lines in 16 files changed: 686 ins; 7 del; 45 mod Patch: https://git.openjdk.org/jdk/pull/21528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21528/head:pull/21528 PR: https://git.openjdk.org/jdk/pull/21528
Re: RFR: 8310996: Add JFR event for connect operations [v8]
> Adds a JFR event for socket connect operations. > > Existing tests TestSocketEvents and TestSocketChannelEvents modified to also > check for connect events. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: requests fixes - Use IOException.toString() instead of getMessage() in case it's empty - Attempts to test connect exceptions may fail due to unexpected successful connect. Tests quit with uncompleted status if the connect is successful and are retried a small number of times until the test can be performed properly. If the retries are exceeded an exception is generated indicating the test can't be setup properly. - Changes: - all: https://git.openjdk.org/jdk/pull/21528/files - new: https://git.openjdk.org/jdk/pull/21528/files/a379609e..5e55ffc6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=06-07 Stats: 62 lines in 6 files changed: 43 ins; 3 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/21528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21528/head:pull/21528 PR: https://git.openjdk.org/jdk/pull/21528
Re: RFR: 8310996: Add JFR event for connect operations [v8]
On Thu, 12 Dec 2024 09:48:45 GMT, Daniel Fuchs wrote: >> Tim Prinzing has updated the pull request incrementally with one additional >> commit since the last revision: >> >> requests fixes >> >> - Use IOException.toString() instead of getMessage() in case it's empty >> - Attempts to test connect exceptions may fail due to unexpected >> successful connect. Tests quit with uncompleted status if the >> connect is successful and are retried a small number of times until >> the test can be performed properly. If the retries are exceeded an >> exception is generated indicating the test can't be setup properly. > > test/jdk/jdk/jfr/event/io/TestSocketAdapterEvents.java line 154: > >> 152: s.connect(addr); >> 153: // unexpected, abandon the test >> 154: return false; > > The main issue with using the ephemeral port range is that you might manage > to connect to a server opened by another test, and that might cause the other > test to fail if it's not expecting connections to get closed. > > If instead you use ports in the IANA reserved port range - at least you know > that you won't connect to other tests running on the same machine. > > Have you tried to connect to port 225 for instance, and increase the port > number up to 241 in case you still manage to connect? > > Ports 225-241 are reserved by IANA - so there should be nobody listening > there. I had some trouble on windows 2016 with port 47, but hopefully ports > 225-241 will not have the same issue. I've changed the exception tests to use the port range you suggested. - PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1887538817
Re: RFR: 8310996: Add JFR event for connect operations [v9]
> Adds a JFR event for socket connect operations. > > Existing tests TestSocketEvents and TestSocketChannelEvents modified to also > check for connect events. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: Refactored connect exception tests to a single IOHelper method that takes a function argument that is excpected to produce the appropriate exception. IANA reserved ports in the range 225 to 241 are used to attempt to produce a connect exception. - Changes: - all: https://git.openjdk.org/jdk/pull/21528/files - new: https://git.openjdk.org/jdk/pull/21528/files/5e55ffc6..81cea489 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=07-08 Stats: 206 lines in 4 files changed: 54 ins; 114 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/21528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21528/head:pull/21528 PR: https://git.openjdk.org/jdk/pull/21528
Re: RFR: 8310996: Add JFR event for connect operations [v5]
> Adds a JFR event for socket connect operations. > > Existing tests TestSocketEvents and TestSocketChannelEvents modified to also > check for connect events. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: Added more tests for socket connect events. - SocketAdapter connect - SocketAdapter connect with exception - Socket connect with exception - SocketChannel connect with exception - SocketChannel non-blocking connect - SocketChannel non-blocking connect with exception - Changes: - all: https://git.openjdk.org/jdk/pull/21528/files - new: https://git.openjdk.org/jdk/pull/21528/files/ce9d39e2..13f81570 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21528&range=03-04 Stats: 177 lines in 5 files changed: 162 ins; 0 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/21528.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21528/head:pull/21528 PR: https://git.openjdk.org/jdk/pull/21528
