On Mon, 2 Jun 2025 08:59:57 GMT, Volkan Yazici <vyaz...@openjdk.org> wrote:
>> Erik Gahlin has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Some reviewer feedback
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/settings/Throttler.java line 30:
>
>> 28: import java.util.concurrent.locks.ReentrantLock;
>> 29: import jdk.jfr.internal.PlatformEventType;
>> 30: public final class Throttler {
>
> **Disclaimer:** I am a passer-by and I don't possess a deep JFR experience.
>
> Throttling (aka. rate limiting) is a pretty much non-trivial functionality.
> Shall we
>
> - Revamp the JavaDoc and document the used algorithm?
> - Have a dedicated test for `Throttler`? (I see that the newly added
> `TestThrottle` tests `@Throttle`, hence it also covers `Throttler`. Though
> there I could not see any verification based on the time[stamp] of events.)
>
> ### References
>
> - Resilience4j:
> -
> [AtomicRateLimiter](https://github.com/resilience4j/resilience4j/blob/master/resilience4j-ratelimiter/src/main/java/io/github/resilience4j/ratelimiter/internal/AtomicRateLimiter.java)
> -
> [AtomicRateLimiterTest](https://github.com/resilience4j/resilience4j/blob/master/resilience4j-ratelimiter/src/test/java/io/github/resilience4j/ratelimiter/internal/AtomicRateLimiterTest.java)
> - Guava:
> -
> [RateLimiter](https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/RateLimiter.java)
> -
> [RateLimiterTest](https://github.com/google/guava/blob/master/guava-tests/test/com/google/common/util/concurrent/RateLimiterTest.java)
I don't think we want to write the algorithm in the specification. It would
make it harder for us to make adjustments in the future. The contract is x
events per time unit without details on how JFR adapts the sampling. The
algorithm is the same one used by the jdk.ObjectAllocationSample event, which
has been in use since JDK 16, but implemented in native code and with a gtest:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/jfr/support/jfrAdaptiveSampler.cpp
The code in the PR is a method per method port of that code. It's hard to write
stable tests that include timestamps. We have tried that in the past for other
settings/events, but it only resulted in false positives.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2120643350
