On Wed, 28 Jun 2023 06:09:14 GMT, Alan Bateman <al...@openjdk.org> 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
