On Sat, 31 May 2025 20:13:01 GMT, Markus Grönlund <mgron...@openjdk.org> 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/Throttle.java line 39:
>
>> 37: * example, {@code "100/s"}).
>> 38: * <p>
>> 39: * If the event class annotated with {@code Throttle} are filtered by
>> other
>
> "is filtered"
Will fix
> src/jdk.jfr/share/classes/jdk/jfr/internal/ClassInspector.java line 148:
>
>> 146: return true;
>> 147: }
>> 148: if (superClass != null) {
>
> Does this also need to search superClass's super?
The annotation is inherited so superClass's super will be included.
> src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java line 256:
>
>> 254: // }
>> 255: getEventConfiguration(codeBuilder);
>> 256: codeBuilder.aload(0);
>
> Can issue a dup() here if you want to avoid the second aload(0).
I don't think it will work because the result of the first getField (startTime)
will be on the stack, when we issue the next getField (duration).
> src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java line 257:
>
>> 255: getEventConfiguration(codeBuilder);
>> 256: codeBuilder.aload(0);
>> 257: getfield(codeBuilder, eventClassDesc,
>> ImplicitFields.FIELD_START_TIME);
>
> In native,we use the endTime for duration events? Is there a need to
> synchronize the two?
The duration is added later, in throttle. There is no this.endTime field to
read here.
> src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java line 590:
>
>> 588: private void setDuration(CodeBuilder codeBuilder,
>> Consumer<CodeBuilder> expression) {
>> 589: codeBuilder.aload(0);
>> 590: expression.accept(codeBuilder);
>
> dont know what expression.accept() does, but does it really consume the this
> pointer? I see its pushed again (aload(0)) if its throttled below?
We need "this" on stack before the arguments are loaded in expression which is
to be used at the end of the method by putfield
> src/jdk.jfr/share/classes/jdk/jfr/internal/event/EventConfiguration.java line
> 59:
>
>> 57: // static boolean shouldThrottleCommit(long timestamp)
>> 58: public boolean shouldThrottleCommit(long timestamp) {
>> 59: return throttler.sample(timestamp);
>
> Can we assert on isEnabled?
Between the time of the enabled() check and this method, the state may change,
so it could lead to false positives in testing.
> 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.
I think we can skip it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2119136217
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2119135926
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2119135820
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2119134807
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2119134400
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2119132932
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2119136976
