Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Mon, 29 Apr 2024 21:53:02 GMT, Tim Prinzing wrote: > 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. I wonder about the usefulness of this event if the default threshold is so high. My guess is that it's rare to change the default threshold. To your question then I suppose this comes back to how this event might be used. Maybe it's worth writing down a few possible usages and that might help to inform if the threshold should be 20ms or something else. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1585104188
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: 8310994: Add JFR event for selection operations [v3]
On Thu, 11 Apr 2024 16:08:31 GMT, Alan Bateman wrote: >> We should probably find a way to not emit the event if n == 0 and the >> operation was interrupted by `Selector.wakeUp`. Since we have another issue >> logged to emit a spin event, I wonder if we should only commit the event >> here if `n != 0`? The case where n == 0 would be handled by the spin event >> (added later) >> @AlanBateman what do you think? > > 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1561313983
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Thu, 11 Apr 2024 09:12:30 GMT, Daniel Fuchs wrote: >>> Maybe that's OK - and maybe in that case the onus is on the user to set a >>> threshold greater than 1500ms? >> >> The threshold is 20ms so these timed-select ops in the HTTP client will >> record an event when they timeout. > > We should probably find a way to not emit the event if n == 0 and the > operation was interrupted by `Selector.wakeUp`. Since we have another issue > logged to emit a spin event, I wonder if we should only commit the event here > if `n != 0`? The case where n == 0 would be handled by the spin event (added > later) > @AlanBateman what do you think? 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1561279163
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Mon, 26 Feb 2024 17:40:59 GMT, Alan Bateman wrote: >> Is n == 0 intended to detect a spinning condition where the selector goes >> back into select when the event has not been handled? >> >> In that case should we still emit an event if a timeout is present and the >> duration is greater than the timeout? For instance, in the HttpClient, we >> have a selector thread that will wake up at regular interval - we set up a >> timeout which is the min between the next expected timer event and 1500ms. >> So that could potentially fire an event every 1500ms if for instance the >> connection threshold is less than 1500ms and the connection is idle. >> >> Maybe that's OK - and maybe in that case the onus is on the user to set a >> threshold greater than 1500ms? >> >> Or should it be ((n == 0 && durationToMillis(duration) < >> timeoutToMillis(timeout)) || ...)) (duration and timeout probably are not in >> the same unit of time) - also if shouldCommit return false will the event >> actually be emitted down the road? Because if not then the ( n== 0 || ...) >> won't work. > >> Maybe that's OK - and maybe in that case the onus is on the user to set a >> threshold greater than 1500ms? > > The threshold is 20ms so these timed-select ops in the HTTP client will > record an event when they timeout. We should probably find a way to not emit the event if n == 0 and the operation was interrupted by `Selector.wakeUp`. Since we have another issue logged to emit a spin event, I wonder if we should only commit the event here if `n != 0`? The case where n == 0 would be handled by the spin event (added later) @AlanBateman what do you think? - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1560693482
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Mon, 26 Feb 2024 14:14:21 GMT, Daniel Fuchs wrote: > Maybe that's OK - and maybe in that case the onus is on the user to set a > threshold greater than 1500ms? The threshold is 20ms so these timed-select ops in the HTTP client will record an event when they timeout. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1503052233
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Wed, 3 Jan 2024 11:11:40 GMT, Alan Bateman wrote:
>> Tim Prinzing has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add select timeout field to the event
>
> src/java.base/share/classes/sun/nio/ch/SelectorImpl.java line 153:
>
>> 151: if ((n == 0) || (SelectorSelectEvent.shouldCommit(duration))) {
>> 152: timeout = (timeout < 0) ? 0 : timeout;
>> 153: SelectorSelectEvent.commit(start, duration, n, timeout);
>
> The comment is a bit confusing here. n == 0 means that no selected keys were
> added or consumed before timeout or wakeup.
Is n == 0 intended to detect a spinning condition where the selector goes back
into select when the event has not been handled?
In that case should we still emit an event if a timeout is present and the
duration is greater than the timeout? For instance, in the HttpClient, we have
a selector thread that will wake up at regular interval - we set up a timeout
which is the min between the next expected timer event and 1500ms. So that
could potentially fire an event every 1500ms if for instance the connection
threshold is less than 1500ms and the connection is idle.
Maybe that's OK - and maybe in that case the onus is on the user to set a
threshold greater than 1500ms?
Or should it be ((n == 0 && durationToMillis(duration) <
timeoutToMillis(timeout)) || ...)) (duration and timeout probably are not in
the same unit of time) - also if shouldCommit return false will the event
actually be emitted down the road? Because if not then the ( n== 0 || ...)
won't work.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1502681499
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Wed, 13 Dec 2023 22:20: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
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add select timeout field to the event
src/java.base/share/classes/sun/nio/ch/SelectorImpl.java line 143:
> 141: throws IOException
> 142: {
> 143: // filter selectNow ops from consideration (timeout == 0)
I think simplify this comment to say no JFR event for selectNow.
src/java.base/share/classes/sun/nio/ch/SelectorImpl.java line 153:
> 151: if ((n == 0) || (SelectorSelectEvent.shouldCommit(duration))) {
> 152: timeout = (timeout < 0) ? 0 : timeout;
> 153: SelectorSelectEvent.commit(start, duration, n, timeout);
The comment is a bit confusing here. n == 0 means that no selected keys were
added or consumed before timeout or wakeup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1440332584
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1440332569
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=16710=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16710=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
