On Fri, 30 May 2025 22:30:25 GMT, Erik Gahlin <egah...@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/internal/EventInstrumentation.java line 261:
> 259: getfield(codeBuilder, eventClassDesc,
> ImplicitFields.FIELD_DURATION);
> 260: Bytecode.invokevirtual(codeBuilder,
> TYPE_EVENT_CONFIGURATION, METHOD_THROTTLE);
> 261: int result = codeBuilder.allocateLocal(TypeKind.LONG);
Do we really need to store the result in a local? Can't we just dup it on the
expression stack and store it directly into the field after another aload, or
dup? Perhaps dup twice to then issue the mask operation?
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?
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?
src/jdk.jfr/share/classes/jdk/jfr/internal/event/EventConfiguration.java line
77:
> 75: return rawDuration;
> 76: }
> 77: long endTime = startTime + rawDuration;
I see now that you are using endTime as well, nice.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118272073
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118272829
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118277498
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118279057
