On Fri, 30 May 2025 22:30:25 GMT, Erik Gahlin <[email protected]> wrote:
> Could I have review of an enhancement that adds rate-limited sampling to Java
> event, including five events in the JDK (SocketRead, SocketWrite, FileRead,
> FileWrite, and JavaExceptionThrow).
>
> Testing: test/jdk/jdk/jfr
>
> Thanks
> Erik
src/jdk.jfr/share/classes/jdk/jfr/internal/settings/Throttler.java line 62:
> 60: // Not synchronized in fast path, but uses volatile reads.
> 61: public boolean sample(long ticks) {
> 62: if (disabled) {
This volatile load is somewhat disappointing. Do you think it is needed? What
happens if it is read without happens-before? It just creates an event that
will most likely get discarded by the recorder engine on reset (if its set on
setting update). If it's set to disabled, then the recorder engine has most
likely stopped already, so the event will be discarded. Event settings are set
with no visibility guarantees as to exact when they apply, so it should not
really matter when it goes to disabled.
src/jdk.jfr/share/classes/jdk/jfr/internal/settings/Throttler.java line 74:
> 72: }
> 73: }
> 74: return activeWindow.sample();
In native, a ticks value outside of the window is not returned true.
src/jdk.jfr/share/classes/jdk/jfr/internal/settings/Throttler.java line 76:
> 74: return activeWindow.sample();
> 75: }
> 76: return current.sample();
This could result in sampling in an already expired window.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118281909
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118282232
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118283830
