On Tue, 3 Jun 2025 12:50:49 GMT, Erik Gahlin <egah...@openjdk.org> wrote:
>> Could I have review of an enhancement that adds rate-limited sampling to
>> Java events, including five events in the JDK (SocketRead, SocketWrite,
>> FileRead, FileWrite, and JavaExceptionThrow).
>>
>> Testing: test/jdk/jdk/jfr
>>
>> Thanks
>> Erik
>
> Erik Gahlin has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix adjust boundary
src/java.base/share/classes/java/net/Socket.java line 970:
> 968: long end = SocketReadEvent.timestamp();
> 969: long duration = end - start;
> 970: if (SocketReadEvent.shouldThrottleCommit(duration, end)) {
The use sites want to ask if an event should be committed. Does the word
"Throttle" need to be in name as I don't think the use cases need to care about
this.
Also, just to point out that the shouldXXX method is called with the the end
timestamp and duration whereas the offset/emit/commit methods are called with
the start timestamp and duration. So two long timestamps and a long measure of
time, easy to get it wrong somewhere. Maybe not this PR but I think would be
clearer at the use sites to use start or end consistently and reduce potential
for mistakes.
src/jdk.jfr/share/classes/jdk/jfr/Throttle.java line 77:
> 75: * Specifying {@code "off"} (case-sensitive) results in all events
> being emitted.
> 76: *
> 77: * @return the throttle value, default {@code "off"} not {@code null}
I think it would be clear to drop "not null" from the return description.
src/jdk.jfr/share/conf/jfr/default.jfc line 762:
> 760: <setting name="enabled">true</setting>
> 761: <setting name="stackTrace">true</setting>
> 762: <setting name="threshold">20 ms</setting>
I agree FileForce isn't a good candidate to throttle.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2126765474
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2126722810
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2126719134
