[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-build-dev
Subject:    Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
From:       Erik Gahlin <egahlin () openjdk ! org>
Date:       2023-09-22 13:06:34
Message-ID: 5tdptBPPCTyNYn8wbZ1_bN52j8C-YRkWpmMksczutQs=.2700421a-3722-4d42-b186-482e38dba8d7 () github ! com
[Download RAW message or body]

On Tue, 19 Sep 2023 15:35:11 GMT, Tim Prinzing <tprinzing@openjdk.org> wrote:

> > The socket read/write JFR events currently use instrumentation of java.base code \
> > using templates in the jdk.jfr modules. This results in some java.base code \
> > residing in the jdk.jfr module which is undesirable. 
> > JDK19 added static support for event classes. The old instrumentor classes should \
> > be replaced with mirror events using the static support. 
> > In the java.base module:
> > Added two new events, jdk.internal.event.SocketReadEvent and \
> > jdk.internal.event.SocketWriteEvent. java.net.Socket and \
> > sun.nio.ch.SocketChannelImpl were changed to make use of the new events. 
> > In the jdk.jfr module:
> > jdk.jfr.events.SocketReadEvent and jdk.jfr.events.SocketWriteEvent were changed \
> > to be mirror events. In the package jdk.jfr.internal.instrument, the classes \
> > SocketChannelImplInstrumentor, SocketInputStreamInstrumentor, and \
> > SocketOutputStreamInstrumentor were removed. The JDKEvents class was updated to \
> > reflect all of those changes. 
> > The existing tests in test/jdk/jdk/jfr/event/io continue to pass with the new \
> >                 implementation:
> > Passed: jdk/jfr/event/io/TestSocketChannelEvents.java
> > Passed: jdk/jfr/event/io/TestSocketEvents.java
> > 
> > I added a micro benchmark which measures the overhead of handling the jfr socket \
> > events. test/micro/org/openjdk/bench/java/net/SocketEventOverhead.java.
> > It needs access the jdk.internal.event package, which is done at runtime with \
> > annotations that add the extra arguments. At compile time the build arguments had \
> > to be augmented in make/test/BuildMicrobenchmark.gmk
> 
> Tim Prinzing has updated the pull request with a new target base due to a merge or \
> a rebase. The pull request now contains 12 commits: 
> - Merge branch 'master' into JDK-8308995
> - More changes from review:
> 
> I didn't like the name of the helper method 'checkForCommit' because it
> doesn't indicate that it might commit the event.  I also don't like
> 'commitEvent' because it might not.  Since JFR events are sort of like a
> queue I went with a name from collections and called it 'offer' so using
> it is something like 'SocketReadEvent.offer(...)' which seems like it
> gets the idea across better.  Also improved the javadoc for it.
> 
> Removed the comments about being instrumented by JFR in
> Socket.SocketInputStream and Socket.SocketOutputStream.
> 
> I went ahead and moved the event commiting out of the finally block so
> that we don't emit events when the read/write did not actually happen.
> The bugid JDK-8310979 will be used to determine if more should be done
> in this area.
> 
> The implRead and implWrite were moved up with the other support methods
> for read/write.
> - less exception filtering when fetching socket read timeout
> - remove unused SOCKET_READ and SOCKET_WRITE configurations.
> - Merge branch 'master' into JDK-8308995
> 
> # Conflicts:
> #	src/jdk.jfr/share/classes/jdk/jfr/events/EventConfigurations.java
> - Avoid exceptions getting address/timeout for jfr event. Remove unused
> EventConiguration fields SOCKET_READ and SOCKET_WRITE.  Remove spurious
> whitespace.
> - some changes from review.
> 
> read0() to implRead()
> write0() to implWrite()
> trailing whitespace
> - fix copyright date
> - Added micro benchmark to measure socket event overhead.
> - Some changes from review.
> 
> Append a 0 to method names being wrapped.  Use getHostString to avoid
> a reverse lookup when fetching the hostname of the remote address.
> - ... and 2 more: https://git.openjdk.org/jdk/compare/607bd4ed...6db6fab4

The new implementation calls getSocketRemoteAddress() and getSOTimeout() regardless \
if the event duration exceeds the threshold or not. This adds unnecessary overhead. \
Most socket write/reads are not committed, only those that take more than 20 ms (by \
default). I think you need something like this:

if (SocketRead.shouldCommit(...)) {
  ...
}

-------------

PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1731379349


[prev in list] [next in list] [prev in thread] [next in thread]